TraceLogging.h preprocessor conformance (#448)

* Preprocessor conformance for MSVC * Clang conformance fixes * Clang still has problems with coroutines, it seems * Formatting * Don't specify llvm version * Fix __stdcall issues with Clang when MS compat is off
microsoft · Jul 1, 2024 · e60e004 · e60e004
1 parent 68ab8c1
commit e60e004
Show file tree

Hide file tree

Showing 9 changed files with 249 additions and 212 deletions.
diff --git a/cmake/common_build_flags.cmake b/cmake/common_build_flags.cmake
@@ -45,16 +45,6 @@ if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
 
  # We don't want legacy MSVC conformance
  -fno-delayed-template-parsing
-
- # NOTE: Windows headers not clean enough for us to realistically attempt to start fixing these errors yet. That
- # said, errors that originate from WIL headers may benefit
- # -fno-ms-compatibility
- # -ferror-limit=999
- # -fmacro-backtrace-limit=0
-
- # -fno-ms-compatibility turns off preprocessor compatability, which currently only works when __VA_OPT__ support
- # is available (i.e. >= C++20)
- # -Xclang -std=c++2a
  )
 else()
  add_compile_options(
@@ -64,12 +54,6 @@ else()
  # wistd::function has padding due to alignment. This is expected
  /wd4324
 
- # TODO: https://github.com/Microsoft/wil/issues/6
- # /experimental:preprocessor
-
- # CRT headers are not yet /experimental:preprocessor clean, so work around the known issues
- # /Wv:18
-
  # Some tests have a LOT of template instantiations
  /bigobj
 

diff --git a/include/wil/Tracelogging.h b/include/wil/Tracelogging.h
diff --git a/include/wil/windowing.h b/include/wil/windowing.h
@@ -24,19 +24,64 @@ namespace details
  {
  };
 
+ template <typename TCallback>
+ BOOL __stdcall EnumWindowsCallbackNoThrow(HWND hwnd, LPARAM lParam)
+ {
+ auto pCallback = reinterpret_cast<TCallback*>(lParam);
+#ifdef __cpp_if_constexpr
+ using result_t = decltype((*pCallback)(hwnd));
+ if constexpr (wistd::is_void_v<result_t>)
+ {
+ (*pCallback)(hwnd);
+ return TRUE;
+ }
+ else if constexpr (wistd::is_same_v<result_t, HRESULT>)
+ {
+ // NB: this works for both HRESULT and NTSTATUS as both S_OK and ERROR_SUCCESS are 0
+ return (S_OK == (*pCallback)(hwnd)) ? TRUE : FALSE;
+ }
+ else if constexpr (std::is_same_v<result_t, bool>)
+ {
+ return (*pCallback)(hwnd) ? TRUE : FALSE;
+ }
+ else
+ {
+ static_assert(details::always_false<TCallback>::value, "Callback must return void, bool, or HRESULT");
+ }
+#else
+ return (*pCallback)(hwnd);
+#endif
+ }
+
  template <typename TEnumApi, typename TCallback>
  void DoEnumWindowsNoThrow(TEnumApi&& enumApi, TCallback&& callback) noexcept
  {
- auto enumproc = [](HWND hwnd, LPARAM lParam) -> BOOL {
- auto pCallback = reinterpret_cast<TCallback*>(lParam);
+ enumApi(EnumWindowsCallbackNoThrow<TCallback>, reinterpret_cast<LPARAM>(&callback));
+ }
+
+#ifdef WIL_ENABLE_EXCEPTIONS
+ template <typename TCallback>
+ struct EnumWindowsCallbackData
+ {
+ std::exception_ptr exception;
+ TCallback* pCallback;
+ };
+
+ template <typename TCallback>
+ BOOL __stdcall EnumWindowsCallback(HWND hwnd, LPARAM lParam)
+ {
+ auto pCallbackData = reinterpret_cast<EnumWindowsCallbackData<TCallback>*>(lParam);
+ try
+ {
+ auto pCallback = pCallbackData->pCallback;
 #ifdef __cpp_if_constexpr
  using result_t = decltype((*pCallback)(hwnd));
- if constexpr (wistd::is_void_v<result_t>)
+ if constexpr (std::is_void_v<result_t>)
  {
  (*pCallback)(hwnd);
  return TRUE;
  }
- else if constexpr (wistd::is_same_v<result_t, HRESULT>)
+ else if constexpr (std::is_same_v<result_t, HRESULT>)
  {
  // NB: this works for both HRESULT and NTSTATUS as both S_OK and ERROR_SUCCESS are 0
  return (S_OK == (*pCallback)(hwnd)) ? TRUE : FALSE;
@@ -52,55 +97,19 @@ namespace details
 #else
  return (*pCallback)(hwnd);
 #endif
- };
- enumApi(enumproc, reinterpret_cast<LPARAM>(&callback));
- }
+ }
+ catch (...)
+ {
+ pCallbackData->exception = std::current_exception();
+ return FALSE;
+ }
+ };
 
-#ifdef WIL_ENABLE_EXCEPTIONS
  template <typename TEnumApi, typename TCallback>
  void DoEnumWindows(TEnumApi&& enumApi, TCallback&& callback)
  {
- struct
- {
- std::exception_ptr exception;
- TCallback* pCallback;
- } callbackData = {nullptr, &callback};
- auto enumproc = [](HWND hwnd, LPARAM lParam) -> BOOL {
- auto pCallbackData = reinterpret_cast<decltype(&callbackData)>(lParam);
- try
- {
- auto pCallback = pCallbackData->pCallback;
-#ifdef __cpp_if_constexpr
- using result_t = decltype((*pCallback)(hwnd));
- if constexpr (std::is_void_v<result_t>)
- {
- (*pCallback)(hwnd);
- return TRUE;
- }
- else if constexpr (std::is_same_v<result_t, HRESULT>)
- {
- // NB: this works for both HRESULT and NTSTATUS as both S_OK and ERROR_SUCCESS are 0
- return (S_OK == (*pCallback)(hwnd)) ? TRUE : FALSE;
- }
- else if constexpr (std::is_same_v<result_t, bool>)
- {
- return (*pCallback)(hwnd) ? TRUE : FALSE;
- }
- else
- {
- static_assert(details::always_false<TCallback>::value, "Callback must return void, bool, or HRESULT");
- }
-#else
- return (*pCallback)(hwnd);
-#endif
- }
- catch (...)
- {
- pCallbackData->exception = std::current_exception();
- return FALSE;
- }
- };
- enumApi(enumproc, reinterpret_cast<LPARAM>(&callbackData));
+ EnumWindowsCallbackData<TCallback> callbackData = {nullptr, &callback};
+ enumApi(EnumWindowsCallback<TCallback>, reinterpret_cast<LPARAM>(&callbackData));
  if (callbackData.exception)
  {
  std::rethrow_exception(callbackData.exception);

diff --git a/scripts/azure-pipelines.yml b/scripts/azure-pipelines.yml
@@ -58,7 +58,7 @@ jobs:
 
  steps:
  - script: |
- choco upgrade llvm -y --version=17.0.6
+ choco upgrade llvm -y
  if %ERRORLEVEL% NEQ 0 goto :eof
  echo ##vso[task.setvariable variable=PATH]%PATH%;C:\Program Files\LLVM\bin
  displayName: 'Install Clang'

diff --git a/tests/ResourceTests.cpp b/tests/ResourceTests.cpp
@@ -767,12 +767,25 @@ TEST_CASE("UniqueEnvironmentStrings", "[resource][win32]")
  }
 }
 
+#if (__STDC__ && !defined(_FORCENAMELESSUNION)) || defined(NONAMELESSUNION) || \
+ (!defined(_MSC_EXTENSIONS) && !defined(_FORCENAMELESSUNION))
+#define VAR_ACCESS_1 .n1
+#define VAR_ACCESS_2 .n1.n2
+#define VAR_ACCESS_3 .n1.n2.n3
+#define VAR_ACCESS_4 .n1.n2.n3.brecVal
+#else
+#define VAR_ACCESS_1
+#define VAR_ACCESS_2
+#define VAR_ACCESS_3
+#define VAR_ACCESS_4
+#endif
+
 TEST_CASE("UniqueVariant", "[resource][com]")
 {
  wil::unique_variant var;
- var.vt = VT_BSTR;
- var.bstrVal = ::SysAllocString(L"25");
- REQUIRE(var.bstrVal != nullptr);
+ var VAR_ACCESS_2.vt = VT_BSTR;
+ var VAR_ACCESS_3.bstrVal = ::SysAllocString(L"25");
+ REQUIRE(var VAR_ACCESS_3.bstrVal != nullptr);
 
  auto call = [](const VARIANT&) {};
  call(var);
@@ -782,8 +795,8 @@ TEST_CASE("UniqueVariant", "[resource][com]")
 
  wil::unique_variant var2;
  REQUIRE_SUCCEEDED(VariantChangeType(&var2, &var, 0, VT_UI4));
- REQUIRE(var2.vt == VT_UI4);
- REQUIRE(var2.uiVal == 25);
+ REQUIRE(var2 VAR_ACCESS_2.vt == VT_UI4);
+ REQUIRE(var2 VAR_ACCESS_3.uiVal == 25);
 }
 
 TEST_CASE("DefaultTemplateParamCompiles", "[resource]")

diff --git a/tests/ResultTests.cpp b/tests/ResultTests.cpp
@@ -733,11 +733,14 @@ TEST_CASE("ResultTests::OriginatedWithMessagePreserved", "[result]")
 
 #endif
 
+static void __stdcall CustomLoggingCallback(const wil::FailureInfo&) noexcept
+{
+ ::SetLastError(ERROR_ABANDON_HIBERFILE);
+}
+
 TEST_CASE("ResultTests::ReportDoesNotChangeLastError", "[result]")
 {
- decltype(wil::details::g_pfnLoggingCallback) oopsie = [](wil::FailureInfo const&) noexcept {
- ::SetLastError(ERROR_ABANDON_HIBERFILE);
- };
+ decltype(wil::details::g_pfnLoggingCallback) oopsie = CustomLoggingCallback;
  auto swap = witest::AssignTemporaryValue(&wil::details::g_pfnLoggingCallback, oopsie);
 
  ::SetLastError(ERROR_ABIOS_ERROR);

diff --git a/tests/WindowingTests.cpp b/tests/WindowingTests.cpp
@@ -156,12 +156,25 @@ TEST_CASE("EnumChildWindows", "[windowing]")
  wil::for_each_window_nothrow([&parent](HWND hwnd) {
  if (IsWindow(hwnd) && IsWindowVisible(hwnd))
  {
- parent = hwnd;
- return false;
+ // Make sure we choose a window that has children
+ bool hasChildren = false;
+ wil::for_each_child_window_nothrow(hwnd, [&](HWND) {
+ hasChildren = true;
+ return false;
+ });
+
+ if (hasChildren)
+ {
+ parent = hwnd;
+ return false;
+ }
  }
  return true;
  });
 
+ // Avoid confusing issues below
+ REQUIRE(parent != nullptr);
+
  // nothrow version
  {
  wil::for_each_child_window_nothrow(parent, [](HWND hwnd) {

diff --git a/tests/cpplatest/CMakeLists.txt b/tests/cpplatest/CMakeLists.txt
@@ -8,12 +8,19 @@ target_precompile_headers(witest.cpplatest PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/.
 target_compile_features(witest.cpplatest PRIVATE cxx_std_20)
 
 if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
- # Clang is not compatible with the experimental coroutine header, so temporarily disable some headers until full
- # C++20 support is available
+ # Clang seems to have issues with coroutines and exceptions, so disable some tests for now
  set(COROUTINE_SOURCES)
+
+ target_compile_options(witest.cpplatest PRIVATE
+ -fno-ms-compatibility
+ )
 else()
  set(COROUTINE_SOURCES
  ${CMAKE_CURRENT_SOURCE_DIR}/../ComApartmentVariableTests.cpp)
+
+ target_compile_options(witest.cpplatest PRIVATE
+ /Zc:preprocessor
+ )
 endif()
 
 target_sources(witest.cpplatest PRIVATE

diff --git a/tests/pch.h b/tests/pch.h
@@ -4,3 +4,6 @@
 #define RESULT_NORETURN
 #define RESULT_NORETURN_NULL
 #define RESULT_NORETURN_RESULT(expr) return (expr)
+
+// Definition using '__declspec(selectany)' not compatible with -fno-ms-compatibility
+#define XMGLOBALCONST inline constexpr const