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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
