[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.