Page MenuHomePhabricator

[RFC] [OpenMP] Turn on -Wall compiler warnings by default
ClosedPublic

Authored by Hahnfeld on Wed, Aug 7, 6:38 AM.

Details

Summary

Instead, maintain a list of disabled options to still build libomp and
libomptarget without warnings. This includes -Wno-error and -Wno-pedantic
to silence warnings that LLVM enables when building in-tree.

I tested the following compilers:

  • Clang 6.0, 7.0, 8.0
  • GCC 4.8.5 (CentOS 7), GCC 6, 7, 8, 9
  • Intel Compiler 16, 17, 18, 19

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Wed, Aug 7, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Aug 7, 6:38 AM

I like this a lot. Thanks

Thanks for this cleanup! Just one comment.

openmp/cmake/config-ix.cmake
4–15 ↗(On Diff #213863)

Should everything be check_cxx_compiler_flag()?

Hahnfeld marked an inline comment as done.Mon, Aug 12, 11:52 AM
Hahnfeld added inline comments.
openmp/cmake/config-ix.cmake
4–15 ↗(On Diff #213863)

This was also my first feeling, but my second thought was that we might have C files again in the future. For example, some very simple tools with OMPT might not need C++ and we'll want all warnings for them. However, that's a weak argument for now without a real use and I can change it to check_cxx_compiler_flag if you think that's more consistent.

jlpeyton added inline comments.Tue, Aug 13, 9:00 AM
openmp/cmake/config-ix.cmake
4–15 ↗(On Diff #213863)

After some thought, I think we should use check_cxx_compiler_flag() and here is my reasoning:
The trend with the openmp project has been turning C code/files/anything into C++, sometimes nominally, but C++ nonetheless. I think it is reasonable to assume language agnostic options (e.g., -Wall) are supported by both the C and corresponding C++ compiler. This assumption is seen in the append_if(HAVE_FLAG "-Wflag" C_FLAGS CXX_FLAGS) code in HandleOpenMPOptions.cmake. So any future C file should be able to use the flags without issue.

Hahnfeld marked an inline comment as done.Tue, Aug 13, 9:08 AM
Hahnfeld added inline comments.
openmp/cmake/config-ix.cmake
4–15 ↗(On Diff #213863)

Ok, so just to be sure: You propose to check with check_cxx_compiler_flag and still add the flag to CMAKE_C_FLAGS? I guess that sounds reasonable.

jlpeyton added inline comments.Tue, Aug 13, 10:47 AM
openmp/cmake/config-ix.cmake
4–15 ↗(On Diff #213863)

Yes.

Hahnfeld updated this revision to Diff 214881.Tue, Aug 13, 10:59 AM

Change check_c_compiler_flag() to check_cxx_compiler_flag().

Hahnfeld marked 3 inline comments as done.Tue, Aug 13, 10:59 AM
This revision is now accepted and ready to land.Tue, Aug 13, 11:37 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 15, 6:12 AM