Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes
Needs ReviewPublic

Authored by jaredgrubb on May 7 2023, 9:02 PM.



Add a style option to specify the order that property attributes should appear in ObjC property declarations (property attributes are things like`nonatomic, strong, nullable`).

Diff Detail

Event Timeline

jaredgrubb created this revision.May 7 2023, 9:02 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 7 2023, 9:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to clang/include/clang/Format/Format.h but does not contain an update to ClangFormatStyleOptions.rst

ClangFormatStyleOptions.rst is generated via clang/docs/tools/, please run this to regenerate the .rst

You can validate that the rst is valid by running.

mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html
jaredgrubb requested review of this revision.May 7 2023, 9:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2023, 9:02 PM

Some implementation notes:

  • The implementation was modeled after QualifierAlignmentFixer and sortCppIncludes.
  • the additions to the .bazel/.gn build files was done naively based on searching for where "QualifierFixerTest" appeared in the repo; I can't really test these. It looks right, but it's a guess.
  • I considered creating a top-level style similar to QualifierAlignment/QualifierOrder; that one has a few pre-canned values (Leave, Left, Right, Custom), where Custom opens up the style QualifierOrder, an array of qualifier names.
    • Pro: we could create a [Leave, LLVM] that would set them in the same order the DeclPrinter.cpp would dump them
    • Pro: users probably would like not having to think about 18 different attributes (I did try to document them in groups to make them easier to find)
    • Con: I couldn't find any style guides that provide a "standard order", so I don't know any other pre-canned settings that wouldn't just be an opinion of my own personal style. If there are, let me know and that might add weight to going this route.

I don't like that there are 18 attributes that users _probably_ want to specify for the style, so having some shortcut seems helpful. Perhaps I could just document it that way so it's easy to copy-paste? I also considered making it possible to have an alias so the style would only need to have a handful of them to get full coverage (eg, if you specify assign in the style, then the tool would infer you also mean to imply that the similar attributes retain, strong, copy, weak, unsafe_unretained would occupy the same "slot" unless you also specify them explicitly).

jaredgrubb updated this revision to Diff 520247.May 7 2023, 9:26 PM

Update ClangFormatStyleOptions.rst as requested by the auto-comment (thanks auto-bot!).

LGTM, but someone other has to approve.




You need a unit tests that tests all the keywords or if one becomes a non identifier then it will break


Make a function, isAttribute()

MyDeveloperDay added inline comments.May 11 2023, 2:14 AM

might it be a good idea to actually test some/all of the keywords you intend to support? just incase some of them are not actually tok::identifiers?

Address review comments:

  • fix some style
  • add unit test for each ObjC attribute recognized by the compiler
  • adjust the docs for the style-option to show a YAML example with all of them in a sane order (something people could copy-paste as a starter version)
jaredgrubb marked 3 inline comments as done.May 13 2023, 3:05 PM
MyDeveloperDay added inline comments.May 14 2023, 8:44 AM

I'm not a massive fan of writing looping logic in tests, I prefer just to lay them all out so when it fails I know precisely where and why. I don't want to have to debug a failure. I want it to be very obvious. My concern here in the first place is if someone makes one of your words a keyword further down the link, this is why you should have specific examples.

jaredgrubb marked an inline comment as done.

Address review comments: unroll a loop in unit tests to explicitly test all the property attributes.

jaredgrubb marked an inline comment as done.May 22 2023, 9:21 AM

Are there any other comments on this patch? I would love to make this into clang-17!

Rebased (no other updates) to re-run build and hopefully get some eyes on this.

I don't have a problem with this, but I'm not an ObjC person so I can't really review from the validity of what its changing. If @owenpan and @HazardyKnusperkeks don't have an problem with this then I don't either as it likely won't impact most C++ users.


This is an unrelated change, it wrong and that was probably my fault, we should do this as a NFC


I never change these file either I think there is a bot or something that does this.


I never change these file either I think there is a bot or something that does this.


I never change these files I don't think we need to?

may remove the other build system files, I don't think its our responsiblity to keep them upto date.