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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054
+    }
   }
 
----------------
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. :(


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

https://reviews.llvm.org/D61046