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