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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/trunk/unittests/AST/ASTImporterTest.cpp:4054
+    }
   }
 
----------------
aganea wrote:
> 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)
On reflection, I'd rather just add the braces and skip the build system complexity. It's what we've done in the past anyway, see rL305507 etc. I'm just annoyed at the lack of upstream gtest maintenance.


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

https://reviews.llvm.org/D61046