[clang-tidy] Fix an assert failure of ForStmt in `readability-braces-around-statements` check.

[alexfh]

test/clang-tidy/readability-braces-around-statements-assert-failure.cpp

13 ↗

(On Diff #47617)

Can you further reduce the test case so it still causes a crash, but doesn't generate so many compilation errors?

The problem with matching compilation errors in clang-tidy tests is that a change in Clang parser can break clang-tidy tests. The more errors you verify, the higher the chance of this kind of a breakage. If we need to verify clang errors, we should keep their number minimal.

[hokein]

test/clang-tidy/readability-braces-around-statements-assert-failure.cpp

13 ↗

(On Diff #47617)

Indeed, this is the minimal test case.

Is there a way to modify the test script to ignore message of compilation errors?

[alexfh]

test/clang-tidy/readability-braces-around-statements-assert-failure.cpp

13 ↗

(On Diff #47617)

Interesting. Does creduce fail to further reduce the test?

As for ignoring errors, it's rather rarely needed in tests. Usually, we need the reverse: to ensure tests compile without errors. If we want to ignore errors here, we can either run clang-tidy and FileCheck directly from the test or add a parameter to the script to modify the -implicit-check-not parameter of the FileCheck.

[hokein]

Make test ignore all compliation errors.

[hokein]

test/clang-tidy/readability-braces-around-statements-assert-failure.cpp

16 ↗

(On Diff #47795)

Interesting. Does creduce fail to further reduce the test?

I reduce the test case manually.

Now I use FileCheck with -implicit-check-not parameter to ignore all compilation errors.

[alexfh]

test/clang-tidy/readability-braces-around-statements-assert-failure.cpp

8 ↗

(On Diff #47795)

Tests shouldn't include any library headers. This is problematic in many different ways:

• standard library can be different in different setups, which may cause test breakages;
• including system headers may not work in some test setups;
• including system headers may lead to significant increase in testing time;
• there's no way to test something with different library implementations in the same test setup.

This is why we usually model APIs needed for the test in the tests (e.g. see test/clang-tidy/misc-inaccurate-erase.cpp). In this specific case the check doesn't need an actual std::vector<>, it just needs to simulate a specific compile error that would lead to a specific incompleteness in the AST. So, please, reduce the test case (the largest reduction would be the removal of #include <vector>) ;)

16 ↗

(On Diff #47795)

I reduce the test case manually.

That's why I was asking about creduce. It can do a much better job with a much less manual effort ;)

[hokein]

Minimize test case.

[hokein]

test/clang-tidy/readability-braces-around-statements-assert-failure.cpp

1 ↗

(On Diff #47959)

This is the simplest way to ignore all the compilation errors while keeping assert failure in the test. The check-clang-tools test script handles all stuff for us.

7 ↗

(On Diff #47959)

Reduce to the minimal test case now. C-Reduce is a useful tool :D.

[alexfh]

Looks good. Thank you!

[hokein]

Add a note in testcase.

clang-tools-extra/trunk/clang-tidy/readability/BracesAroundStatementsCheck.cpp

bool BracesAroundStatementsCheck::checkStmt(
   // token, the check inserts "\n}" right before that token.
   // 3) Otherwise the check finds the end of line (possibly after some block or
   // line comments) and inserts "\n}" right before that EOL.
   if (!S || isa<CompoundStmt>(S)) {
     // Already inside braces.
     return false;
   }
 
+  if (!InitialLoc.isValid())
+    return false;
   const SourceManager &SM = *Result.SourceManager;
   const ASTContext *Context = Result.Context;
 
   // Treat macros.
   CharSourceRange FileRange = Lexer::makeFileCharRange(
       CharSourceRange::getTokenRange(S->getSourceRange()), SM,
       Context->getLangOpts());
   if (FileRange.isInvalid())
     return false;
 
-  // InitialLoc points at the last token before opening brace to be inserted.
-  assert(InitialLoc.isValid());
   // Convert InitialLoc to file location, if it's on the same macro expansion
   // level as the start of the statement. We also need file locations for
   // Lexer::getLocForEndOfToken working properly.
   InitialLoc = Lexer::makeFileCharRange(
                    CharSourceRange::getCharRange(InitialLoc, S->getLocStart()),
                    SM, Context->getLangOpts())
                    .getBegin();
   if (InitialLoc.isInvalid())

clang-tools-extra/trunk/test/clang-tidy/readability-braces-around-statements-assert-failure.cpp

-// RUN: %check_clang_tidy %s readability-braces-around-statements %t
+// RUN: clang-tidy -checks='-*,readability-braces-around-statements' %s --
+
+// Note: this test expects no assert failure happened in clang-tidy.
 
 int test_failure() {
   if (std::rand()) {
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: error: use of undeclared identifier 'std'
   }
 }
+
+void test_failure2() {
+  for (a b c;;
+}