[PATCH] D61046: Fix compilation warnings when compiling with GCC 7.3

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 3 inline comments as done.
aganea added inline comments.


================
Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054
+    }
   }
 
----------------
rnk wrote:
> nikic wrote:
> > shafik wrote:
> > > aganea wrote:
> > > > Fixes
> > > > ```
> > > > [2097/2979] Building CXX object tools/clang/unittests/Tooling/CMakeFiles/ToolingTests.dir/LookupTest.cpp.o
> > > > /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp: In lambda function:
> > > > /mnt/f/svn/clang/unittests/Tooling/LookupTest.cpp:220:8: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
> > > >      if (Type.getDecl()->getQualifiedNameAsString() == "x::y::Old")
> > > >         ^
> > > > ```
> > > You mixed up the error messages but I see what is going on.
> > > 
> > > So you may want to add a comment since it is not apparent that what is going on is due the `EXPECT_TRUE` macro eventually expanding to an `if..else` which is what is triggering the warning. Since someone may come by in the future and just remove the braces since it is not apparent why they are there.
> > > 
> > > Same below as well.
> > EXPECT_* inside if is quite common, I don't think we should add a comment every time it is used.
> This is a longstanding issue. gtest even includes this beautiful snippet of code:
> 
> ```
> // The GNU compiler emits a warning if nested "if" statements are followed by
> // an "else" statement and braces are not used to explicitly disambiguate the
> // "else" binding.  This leads to problems with code like:
> //
> //   if (gate)
> //     ASSERT_*(condition) << "Some message";
> //
> // The "switch (0) case 0:" idiom is used to suppress this.
> #ifdef __INTEL_COMPILER
> # define GTEST_AMBIGUOUS_ELSE_BLOCKER_
> #else
> # define GTEST_AMBIGUOUS_ELSE_BLOCKER_ switch (0) case 0: default:  // NOLINT
> #endif
> ```
> https://github.com/llvm/llvm-project/blob/a974b33a10745b528c34f0accbd230b0a4e1fb87/llvm/utils/unittest/googletest/include/gtest/internal/gtest-port.h#L834
> 
> The expansion looks something like this:
> ```
> if (user_cond)
>   switch (0) case 0: default:
>     if (const TestResult res = ...);
>     else fail(res, ...) << "User streaming can follow"
> ```
> 
> It seems new versions of GCC have gotten "smarter" and this pattern no longer suppresses the warning as desired. It might be worth disabling -Wdangling-else for GCC at this point, since this will be an ongoing problem because LLVM typically elides braces when possible.
> 
> Oh, we even reported it back in 2017:
> https://github.com/google/googletest/issues/1119
> ... and it was closed as inactive. Woohoo. :(
So revert those braces and disable -Wdangling-else if CMAKE_COMPILER_IS_GNUCXX ? (somewhere in HandleLLVMOptions.cmake)


================
Comment at: llvm/trunk/unittests/Transforms/Scalar/CMakeLists.txt:14-17
+# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
+if (CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 6.0)
+  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
+endif()
----------------
tstellar wrote:
> aganea wrote:
> > tstellar wrote:
> > > Same thing here too.
> > Not sure I understand, there're plenty of compiler-specific examples in the .cmake files. Is there an alternate way to fix this?
> I know there are some, but I don't think a warning like this is important enough to fix with a compiler specific work-around.
You mean more like
```
# Workaround for the gcc 6.1 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80916.
if (NOT MSVC)
  set_source_files_properties(LoopPassManagerTest.cpp PROPERTIES COMPILE_FLAGS -Wno-unused-function)
endif()
```
Or leave the warning? There's already an #ifdef below in LoopPassManagerTest.cpp but it's not at the right place, this simply corrects it.


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

https://reviews.llvm.org/D61046