Enable a more strict set of warnings when building Windows targets (#28)

* Configure windows builds to be strict. All warnings are on, and are treated as errors. To make this tolerable, update CMake minimum version to 3.15 and enable CMP0092 to remove /W3 from the baseline compiler flags. * Group compile defs together, and set compile options for warnings before other compile options. * Fix warnings in Windows builds. * Fix unreachable code in release x86 build.
microsoft · Apr 2, 2020 · b34e554 · b34e554
1 parent 6fa2d46
commit b34e554
Show file tree

Hide file tree

Showing 19 changed files with 119 additions and 79 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -2,7 +2,8 @@
 # Licensed under the MIT license.
 
 # set minimum cmake version
-cmake_minimum_required(VERSION 3.14 FATAL_ERROR)
+cmake_minimum_required(VERSION 3.15 FATAL_ERROR)
+cmake_policy(SET CMP0092 NEW)
 
 # project name and language
 project(Mso

diff --git a/change/@microsoft-mso-2020-04-01-20-33-50-afoxman-win-strict.json b/change/@microsoft-mso-2020-04-01-20-33-50-afoxman-win-strict.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Turn on level 4 warnings, use non-permissive mode, and make all warnings show up as errors. Fix all impacted code.",
+  "packageName": "@microsoft/mso",
+  "email": "[email protected]",
+  "dependentChangeType": "patch",
+  "date": "2020-04-02T03:33:50.525Z"
+}
diff --git a/libs/compilerAdapters/include/compilerAdapters/compilerWarnings.h b/libs/compilerAdapters/include/compilerAdapters/compilerWarnings.h
@@ -29,6 +29,9 @@
   BEGIN_DISABLE_COMPILER_WARNING_CLANG("-Winconsistent-missing-override")
 #define END_DISABLE_WARNING_INCONSISTENT_MISSING_OVERRIDE() END_DISABLE_COMPILER_WARNING()
 
+#define BEGIN_DISABLE_WARNING_UNREACHABLE_CODE() BEGIN_DISABLE_COMPILER_WARNING_MSVC(4702)
+#define END_DISABLE_WARNING_UNREACHABLE_CODE() END_DISABLE_COMPILER_WARNING()
+
 #define BEGIN_DISABLE_WARNING_UNUSED_CONST_VARIABLE() BEGIN_DISABLE_COMPILER_WARNING_CLANG("-Wunused-const-variable")
 #define END_DISABLE_WARNING_UNUSED_CONST_VARIABLE() END_DISABLE_COMPILER_WARNING()
 

diff --git a/libs/functional/tests/functorTest.cpp b/libs/functional/tests/functorTest.cpp
@@ -100,11 +100,13 @@ static int TestFreeFunction_Subtract(int i) noexcept
   return g_freeFunctionState;
 }
 
+#if 0 // unused function; test which calls it is commented out
 static void TestFreeFunction_Throw(int i)
 {
   if (i < 0)
     throw std::runtime_error("test");
 }
+#endif
 
 TestClassComponent(FunctorTest, Mso.Functor) TEST_CLASS (FunctorTest)
 {

diff --git a/libs/memoryApi/include/core/memoryApi.h b/libs/memoryApi/include/core/memoryApi.h
@@ -46,7 +46,7 @@ struct ILibletMemoryMarking
 struct IMsoMemHeap;
 LIBLET_PUBLICAPI_EX("android", "win")
 MSOAPI_(BOOL)
-    FMemHeapMsoSaveBeHost(void* pinst, LPARAM lParam, const void* pvBlock, LONG_PTR cb, IMsoMemHeap* pmmh) MSONOEXCEPT;
+FMemHeapMsoSaveBeHost(void* pinst, LPARAM lParam, const void* pvBlock, LONG_PTR cb, IMsoMemHeap* pmmh) MSONOEXCEPT;
 LIBLET_PUBLICAPI_EX("android", "win") MSOAPI_(void) MsoCheckShutdownLeaks() noexcept;
 LIBLET_PUBLICAPI_EX("win") MSOAPI_(void) HeapEnableLeakTracking(bool isEnabled);
 LIBLET_PUBLICAPI_EX("win") MSOAPI_(void) MsoBeforeThreadTerminatesThreaded(DWORD mainThreadId) noexcept;

diff --git a/libs/motifCpp/include/motifCpp/motifCppTest.h b/libs/motifCpp/include/motifCpp/motifCppTest.h
@@ -2,13 +2,16 @@
 // Licensed under the MIT license.
 
 #pragma once
+
+#include <csetjmp>
+#include <csignal>
+#include <cstdarg>
+#include <functional>
 #include <type_traits>
+
+#include <compilerAdapters/compilerWarnings.h>
 #include <oacr.h>
-#include <functional>
 #include <object/IUnknownShim.h>
-#include <cstdarg>
-#include <csetjmp>
-#include <csignal>
 
 #if defined(MSO_USE_GTEST)
 #include <motifCpp/gTestAdapter.h>
@@ -228,8 +231,9 @@ inline DWORD FilterCrashExceptions(DWORD exceptionCode) noexcept
   return EXCEPTION_EXECUTE_HANDLER;
 }
 
+BEGIN_DISABLE_WARNING_UNREACHABLE_CODE()
 template <typename Fn>
-inline bool ExpectCrashCore(const Fn& fn, const WCHAR* message)
+inline bool ExpectCrashCore(const Fn& fn, const WCHAR* /*message*/)
 {
   __try
   {
@@ -243,6 +247,8 @@ inline bool ExpectCrashCore(const Fn& fn, const WCHAR* message)
   // Fail(message == nullptr || message[0] == L'\0' ? L"Test function did not crash!" : message);
   return false;
 }
+END_DISABLE_WARNING_UNREACHABLE_CODE()
+
 #else
 
 using SigAction = void (*)(int, siginfo_t*, void*);

diff --git a/libs/oacr/include/oacr.h b/libs/oacr/include/oacr.h
@@ -245,7 +245,7 @@
 // special reason.
 // e.g. a wrapper function to another function that needs to be reviewed
 //_Needsreview_ __inline BOOL MsoGetStringTypeExW(LCID Locale, DWORD dwInfoType, LPCWSTR lpSrcStr, int cchSrc, LPWORD
-//lpCharType)
+// lpCharType)
 //{
 //   return OACR_REVIEWED_CALL("hannesr", GetStringTypeExW(Locale, dwInfoType, lpSrcStr, cchSrc, lpCharType));
 //}

diff --git a/libs/object/include/object/make.h b/libs/object/include/object/make.h
@@ -9,8 +9,9 @@
 
 #ifdef __cplusplus
 
-#include <core/TCntPtr.h>
+#include <compilerAdapters/compilerWarnings.h>
 #include <compilerAdapters/cppMacrosDebug.h>
+#include <core/TCntPtr.h>
 #include <memoryApi/memoryApi.h>
 
 #pragma pack(push, _CRT_PACKING)
@@ -130,6 +131,7 @@ namespace MakePolicy {
 /// ThrowCtor MakePolicy passes all parameters to the constructor.
 /// ThrowCtor::Make may throw an exception if constructor throws.
 /// To allow this class to access private constructor add "friend MakePolicy;" to your class.
+BEGIN_DISABLE_WARNING_UNREACHABLE_CODE()
 struct ThrowCtor
 {
   static const bool IsNoExcept = false;
@@ -142,6 +144,7 @@ struct ThrowCtor
     memoryGuard.ObjMemory = nullptr; // Memory is now controlled by the object. Set to null to avoid memory destruction.
   }
 };
+END_DISABLE_WARNING_UNREACHABLE_CODE()
 
 /// NoThrowCtor MakePolicy passes all parameters to the constructor.
 /// NoThrowCtor::Make does not throw exception and crashes the app if constructor throws.

diff --git a/libs/object/include/object/swarm.h b/libs/object/include/object/swarm.h
@@ -8,6 +8,7 @@ share the same reference counter.
 
 #pragma once
 
+#include <compilerAdapters/compilerWarnings.h>
 #include <object/objectWithWeakRef.h>
 #include <object/queryCast.h>
 #include <object/weakPtr.h>
@@ -156,6 +157,7 @@ class Swarm : public ObjectWeakRef
 
   // We return swarm member as a raw pointer because the new object shares ref count with the swarm and in many cases
   // we want to avoid the extra AddRef/Release because object's lifetime is already tracked.
+BEGIN_DISABLE_WARNING_UNREACHABLE_CODE()
   template <typename T, typename TResult = T, typename... TArgs>
   TResult* MakeMember(TArgs&&... args) noexcept(T::MakePolicy::IsNoExcept)
   {
@@ -179,6 +181,7 @@ class Swarm : public ObjectWeakRef
     memoryGuard.Obj = nullptr; // To prevent memoryGuard from destroying the object.
     return result;
   }
+END_DISABLE_WARNING_UNREACHABLE_CODE()
 
   template <typename T, typename TResult = T, typename TAllocArg, typename... TArgs>
   TResult* MakeMemberAlloc(TAllocArg&& allocArg, TArgs&&... args) noexcept(T::MakePolicy::IsNoExcept)

diff --git a/libs/object/tests/TCntPtrRefTest.cpp b/libs/object/tests/TCntPtrRefTest.cpp
@@ -29,23 +29,23 @@ class SimpleTestRef : public Mso::TRefCountedImpl<ISimple>
   void DoSomething() override {}
 };
 
-inline static std::wstring ToString(ISimple* q)
+inline static std::wstring ToString(ISimple* /*q*/)
 {
   return L"";
 }
-inline static std::wstring ToString(ISimple** q)
+inline static std::wstring ToString(ISimple** /*q*/)
 {
   return L"";
 }
-inline static std::wstring ToString(const ISimple* q)
+inline static std::wstring ToString(const ISimple* /*q*/)
 {
   return L"";
 }
-inline static std::wstring ToString(ISimple* const* q)
+inline static std::wstring ToString(ISimple* const* /*q*/)
 {
   return L"";
 }
-inline static std::wstring ToString(const Mso::TCntPtr<ISimple>* q)
+inline static std::wstring ToString(const Mso::TCntPtr<ISimple>* /*q*/)
 {
   return L"";
 }

diff --git a/libs/object/tests/TCntPtrTest.cpp b/libs/object/tests/TCntPtrTest.cpp
@@ -123,19 +123,19 @@ class UnkSimpleClass : public IUnkSimple
   RefCountChangedCallback m_onRefCountChanged;
 };
 
-inline static std::wstring ToString(UnkSimpleClass* q)
+inline static std::wstring ToString(UnkSimpleClass* /*q*/)
 {
   return L"";
 }
-inline static std::wstring ToString(SimpleClass* q)
+inline static std::wstring ToString(SimpleClass* /*q*/)
 {
   return L"";
 }
-inline static std::wstring ToString(const UnkSimpleClass* q)
+inline static std::wstring ToString(const UnkSimpleClass* /*q*/)
 {
   return L"";
 }
-inline static std::wstring ToString(const SimpleClass* q)
+inline static std::wstring ToString(const SimpleClass* /*q*/)
 {
   return L"";
 }

diff --git a/libs/object/tests/fixedSwarmTest.cpp b/libs/object/tests/fixedSwarmTest.cpp
@@ -13,7 +13,7 @@ Unit tests for classes in the ObjectSwarm.h
 #include <test/testCheck.h>
 
 //#define TEST_BAD_INHERITANCE1 // Uncomment to confirm VEC, but observe a memory leak. We cannot safely destroy this
-//class.
+// class.
 
 MSO_STRUCT_GUID(IFixedSwarmSample1, "AA2EB60A-06DD-486F-AC9B-DBF1DDE21408")
 struct DECLSPEC_NOVTABLE IFixedSwarmSample1 : IUnknown
@@ -156,20 +156,20 @@ TEST_CLASS (ObjectFixedSwarmTest)
     Mso::TCntPtr<Swarm1> swarm1 = Mso::Make<Swarm1>();
     TestAssert::IsFalse(swarm1.IsEmpty());
 
-    Debug(TestAssert::AreEqual(1, swarm1->RefCount()));
-    Debug(TestAssert::AreEqual(1, swarm1->WeakRefCount()));
+    Debug(TestAssert::AreEqual(1u, swarm1->RefCount()));
+    Debug(TestAssert::AreEqual(1u, swarm1->WeakRefCount()));
 
     Mso::TCntPtr<Swarm1> swarm11 = swarm1;
-    Debug(TestAssert::AreEqual(2, swarm1->RefCount()));
-    Debug(TestAssert::AreEqual(1, swarm1->WeakRefCount()));
+    Debug(TestAssert::AreEqual(2u, swarm1->RefCount()));
+    Debug(TestAssert::AreEqual(1u, swarm1->WeakRefCount()));
 
     Mso::WeakPtr<Swarm1> weakSwarm1 = swarm1;
-    Debug(TestAssert::AreEqual(2, swarm1->RefCount()));
-    Debug(TestAssert::AreEqual(2, swarm1->WeakRefCount()));
+    Debug(TestAssert::AreEqual(2u, swarm1->RefCount()));
+    Debug(TestAssert::AreEqual(2u, swarm1->WeakRefCount()));
 
     Mso::WeakPtr<Swarm1> weakSwarm11 = swarm1;
-    Debug(TestAssert::AreEqual(2, swarm1->RefCount()));
-    Debug(TestAssert::AreEqual(3, swarm1->WeakRefCount()));
+    Debug(TestAssert::AreEqual(2u, swarm1->RefCount()));
+    Debug(TestAssert::AreEqual(3u, swarm1->WeakRefCount()));
   }
 
   TEST_METHOD(ObjectFixedSwarm_GetMember)
@@ -204,13 +204,13 @@ TEST_CLASS (ObjectFixedSwarmTest)
         TestAssert::IsTrue(created);
         TestAssert::IsFalse(deleted);
 
-        Debug(TestAssert::AreEqual(2, swarm1->RefCount()));
-        Debug(TestAssert::AreEqual(2, swarm1->WeakRefCount()));
+        Debug(TestAssert::AreEqual(2u, swarm1->RefCount()));
+        Debug(TestAssert::AreEqual(2u, swarm1->WeakRefCount()));
       }
 
       TestAssert::IsFalse(deleted);
-      Debug(TestAssert::AreEqual(1, swarm1->RefCount()));
-      Debug(TestAssert::AreEqual(2, swarm1->WeakRefCount()));
+      Debug(TestAssert::AreEqual(1u, swarm1->RefCount()));
+      Debug(TestAssert::AreEqual(2u, swarm1->WeakRefCount()));
     }
 
     TestAssert::IsTrue(deleted);
@@ -229,26 +229,26 @@ TEST_CLASS (ObjectFixedSwarmTest)
         swarm3->MakeMember<0>(created, deleted);
         Mso::TCntPtr<IFixedSwarmSample1> member0 = swarm3->Get<0>();
         TestAssert::IsFalse(member0.IsEmpty());
-        Debug(TestAssert::AreEqual(2, swarm3->RefCount()));
-        Debug(TestAssert::AreEqual(2, swarm3->WeakRefCount()));
+        Debug(TestAssert::AreEqual(2u, swarm3->RefCount()));
+        Debug(TestAssert::AreEqual(2u, swarm3->WeakRefCount()));
 
         swarm3->MakeMember<1>();
         Mso::TCntPtr<IFixedSwarmSample2> member1 = swarm3->Get<1>();
         TestAssert::IsFalse(member1.IsEmpty());
-        Debug(TestAssert::AreEqual(3, swarm3->RefCount()));
-        Debug(TestAssert::AreEqual(3, swarm3->WeakRefCount()));
+        Debug(TestAssert::AreEqual(3u, swarm3->RefCount()));
+        Debug(TestAssert::AreEqual(3u, swarm3->WeakRefCount()));
 
         swarm3->MakeMember<2>();
         Mso::TCntPtr<FixedSwarmMemberSample3> member2 = swarm3->Get<2>();
         TestAssert::IsFalse(member2.IsEmpty());
-        Debug(TestAssert::AreEqual(4, swarm3->RefCount()));
-        Debug(TestAssert::AreEqual(4, swarm3->WeakRefCount()));
+        Debug(TestAssert::AreEqual(4u, swarm3->RefCount()));
+        Debug(TestAssert::AreEqual(4u, swarm3->WeakRefCount()));
       }
 
       TestAssert::IsFalse(deleted);
-      Debug(TestAssert::AreEqual(1, swarm3->RefCount()));
+      Debug(TestAssert::AreEqual(1u, swarm3->RefCount()));
       // Each object +1 for weak reference count, +1 weak reference if strong ref count != 0.
-      Debug(TestAssert::AreEqual(4, swarm3->WeakRefCount()));
+      Debug(TestAssert::AreEqual(4u, swarm3->WeakRefCount()));
     }
 
     TestAssert::IsTrue(deleted);

diff --git a/libs/object/tests/lazyInitTest.cpp b/libs/object/tests/lazyInitTest.cpp
@@ -252,22 +252,22 @@ TEST_CLASS (LazyInitTests)
     TestAssert::IsFalse(instance.IsInitialized(), L"Instance should not be initialized after Release().");
   }
 
-  inline static std::wstring ToString(CountedType * q) noexcept
+  inline static std::wstring ToString(CountedType * /*q*/) noexcept
   {
     return L"CountedType";
   }
 
-  inline static std::wstring ToString(ICountedType * q) noexcept
+  inline static std::wstring ToString(ICountedType * /*q*/) noexcept
   {
     return L"ICountedType";
   }
 
-  inline static std::wstring ToString(UnknownType * q) noexcept
+  inline static std::wstring ToString(UnknownType * /*q*/) noexcept
   {
     return L"UnknownType";
   }
 
-  inline static std::wstring ToString(IUnknownType * q) noexcept
+  inline static std::wstring ToString(IUnknownType * /*q*/) noexcept
   {
     return L"IUnknownType";
   }

diff --git a/libs/object/tests/objectRefCountTest.cpp b/libs/object/tests/objectRefCountTest.cpp
@@ -7,6 +7,7 @@ Unit tests for classes in the ObjectRefCount.h
 
 #include "precomp.h"
 //#include <core/memoryNew_Throw.h> // It must be here to define operator new which helps memory leak detection.
+#include <compilerAdapters/compilerWarnings.h>
 #include <object/refCountedObject.h>
 #include <test/skipSEHUT.h>
 #include "testAllocators.h"
@@ -284,6 +285,7 @@ TEST_CLASS (ObjectRefCountTest)
     TestAssert::IsTrue(obj.IsEmpty());
   }
 
+BEGIN_DISABLE_WARNING_UNREACHABLE_CODE()
   TEST_METHOD(ObjectRefCount_Make_CtorThrows)
   {
     Mso::TCntPtr<ObjectRefCountSample4Throw> obj;
@@ -305,6 +307,7 @@ TEST_CLASS (ObjectRefCountTest)
     TestAssert::IsTrue(deleted); // If InitializeThis throws then destructor must be called.
     TestAssert::IsTrue(obj.IsEmpty());
   }
+END_DISABLE_WARNING_UNREACHABLE_CODE()
 
   TEST_METHOD(ObjectRefCount_MakeElseNull)
   {
@@ -410,6 +413,7 @@ TEST_CLASS (ObjectRefCountTest)
     TestAssert::IsTrue(obj.IsEmpty());
   }
 
+BEGIN_DISABLE_WARNING_UNREACHABLE_CODE()
   TEST_METHOD(ObjectRefCount_MakeAlloc_CtorThrows)
   {
     Mso::TCntPtr<ObjectRefCountSample41Throw> obj;
@@ -435,6 +439,7 @@ TEST_CLASS (ObjectRefCountTest)
     AssertAllocState(state); // If InitializeThis throws then destructor must be called.
     TestAssert::IsTrue(obj.IsEmpty());
   }
+END_DISABLE_WARNING_UNREACHABLE_CODE()
 
   TEST_METHOD(ObjectRefCount_MakeAllocElseNull)
   {