[clang-format] Improve annotating templates

[HazardyKnusperkeks]

The static_assert hinted to be an expression and when the || was hit the fate was doomed. As far as I see we can never be sure if it's an expression or a template, but we can improve the situation for common cases.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Björn Schäpers (HazardyKnusperkeks)

Changes

The static_assert hinted to be an expression and when the || was hit the fate was doomed. As far as I see we can never be sure if it's an expression or a template, but we can improve the situation for common cases.

---

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

2 Files Affected:

• (modified) clang/lib/Format/TokenAnnotator.cpp (+14-4)
• (modified) clang/unittests/Format/TokenAnnotatorTest.cpp (+24)

diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 021d8c658eb11..3ca4ec7e1dda1 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -150,14 +150,14 @@ class AnnotatingParser {
     if (!CurrentToken)
       return false;
 
-    auto *Left = CurrentToken->Previous; // The '<'.
+    auto *const Left = CurrentToken->Previous; // The '<'.
     if (!Left)
       return false;
 
     if (NonTemplateLess.count(Left) > 0)
       return false;
 
-    const auto *BeforeLess = Left->Previous;
+    const auto *const BeforeLess = Left->Previous;
 
     if (BeforeLess) {
       if (BeforeLess->Tok.isLiteral())
@@ -206,8 +206,18 @@ class AnnotatingParser {
               (!Next || Next->isNoneOf(tok::l_paren, tok::l_brace))) {
             return false;
           }
-          if (!MaybeAngles)
-            return false;
+          if (!MaybeAngles) {
+            const bool IsCommonCppTemplate =
+                (BeforeLess && BeforeLess->is(tok::identifier) &&
+                 (BeforeLess->TokenText.ends_with("_t") ||
+                  BeforeLess->TokenText.ends_with("_v"))) ||
+                (Next &&
+                 Next->startsSequence(tok::coloncolon, tok::identifier) &&
+                 (Next->Next->TokenText == "type" ||
+                  Next->Next->TokenText == "value"));
+            if (!IsCommonCppTemplate)
+              return false;
+          }
         }
         Left->MatchingParen = CurrentToken;
         CurrentToken->MatchingParen = Left;
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index c046142c613b0..8335cdade756a 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -799,6 +799,30 @@ TEST_F(TokenAnnotatorTest, UnderstandsTemplateTemplateParameters) {
   EXPECT_TOKEN(Tokens[23], tok::identifier, TT_ClassHeadName);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsCommonCppTemplates) {
+  auto Tokens =
+      annotate("static_assert(std::conditional_t<A || B, C, D>::value);");
+  ASSERT_EQ(Tokens.size(), 19u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[13], tok::greater, TT_TemplateCloser);
+
+  Tokens =
+      annotate("static_assert(std::conditional<A || B, C, D>::type::value);");
+  ASSERT_EQ(Tokens.size(), 21u) << Tokens;
+  EXPECT_TOKEN(Tokens[5], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[13], tok::greater, TT_TemplateCloser);
+
+  Tokens = annotate("static_assert(fancy_v<A || B>);");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[7], tok::greater, TT_TemplateCloser);
+
+  Tokens = annotate("static_assert(fancy<A || B>::value);");
+  ASSERT_EQ(Tokens.size(), 13u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::less, TT_TemplateOpener);
+  EXPECT_TOKEN(Tokens[7], tok::greater, TT_TemplateCloser);
+}
+
 TEST_F(TokenAnnotatorTest, UnderstandsWhitespaceSensitiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.WhitespaceSensitiveMacros.push_back("FOO");

[owenca]

We have the TemplateNames option for this. Is it too much to ask the user to disambiguate the input for clang-format by using the option?

[HazardyKnusperkeks]

I'd argue yes, because when a lot of people (basically everyone, once they hit such a construct) have to add a potential long list of templates, then clang-format should provide just a good out of the box experience.

[owenca]

The static_assert hinted to be an expression and when the || was hit the fate was doomed. As far as I see we can never be sure if it's an expression or a template, but we can improve the situation for common cases.

It seems to me that static_assert is a declaration with a boolean expression as its first argument. IMO it would be safer to fix that instead.

[HazardyKnusperkeks]

I can live with that.