[clang-tidy] Fix PR35824

[xazax.hun]

Summary

This approach will also introduce false negatives.
A better approach would be to check if the null statement is the result of folding an if constexpr.
The current AST API does not expose this information.
A better fix might require AST patches on the clang side.
The fix is proposed here: https://reviews.llvm.org/D46234

Richard suggested to replace null stmts with nulls, but I have no time to deal with all the fallouts for now. If someone is willing to pick that up feel free to, but until then it might be a good idea to land this quick fix.

[aaron.ballman]

Which solution do you prefer?

If I understand the issue properly: both. :-)

Having the AST track information that's been folded away is still useful -- some users are using the AST for purposes other than codegen, and the fact that a construct has been folded away is good to know about while still retaining as much AST fidelity as possible.

On the other hand, from an AST matcher perspective, I think it's natural for users to write ifStmt(isConstexpr()) and so that seems like a useful extension to the matcher. Further, it is extensible if the committee adds other constexpr foo statements.

As for which solution gets used by this check to fix the PR, I don't have a strong opinion at this time (currently at WG14 meetings and a bit distracted).

[ZaMaZaN4iK]

What is the status of the PR?

[JonasToth]

In D46027#1259989, @ZaMaZaN4iK wrote:

What is the status of the PR?

I believe xazax doesnt have time to work further, you can commandeer if you want :)

[xazax.hun]

• Use matcher.

[JonasToth]

Maybe a short notice in the release notes wouldn't hurt. Otherwise LGTM

*EDIT*: Aaron commented as well. Plz let him react before committing :)

[xazax.hun]

Thanks!

Updated context for this patch:
A superior fix would be to follow through with the approach suggested by Richard in https://reviews.llvm.org/D46234 . However, since I do not have time to finish that and there are people complaining in the PR, I think it is better to land this.

[alexfh]

This approach will also introduce false negatives.

Could you add a test showing this with a FIXME comment?

A better approach would be to check if the null statement is the result of folding an if constexpr.
The current AST API does not expose this information.
A better fix might require AST patches on the clang side.
The fix is proposed here: https://reviews.llvm.org/D46234

Richard suggested to replace null stmts with nulls, but I have no time to deal with all the fallouts for now. If someone is willing to pick that up feel free to, but until then it might be a good idea to land this quick fix.

Could you add a FIXME comment to this effect?

Otherwise LG.

clang-tools-extra/clang-tidy/bugprone/SuspiciousSemicolonCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 void SuspiciousSemicolonCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       stmt(anyOf(ifStmt(hasThen(nullStmt().bind("semi")),
-                        unless(hasElse(stmt()))),
+                        unless(hasElse(stmt())),
+                        unless(isConstexpr())),
                  forStmt(hasBody(nullStmt().bind("semi"))),
                  cxxForRangeStmt(hasBody(nullStmt().bind("semi"))),
                  whileStmt(hasBody(nullStmt().bind("semi")))))
           .bind("stmt"),
       this);
 }
 
 void SuspiciousSemicolonCheck::check(const MatchFinder::MatchResult &Result) {

clang-tools-extra/test/clang-tidy/bugprone-suspicious-semicolon-constexpr.cpp

+// RUN: %check_clang_tidy %s bugprone-suspicious-semicolon %t -- -- -std=c++17
+
+void fail()
+{
+  int x = 0;
+  if(x > 5); (void)x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: potentially unintended semicolon [bugprone-suspicious-semicolon]
+  // CHECK-FIXES: if(x > 5) (void)x;
+}
+
+template <int X>
+int foo(int a) {
+    if constexpr(X > 0) {
+        return a;
+    }
+    return a + 1;
+}
+
+template <int X>
+int foo2(int a) {
+    // FIXME: diagnose the case below. See https://reviews.llvm.org/D46234
+    // for details.
+    if constexpr(X > 0);
+        return a;
+    return a + 1;
+}
+
+int main(void) {
+    foo2<0>(1);
+    return foo<0>(1);
+}