This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by arichardson on Aug 27 2020, 7:21 AM.

Details

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); }

Diff Detail

Event Timeline

arichardson created this revision.Aug 27 2020, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2020, 7:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arichardson requested review of this revision.Aug 27 2020, 7:21 AM
JakeMerdichAMD accepted this revision.Aug 27 2020, 8:31 AM

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

This revision is now accepted and ready to land.Aug 27 2020, 8:31 AM

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

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.

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.
This revision was landed with ongoing or failed builds.Aug 28 2020, 3:32 AM
This revision was automatically updated to reflect the committed changes.

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