[clang-format] Parse nullability attributes as a pointer qualifier

[arichardson]

Summary

Before:
void f() { MACRO(A * _Nonnull a); }
void f() { MACRO(A * _Nullable a); }
void f() { MACRO(A * _Null_unspecified a); }

After:
void f() { MACRO(A *_Nonnull a); }
void f() { MACRO(A *_Nullable a); }
void f() { MACRO(A *_Null_unspecified a); }

[JakeMerdichAMD]

LGTM, again assuming tests pass locally (patch did not resolve).

Out of curiosity, is _Atomic on your radar? I found some code in clang proper that handled restrict and _Atomic together. C/C++ have way too many qualifiers...

[arichardson]

In D86713#2242253, @JakeMerdichAMD wrote:

LGTM, again assuming tests pass locally (patch did not resolve).

Out of curiosity, is _Atomic on your radar? I found some code in clang proper that handled restrict and _Atomic together. C/C++ have way too many qualifiers...

I have not looked at _Atomic yet and it's probably low priority for me unless it's a trivial change.
My main motivation with these changes is to format a __capability pointer qualifier correctly (an extension that we add for our out-of-tree CHERI C/C++ dialect).

[JakeMerdichAMD]

In D86713#2242300, @arichardson wrote:

In D86713#2242253, @JakeMerdichAMD wrote:

LGTM, again assuming tests pass locally (patch did not resolve).

Out of curiosity, is _Atomic on your radar? I found some code in clang proper that handled restrict and _Atomic together. C/C++ have way too many qualifiers...

I have not looked at _Atomic yet and it's probably low priority for me unless it's a trivial change.
My main motivation with these changes is to format a __capability pointer qualifier correctly (an extension that we add for our out-of-tree CHERI C/C++ dialect).

I don't know of anyone who uses it yet, so just adding it for posterity, definitely not a blocker. Were you planning on handling __capability directly or in a user-configurable option? I can imagine other ad-hoc pointer qualifiers specific to static analysis tools or as properly-ifdef'd aliases of __attribute__(...), so an option might be useful.

[arichardson]

In D86713#2242354, @JakeMerdichAMD wrote:

In D86713#2242300, @arichardson wrote:

In D86713#2242253, @JakeMerdichAMD wrote:

LGTM, again assuming tests pass locally (patch did not resolve).

Out of curiosity, is _Atomic on your radar? I found some code in clang proper that handled restrict and _Atomic together. C/C++ have way too many qualifiers...

I have not looked at _Atomic yet and it's probably low priority for me unless it's a trivial change.
My main motivation with these changes is to format a __capability pointer qualifier correctly (an extension that we add for our out-of-tree CHERI C/C++ dialect).

I don't know of anyone who uses it yet, so just adding it for posterity, definitely not a blocker. Were you planning on handling __capability directly or in a user-configurable option? I can imagine other ad-hoc pointer qualifiers specific to static analysis tools or as properly-ifdef'd aliases of __attribute__(...), so an option might be useful.

I'm not sure yet what the best solution is. I was thinking of either

a) option to treat double-underscore prefixed strings after `*/&` as qualifiers (probably on by default, but not sure about that).
b) adding a config option with a list of strings that should be treated as qualifiers
c) Adding a new __capability keyword to clang-format since it already includes things such as Qt-specific keywords.
d) Option b) but with __capability included in the default list of qualifiers.

[arichardson]

In D86713#2242381, @arichardson wrote:

In D86713#2242354, @JakeMerdichAMD wrote:

In D86713#2242300, @arichardson wrote:

In D86713#2242253, @JakeMerdichAMD wrote:

LGTM, again assuming tests pass locally (patch did not resolve).

Out of curiosity, is _Atomic on your radar? I found some code in clang proper that handled restrict and _Atomic together. C/C++ have way too many qualifiers...

I have not looked at _Atomic yet and it's probably low priority for me unless it's a trivial change.
My main motivation with these changes is to format a __capability pointer qualifier correctly (an extension that we add for our out-of-tree CHERI C/C++ dialect).

I don't know of anyone who uses it yet, so just adding it for posterity, definitely not a blocker. Were you planning on handling __capability directly or in a user-configurable option? I can imagine other ad-hoc pointer qualifiers specific to static analysis tools or as properly-ifdef'd aliases of __attribute__(...), so an option might be useful.

I'm not sure yet what the best solution is. I was thinking of either

a) option to treat double-underscore prefixed strings after `*/&` as qualifiers (probably on by default, but not sure about that).
b) adding a config option with a list of strings that should be treated as qualifiers
c) Adding a new __capability keyword to clang-format since it already includes things such as Qt-specific keywords.
d) Option b) but with __capability included in the default list of qualifiers.

Implemented option d) in D86782 and fixed parsing of _Atomic in D86959

clang/lib/Format/TokenAnnotator.cpp

if (Style.isCSharp() && Tok.is(tok::ampamp))
       return TT_BinaryOperator;
 
     const FormatToken *PrevToken = Tok.getPreviousNonComment();
     if (!PrevToken)
       return TT_UnaryOperator;
 
     const FormatToken *NextToken = Tok.getNextNonComment();
     if (!NextToken ||
-        NextToken->isOneOf(tok::arrow, tok::equal, tok::kw_const,
-                           tok::kw_restrict, tok::kw_volatile,
-                           tok::kw___attribute, tok::kw_noexcept) ||
+        NextToken->isOneOf(
+            tok::arrow, tok::equal, tok::kw_const, tok::kw_restrict,
+            tok::kw_volatile, tok::kw___attribute, tok::kw__Nonnull,
+            tok::kw__Nullable, tok::kw__Null_unspecified, tok::kw_noexcept) ||
         (NextToken->is(tok::l_brace) && !NextToken->getNextNonComment()))
       return TT_PointerOrReference;
 
     if (PrevToken->is(tok::coloncolon))
       return TT_PointerOrReference;
 
     if (PrevToken->isOneOf(tok::l_paren, tok::l_square, tok::l_brace,
                            tok::comma, tok::semi, tok::kw_return, tok::colon,

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   verifyFormat("STATIC_ASSERT(0 == (a & b));");
   verifyFormat("template <bool a, bool b> "
                "typename t::if<x && y>::type f() {}");
   verifyFormat("template <int *y> f() {}");
   verifyFormat("vector<int *> v;");
   verifyFormat("vector<int *const> v;");
   verifyFormat("vector<int *const **const *> v;");
   verifyFormat("vector<int *volatile> v;");
+  verifyFormat("vector<a *_Nonnull> v;");
+  verifyFormat("vector<a *_Nullable> v;");
+  verifyFormat("vector<a *_Null_unspecified> v;");
+  verifyFormat("vector<a * _NotAQualifier> v;");
   verifyFormat("vector<a * b> v;");
   verifyFormat("foo<b && false>();");
   verifyFormat("foo<b & 1>();");
   verifyFormat("decltype(*::std::declval<const T &>()) void F();");
   verifyFormat(
       "template <class T, class = typename std::enable_if<\n"
       "                       std::is_integral<T>::value &&\n"
       "                       (sizeof(T) > 1 || sizeof(T) < 8)>::type>\n"
TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   verifyIndependentOfContext("MACRO(const A *a);");
   verifyIndependentOfContext("MACRO(A *const a);");
   verifyIndependentOfContext("MACRO(A *restrict a);");
   verifyIndependentOfContext("MACRO(A *__restrict__ a);");
   verifyIndependentOfContext("MACRO(A *__restrict a);");
   verifyIndependentOfContext("MACRO(A *volatile a);");
   verifyIndependentOfContext("MACRO(A *__volatile a);");
   verifyIndependentOfContext("MACRO(A *__volatile__ a);");
+  verifyIndependentOfContext("MACRO(A *_Nonnull a);");
+  verifyIndependentOfContext("MACRO(A *_Nullable a);");
+  verifyIndependentOfContext("MACRO(A *_Null_unspecified a);");
   verifyIndependentOfContext("MACRO(A *__attribute__((foo)) a);");
   verifyIndependentOfContext("MACRO(A *__attribute((foo)) a);");
   verifyIndependentOfContext("MACRO('0' <= c && c <= '9');");
   verifyFormat("void f() { f(float{1}, a * a); }");
   // FIXME: Is there a way to make this work?
   // verifyIndependentOfContext("MACRO(A *a);");
 
   verifyFormat("DatumHandle const *operator->() const { return input_; }");