[clang-format] throws an incorrect assertion in consumeToken() formatting the MSVC stl

[MyDeveloperDay]

Summary

An incorrect assertion is thrown when clang-formatting MSVC's STL library

Assertion failed: !Line.startsWith(tok::hash), file C:/llvm/llvm-project/clang/lib/Format/TokenAnnotator.cpp, line 847
Stack dump:
0.      Program arguments: C:\llvm\build\bin\clang-format.exe -i -n ./stl/inc/xkeycheck.h

#if defined(while)
#define while EMIT WARNING C4005
#error The C++ Standard Library forbids macroizing the keyword "while". \
Enable warning C4005 to find the forbidden define.
#endif // while

[mitchell-stellar]

clang/lib/Format/TokenAnnotator.cpp

847

It's not clear to me whether or not the token should be consumed. The previous assertion leads me to think no, and in that case, I think this should be

if (Line.startsWith(tok::hash))
    return false;

A comment on this would also be helpful.

[MyDeveloperDay]

I'm not even sure if the assertion is valid?

[mitchell-stellar]

To me, whether or not the assertion was valid is irrelevant. That assertion was intentionally added, and its replacement with an if() statement materially changes the inputs and outputs of the function. I'm questioning whether the input of a "while" token within a preprocessor statement should output true or false. (Before, it didn't output anything; it errored.) Does this make sense? I'm just asking for clarification on this change.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

847

to be honest I'm not sure what the assertion is trying to actually assert (I'm not a massive fan of assertions especially like this), just because I see while am I not expecting to see a # at the beginning of the line?

I've tried both yours, mine and removing it completely and none of the test fail, so I feel like imposing the "Beyonce rule!!" and getting rid of it.

[mitchell-stellar]

Okay.

LGTM.

clang/lib/Format/TokenAnnotator.cpp

case tok::amp:
       // | and & in declarations/type expressions represent union and
       // intersection types, respectively.
       if (Style.Language == FormatStyle::LK_JavaScript &&
           !Contexts.back().IsExpression)
         Tok->Type = TT_JsTypeOperator;
       break;
     case tok::kw_if:
     case tok::kw_while:
-      assert(!Line.startsWith(tok::hash));
+      if (!Line.startsWith(tok::hash)) {
         if (Tok->is(tok::kw_if) && CurrentToken &&
             CurrentToken->isOneOf(tok::kw_constexpr, tok::identifier))
           next();
         if (CurrentToken && CurrentToken->is(tok::l_paren)) {
           next();
           if (!parseParens(/*LookForDecls=*/true))
             return false;
         }
+      }
       break;
     case tok::kw_for:
       if (Style.Language == FormatStyle::LK_JavaScript) {
         // x.for and {for: ...}
         if ((Tok->Previous && Tok->Previous->is(tok::period)) ||
             (Tok->Next && Tok->Next->is(tok::colon)))
           break;
         // JS' for await ( ...

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, AlternativeOperators) {
   verifyFormat("foo() <%%>;");      // foo();
   verifyFormat("void foo() <%%>;"); // void foo(){}
   verifyFormat("int a <:1:>;");     // int a[1];[
   verifyFormat("%:define ABC abc"); // #define ABC abc
   verifyFormat("%:%:");             // ##
   */
 }
 
-} // end namespace
-} // end namespace format
-} // end namespace clang
+TEST_F(FormatTest, STLWhileNotDefineChed) {
+  verifyFormat("#if defined(while)\n"
+               "#define while EMIT WARNING C4005\n"
+               "#endif // while");
+}
+
+} // namespace
+} // namespace format
+} // namespace clang