[NFC] [CMake] Add -Wno-dangling-else for GCC built unittests #112817

jsji · 2024-10-18T03:13:35Z

This is one of the many PRs to fix errors with LLVM_ENABLE_WERROR=on. Built by GCC 11.

Fix warnings:

llvm/unittests/ProfileData/CoverageMappingTest.cpp: In member function ‘virtual void {anonymous}::CoverageMappingTest_TVIdxBuilder_Test::TestBody()’:
llvm/unittests/ProfileData/CoverageMappingTest.cpp:1116:10: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]
1116 | if (Node.NextIDs[C] < 0)

The problem here is because these macros, eg: EXPECT_TRUE are expanded to a single line multi-statement code with if/else, which is indeed ambiguous after pre-processing. a simple example would be like: https://godbolt.org/z/4zjn56qrP

if(x)
 switch (0) case 0: default: if (...) ; else return;;

Given that omit braces in such cases is part of LLVM's style guide, and it is hard to add braces in gtest just for GCC's warning, add -Wno-dangling-else for GCC instead.

llvmbot · 2024-10-18T03:14:07Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-flang-openmp

Author: Jinsong Ji (jsji)

Changes

This is one of the many PRs to fix errors with LLVM_ENABLE_WERROR=on. Built by GCC 11.

Fix warnings:

llvm/unittests/ProfileData/CoverageMappingTest.cpp: In member function ‘virtual void {anonymous}::CoverageMappingTest_TVIdxBuilder_Test::TestBody()’:
llvm/unittests/ProfileData/CoverageMappingTest.cpp:1116:10: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]
1116 | if (Node.NextIDs[C] < 0)

Full diff: https://github.com/llvm/llvm-project/pull/112817.diff

5 Files Affected:

(modified) llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp (+2-1)
(modified) llvm/unittests/IR/AttributesTest.cpp (+2-2)
(modified) llvm/unittests/IR/PatternMatch.cpp (+8-4)
(modified) llvm/unittests/Object/ELFObjectFileTest.cpp (+2-1)
(modified) llvm/unittests/ProfileData/CoverageMappingTest.cpp (+2-1)

diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index fe04cbbce12dcd..6b6ee1a3f2cbcf 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -2936,8 +2936,9 @@ TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
   const DataLayout &DL = M->getDataLayout();
   const llvm::Align TypeAlign = DL.getABITypeAlign(CriticalNamePtrTy);
   const llvm::Align PtrAlign = DL.getPointerABIAlignment(GV->getAddressSpace());
-  if (const llvm::MaybeAlign Alignment = GV->getAlign())
+  if (const llvm::MaybeAlign Alignment = GV->getAlign()) {
     EXPECT_EQ(*Alignment, std::max(TypeAlign, PtrAlign));
+  }
 }
 
 TEST_F(OpenMPIRBuilderTest, OrderedDirectiveDependSource) {
diff --git a/llvm/unittests/IR/AttributesTest.cpp b/llvm/unittests/IR/AttributesTest.cpp
index f73f2b20e9fea5..97aaf42570d1fe 100644
--- a/llvm/unittests/IR/AttributesTest.cpp
+++ b/llvm/unittests/IR/AttributesTest.cpp
@@ -468,9 +468,9 @@ TEST(Attributes, SetIntersect) {
     AS0 = AttributeSet::get(C0, AB0);
     Res = AS0.intersectWith(C0, AS1);
     ASSERT_EQ(Res.has_value(), CanDrop);
-    if (CanDrop)
+    if (CanDrop) {
       ASSERT_FALSE(Res->hasAttributes());
-
+    }
     AS1 = AttributeSet::get(C1, AB0);
     Res = AS0.intersectWith(C0, AS1);
     ASSERT_TRUE(Res.has_value());
diff --git a/llvm/unittests/IR/PatternMatch.cpp b/llvm/unittests/IR/PatternMatch.cpp
index 367ba6ab52a596..e6a08fd713bdda 100644
--- a/llvm/unittests/IR/PatternMatch.cpp
+++ b/llvm/unittests/IR/PatternMatch.cpp
@@ -637,20 +637,23 @@ TEST_F(PatternMatchTest, CheckedInt) {
     CRes = nullptr;
     EXPECT_EQ(CheckUgt1(APVal), m_CheckedInt(CheckUgt1).match(C));
     EXPECT_EQ(CheckUgt1(APVal), m_CheckedInt(CRes, CheckUgt1).match(C));
-    if (CheckUgt1(APVal))
+    if (CheckUgt1(APVal)) {
       EXPECT_EQ(CRes, C);
+    }
 
     CRes = nullptr;
     EXPECT_EQ(CheckNonZero(APVal), m_CheckedInt(CheckNonZero).match(C));
     EXPECT_EQ(CheckNonZero(APVal), m_CheckedInt(CRes, CheckNonZero).match(C));
-    if (CheckNonZero(APVal))
+    if (CheckNonZero(APVal)) {
       EXPECT_EQ(CRes, C);
+    }
 
     CRes = nullptr;
     EXPECT_EQ(CheckPow2(APVal), m_CheckedInt(CheckPow2).match(C));
     EXPECT_EQ(CheckPow2(APVal), m_CheckedInt(CRes, CheckPow2).match(C));
-    if (CheckPow2(APVal))
+    if (CheckPow2(APVal)) {
       EXPECT_EQ(CRes, C);
+    }
 
   };
 
@@ -710,8 +713,9 @@ TEST_F(PatternMatchTest, CheckedInt) {
     EXPECT_EQ(Expec, m_CheckedInt(CRes, CheckFn).match(C));
     if (Expec) {
       EXPECT_NE(CRes, nullptr);
-      if (AllSame)
+      if (AllSame) {
         EXPECT_EQ(CRes, C);
+      }
     }
   };
   auto DoVecCheck = [&](ArrayRef<std::optional<int8_t>> Vals) {
diff --git a/llvm/unittests/Object/ELFObjectFileTest.cpp b/llvm/unittests/Object/ELFObjectFileTest.cpp
index c13dc0e3fab898..153ac560179985 100644
--- a/llvm/unittests/Object/ELFObjectFileTest.cpp
+++ b/llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -1244,8 +1244,9 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
           }
           EXPECT_EQ(PGOAnalyses, *ExpectedPGO);
           for (auto &&[BB, PGO] : llvm::zip(*BBAddrMaps, PGOAnalyses)) {
-            if (PGO.FeatEnable.BBFreq || PGO.FeatEnable.BrProb)
+            if (PGO.FeatEnable.BBFreq || PGO.FeatEnable.BrProb) {
               EXPECT_EQ(BB.getNumBBEntries(), PGO.BBEntries.size());
+            }
           }
         }
       };
diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
index ef147674591c51..288bf8e254d498 100644
--- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -1113,8 +1113,9 @@ TEST(CoverageMappingTest, TVIdxBuilder) {
     EXPECT_EQ(Node.Width, IndicesRefs[I].Width);
     for (int C = 0; C < 2; ++C) {
       auto Index = TheBuilder.Indices[I][C];
-      if (Node.NextIDs[C] < 0)
+      if (Node.NextIDs[C] < 0) {
         EXPECT_TRUE(Decisions.insert({Index, Node.Width}).second);
+      }
     }
 #endif
   }

llvmbot · 2024-10-18T03:14:08Z

@llvm/pr-subscribers-pgo

Author: Jinsong Ji (jsji)

Changes

This is one of the many PRs to fix errors with LLVM_ENABLE_WERROR=on. Built by GCC 11.

Fix warnings:

llvm/unittests/ProfileData/CoverageMappingTest.cpp: In member function ‘virtual void {anonymous}::CoverageMappingTest_TVIdxBuilder_Test::TestBody()’:
llvm/unittests/ProfileData/CoverageMappingTest.cpp:1116:10: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else]
1116 | if (Node.NextIDs[C] < 0)

Full diff: https://github.com/llvm/llvm-project/pull/112817.diff

5 Files Affected:

(modified) llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp (+2-1)
(modified) llvm/unittests/IR/AttributesTest.cpp (+2-2)
(modified) llvm/unittests/IR/PatternMatch.cpp (+8-4)
(modified) llvm/unittests/Object/ELFObjectFileTest.cpp (+2-1)
(modified) llvm/unittests/ProfileData/CoverageMappingTest.cpp (+2-1)

diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index fe04cbbce12dcd..6b6ee1a3f2cbcf 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -2936,8 +2936,9 @@ TEST_F(OpenMPIRBuilderTest, CriticalDirective) {
   const DataLayout &DL = M->getDataLayout();
   const llvm::Align TypeAlign = DL.getABITypeAlign(CriticalNamePtrTy);
   const llvm::Align PtrAlign = DL.getPointerABIAlignment(GV->getAddressSpace());
-  if (const llvm::MaybeAlign Alignment = GV->getAlign())
+  if (const llvm::MaybeAlign Alignment = GV->getAlign()) {
     EXPECT_EQ(*Alignment, std::max(TypeAlign, PtrAlign));
+  }
 }
 
 TEST_F(OpenMPIRBuilderTest, OrderedDirectiveDependSource) {
diff --git a/llvm/unittests/IR/AttributesTest.cpp b/llvm/unittests/IR/AttributesTest.cpp
index f73f2b20e9fea5..97aaf42570d1fe 100644
--- a/llvm/unittests/IR/AttributesTest.cpp
+++ b/llvm/unittests/IR/AttributesTest.cpp
@@ -468,9 +468,9 @@ TEST(Attributes, SetIntersect) {
     AS0 = AttributeSet::get(C0, AB0);
     Res = AS0.intersectWith(C0, AS1);
     ASSERT_EQ(Res.has_value(), CanDrop);
-    if (CanDrop)
+    if (CanDrop) {
       ASSERT_FALSE(Res->hasAttributes());
-
+    }
     AS1 = AttributeSet::get(C1, AB0);
     Res = AS0.intersectWith(C0, AS1);
     ASSERT_TRUE(Res.has_value());
diff --git a/llvm/unittests/IR/PatternMatch.cpp b/llvm/unittests/IR/PatternMatch.cpp
index 367ba6ab52a596..e6a08fd713bdda 100644
--- a/llvm/unittests/IR/PatternMatch.cpp
+++ b/llvm/unittests/IR/PatternMatch.cpp
@@ -637,20 +637,23 @@ TEST_F(PatternMatchTest, CheckedInt) {
     CRes = nullptr;
     EXPECT_EQ(CheckUgt1(APVal), m_CheckedInt(CheckUgt1).match(C));
     EXPECT_EQ(CheckUgt1(APVal), m_CheckedInt(CRes, CheckUgt1).match(C));
-    if (CheckUgt1(APVal))
+    if (CheckUgt1(APVal)) {
       EXPECT_EQ(CRes, C);
+    }
 
     CRes = nullptr;
     EXPECT_EQ(CheckNonZero(APVal), m_CheckedInt(CheckNonZero).match(C));
     EXPECT_EQ(CheckNonZero(APVal), m_CheckedInt(CRes, CheckNonZero).match(C));
-    if (CheckNonZero(APVal))
+    if (CheckNonZero(APVal)) {
       EXPECT_EQ(CRes, C);
+    }
 
     CRes = nullptr;
     EXPECT_EQ(CheckPow2(APVal), m_CheckedInt(CheckPow2).match(C));
     EXPECT_EQ(CheckPow2(APVal), m_CheckedInt(CRes, CheckPow2).match(C));
-    if (CheckPow2(APVal))
+    if (CheckPow2(APVal)) {
       EXPECT_EQ(CRes, C);
+    }
 
   };
 
@@ -710,8 +713,9 @@ TEST_F(PatternMatchTest, CheckedInt) {
     EXPECT_EQ(Expec, m_CheckedInt(CRes, CheckFn).match(C));
     if (Expec) {
       EXPECT_NE(CRes, nullptr);
-      if (AllSame)
+      if (AllSame) {
         EXPECT_EQ(CRes, C);
+      }
     }
   };
   auto DoVecCheck = [&](ArrayRef<std::optional<int8_t>> Vals) {
diff --git a/llvm/unittests/Object/ELFObjectFileTest.cpp b/llvm/unittests/Object/ELFObjectFileTest.cpp
index c13dc0e3fab898..153ac560179985 100644
--- a/llvm/unittests/Object/ELFObjectFileTest.cpp
+++ b/llvm/unittests/Object/ELFObjectFileTest.cpp
@@ -1244,8 +1244,9 @@ TEST(ELFObjectFileTest, ReadPGOAnalysisMap) {
           }
           EXPECT_EQ(PGOAnalyses, *ExpectedPGO);
           for (auto &&[BB, PGO] : llvm::zip(*BBAddrMaps, PGOAnalyses)) {
-            if (PGO.FeatEnable.BBFreq || PGO.FeatEnable.BrProb)
+            if (PGO.FeatEnable.BBFreq || PGO.FeatEnable.BrProb) {
               EXPECT_EQ(BB.getNumBBEntries(), PGO.BBEntries.size());
+            }
           }
         }
       };
diff --git a/llvm/unittests/ProfileData/CoverageMappingTest.cpp b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
index ef147674591c51..288bf8e254d498 100644
--- a/llvm/unittests/ProfileData/CoverageMappingTest.cpp
+++ b/llvm/unittests/ProfileData/CoverageMappingTest.cpp
@@ -1113,8 +1113,9 @@ TEST(CoverageMappingTest, TVIdxBuilder) {
     EXPECT_EQ(Node.Width, IndicesRefs[I].Width);
     for (int C = 0; C < 2; ++C) {
       auto Index = TheBuilder.Indices[I][C];
-      if (Node.NextIDs[C] < 0)
+      if (Node.NextIDs[C] < 0) {
         EXPECT_TRUE(Decisions.insert({Index, Node.Width}).second);
+      }
     }
 #endif
   }

nikic

Given that omit braces in such cases is part of LLVM's style guide, I wonder whether it may be better to use -Wno-dangling-else when compiling gtest based code using GCC?

As Clang doesn't have this false positive, this warning will creep back in over time.

teresajohnson · 2024-10-18T13:01:22Z

Given that omit braces in such cases is part of LLVM's style guide

Reference: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

I wonder whether it may be better to use -Wno-dangling-else when compiling gtest based code using GCC?

As Clang doesn't have this false positive, this warning will creep back in over time.

Agree. But where is the else in this case? I assume it is inside the gtest macros being invoked in the if statement bodies - might be better to add fixes there?

This is one of the many PRs to fix errors with LLVM_ENABLE_WERROR=on. Built by GCC 11. Fix warnings: llvm/unittests/ProfileData/CoverageMappingTest.cpp: In member function ‘virtual void {anonymous}::CoverageMappingTest_TVIdxBuilder_Test::TestBody()’: llvm/unittests/ProfileData/CoverageMappingTest.cpp:1116:10: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] 1116 | if (Node.NextIDs[C] < 0)

This reverts commit ea33083.

jsji · 2024-10-21T16:38:55Z

But where is the else in this case? I assume it is inside the gtest macros being invoked in the if statement bodies - might be better to add fixes there?

I don't think macros inside gest will be become else of the if statement.

jsji · 2024-10-21T16:59:00Z

Given that omit braces in such cases is part of LLVM's style guide

Reference: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

I wonder whether it may be better to use -Wno-dangling-else when compiling gtest based code using GCC?
As Clang doesn't have this false positive, this warning will creep back in over time.

Agree. But where is the else in this case? I assume it is inside the gtest macros being invoked in the if statement bodies - might be better to add fixes there?

The problem here is because these macros, eg: EXPECT_TRUE are expanded to a single line multi-statement code with if/else, which is indeed ambiguous after pre-processing. a simple example would be like: https://godbolt.org/z/4zjn56qrP

    if(x)
     switch (0) case 0: default: if (x > 3) ; else return;;

I don't think it is realistic to change gtest code for such warnings. So, either disable the warnings, or add braces.

jsji · 2024-10-22T21:41:29Z

@nikic @teresajohnson Any further comments? Thanks.

teresajohnson

lgtm but please update the title and description to reflect the new approach, and I suggested a more verbose comment below.

teresajohnson · 2024-10-26T14:48:53Z

llvm/unittests/CMakeLists.txt

@@ -14,6 +14,11 @@ function(add_llvm_target_unittest test_dir_name)
  add_llvm_unittest(${test_dir_name} DISABLE_LLVM_LINK_LLVM_DYLIB ${ARGN})
 endfunction()

+# GCC may emit false positive warnings against LLVM's style guide.


Can you expand the comment to note that this is because of the gtest macro expansion.

Sure. Done. Thanks @teresajohnson !

jh7370

Please address the typos I've highlighted in another commit.

jh7370 · 2024-10-28T08:32:04Z

llvm/unittests/CMakeLists.txt

+#   if (...)
+#     EXPECT_TURE(...)
+# will be expanded into something like:
+#   if(...)
+#     switch (0) case 0: default: if (...) ; else return;;


EXPECT_TURE -> EXPECT_TRUE
if(...) -> if (...)

jh7370 · 2024-10-28T08:33:16Z

llvm/unittests/CMakeLists.txt

+#     switch (0) case 0: default: if (...) ; else return;;
+# GCC may emit false positive dangling-else warnings for such code.
+# However, such warnings are actually against LLVM's style guide.
+# disable the warning for GCC so that one can enbable Werror.


disable -> Disable
enbable -> enable

I'd also do Werror -> -Werror to make it clear that this is an option.

…2817) This is one of the many PRs to fix errors with LLVM_ENABLE_WERROR=on. Built by GCC 11. Fix warnings: llvm/unittests/ProfileData/CoverageMappingTest.cpp: In member function ‘virtual void {anonymous}::CoverageMappingTest_TVIdxBuilder_Test::TestBody()’: llvm/unittests/ProfileData/CoverageMappingTest.cpp:1116:10: error: suggest explicit braces to avoid ambiguous ‘else’ [-Werror=dangling-else] 1116 | if (Node.NextIDs[C] < 0) The problem here is because these macros, eg: EXPECT_TRUE are expanded to a single line multi-statement code with if/else, which is indeed ambiguous after pre-processing. a simple example would be like: https://godbolt.org/z/4zjn56qrP if(x) switch (0) case 0: default: if (...) ; else return;; Given that omit braces in such cases is part of LLVM's style guide, and it is hard to add braces in gtest just for GCC's warning, add -Wno-dangling-else for GCC instead.

llvmbot added PGO Profile Guided Optimizations flang:openmp llvm:ir clang:openmp OpenMP related changes to Clang labels Oct 18, 2024

jsji requested review from goldsteinn, jh7370 and nikic October 18, 2024 03:13

jsji self-assigned this Oct 18, 2024

nikic reviewed Oct 18, 2024

View reviewed changes

jsji requested a review from nikic October 18, 2024 13:48

jsji added 3 commits October 19, 2024 09:26

Revert "[NFC] Fix dangling-else warning"

0a5a061

This reverts commit ea33083.

disable dangling else for gcc

fc75fa2

jsji force-pushed the danglingelse branch from 3767241 to fc75fa2 Compare October 19, 2024 13:26

teresajohnson approved these changes Oct 26, 2024

View reviewed changes

jsji changed the title ~~[NFC] Fix dangling-else warning~~ [NFC] [CMake] Add -Wno-dangling-else for GCC built unittests Oct 26, 2024

Update comments

2015b06

jsji merged commit d72e711 into llvm:main Oct 26, 2024
4 of 7 checks passed

jh7370 reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC] [CMake] Add -Wno-dangling-else for GCC built unittests #112817

[NFC] [CMake] Add -Wno-dangling-else for GCC built unittests #112817

Uh oh!

jsji commented Oct 18, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 18, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 18, 2024

Uh oh!

nikic left a comment

Uh oh!

teresajohnson commented Oct 18, 2024

Uh oh!

jsji commented Oct 21, 2024

Uh oh!

jsji commented Oct 21, 2024 •

edited

Loading

Uh oh!

jsji commented Oct 22, 2024

Uh oh!

teresajohnson left a comment

Uh oh!

teresajohnson Oct 26, 2024

Uh oh!

jsji Oct 26, 2024

Uh oh!

Uh oh!

jh7370 left a comment

Uh oh!

jh7370 Oct 28, 2024

Uh oh!

jh7370 Oct 28, 2024

Uh oh!

Uh oh!

[NFC] [CMake] Add -Wno-dangling-else for GCC built unittests #112817

[NFC] [CMake] Add -Wno-dangling-else for GCC built unittests #112817

Uh oh!

Conversation

jsji commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 18, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

teresajohnson commented Oct 18, 2024

Uh oh!

jsji commented Oct 21, 2024

Uh oh!

jsji commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsji commented Oct 22, 2024

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

teresajohnson Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

jsji Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

jh7370 Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsji commented Oct 18, 2024 •

edited

Loading

llvmbot commented Oct 18, 2024 •

edited

Loading

jsji commented Oct 21, 2024 •

edited

Loading