This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms
ClosedPublic

Authored by benhamilton on Jan 18 2018, 12:52 PM.

Details

Summary

The existing option objc-property-declaration.Acronyms
replaces the built-in set of acronyms.

While this behavior is OK for clients that don't want the default
behavior, many clients may just want to add their own custom acronyms
to the default list.

This revision introduces a new option,
objc-property-declaration.IncludeDefaultAcronyms, which controls
whether the acronyms in objc-property-declaration.Acronyms are
appended to the default list (the default behavior) or whether they
replace.

I also updated the documentation.

Test Plan: make -j12 check-clang-tools

Diff Detail

Repository
rL LLVM

Event Timeline

benhamilton created this revision.Jan 18 2018, 12:52 PM
Eugene.Zelenko added inline comments.
docs/clang-tidy/checks/objc-property-declaration.rst
45 ↗(On Diff #130479)

Please limit string length to 80 symbols.

hokein added inline comments.Jan 19 2018, 1:35 AM
clang-tidy/objc/PropertyDeclarationCheck.h
38 ↗(On Diff #130479)

nit: code indent

docs/clang-tidy/checks/objc-property-declaration.rst
47 ↗(On Diff #130479)

It seems to me the Acronyms and AdditionalAcronyms play most same role here.

If we want to keep the long default list, how about using a bool flag IncludeDefaultList + the existing Acronyms option?

  • if IncludeDefaultList is on, the acronyms will be "default" + "Acronyms".
  • if IncludeDefaultList is off, the acronyms will be only "Acronyms".
benhamilton marked 2 inline comments as done.
  • Switch to IncludeDefaultAcronyms option (defaults to 1).
  • Use array for default acronyms, since we no longer need to parse it.
  • Don't regex-escape default acronyms, since we control them.

Thanks, fixed.

clang-tidy/objc/PropertyDeclarationCheck.h
38 ↗(On Diff #130479)

Ah, the previous one was wrong, I see. Fixed both.

docs/clang-tidy/checks/objc-property-declaration.rst
45 ↗(On Diff #130479)

Thanks, fixed.

47 ↗(On Diff #130479)

I think most people will want the "include default + add more" option. My goal was to make that possible, which is why I added the new AdditionalAcronyms option.

I agree the idea of a setting to control including the default list is nice, but I feel that should be on by default, since most users will want that.

If we add IncludeDefaultList, it should need to be on by default. That would break backwards compatibility for existing users. Do we think that's okay?

I'm assuming not a lot of people use this check yet, but I have no way of knowing that.

benhamilton edited the summary of this revision. (Show Details)Jan 19 2018, 10:31 AM
benhamilton retitled this revision from [clang-tidy objc-property-declaration] New option AdditionalAcronyms to [clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms.
  • Remove debugging code
hokein accepted this revision.Jan 22 2018, 1:02 AM

LGTM.

docs/clang-tidy/checks/objc-property-declaration.rst
47 ↗(On Diff #130479)

Yeah, IncludeDefaultList should be on by default.

This revision is now accepted and ready to land.Jan 22 2018, 1:02 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.