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