[libcxx-commits] [PATCH] D144252: [runtimes] Synchronize warnings flags between libc++/libc++abi/libunwind

Thu Mar 9 08:24:45 PST 2023

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Can you record in the commit message what the changes to the warnings are? It seems like the libcxxabi and libunwind warning flags were the same, so those are synced to the libcxx warnings. However, that synchronization removes `-Wconversion` from the libcxxabi and libunwind flags (the TODO you added). Is that all?

Thanks for the refactoring by the way, this is a great way to get where we needed to be while leaving the code better behind you.

================
Comment at: libcxx/CMakeLists.txt:843
   cxx_add_basic_build_flags(${target})
-  cxx_add_warning_flags(${target})
+  cxx_add_warning_flags(${target} ${LIBCXX_ENABLE_WERROR} ${LIBCXX_ENABLE_PEDANTIC})
   cxx_add_windows_flags(${target})
----------------
Here you use `cxx_add_warning_flags` but you have not included `WarningFlags.cmake`. I think that's your Windows CI issue. IDK why it works on other platforms, that's almost concerning.

================
Comment at: runtimes/cmake/Modules/WarningFlags.cmake:69-72
+  else()
+    # TODO(EricWF) Remove this. We shouldn't be suppressing errors when -Werror is
+    # added elsewhere.
+    target_add_compile_flags_if_supported(${target} PRIVATE -Wno-error)
----------------
FWIW I strongly suspect this is not needed anymore. Let's try to get rid of it in a different patch.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144252/new/

https://reviews.llvm.org/D144252