[clang-format] ObjCPropertyAttributeOrder to sort ObjC property attributes

[jaredgrubb]

Summary

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

[rymiel]

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/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html

[jaredgrubb]

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]

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

[HazardyKnusperkeks]

LGTM, but someone other has to approve.

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

44–45

[MyDeveloperDay]

Interesting

clang/lib/Format/ObjCPropertyAttributeOrderFixer.cpp

70

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

71

Make a function, isAttribute()

[MyDeveloperDay]

clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

78

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?

[jaredgrubb]

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)

[MyDeveloperDay]

clang/unittests/Format/ObjCPropertyAttributeOrderFixerTest.cpp

157

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]

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

[jaredgrubb]

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

[jaredgrubb]

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

[MyDeveloperDay]

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.

clang/lib/Format/QualifierAlignmentFixer.cpp

10

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

llvm/utils/gn/secondary/clang/lib/Format/BUILD.gn

23

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

llvm/utils/gn/secondary/clang/unittests/Format/BUILD.gn

39

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

utils/bazel/llvm-project-overlay/clang/BUILD.bazel

1360

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

[MyDeveloperDay]

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