[PATCH] D42261: [clang-tidy objc-property-declaration] New option AdditionalAcronyms
Ben Hamilton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 19 10:31:18 PST 2018
benhamilton added a comment.
Thanks, fixed.
================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.h:38
const std::vector<std::string> SpecialAcronyms;
+ const std::vector<std::string> AdditionalAcronyms;
};
----------------
hokein wrote:
> nit: code indent
Ah, the previous one was wrong, I see. Fixed both.
================
Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:45
+
+ If set, replaces the default list. (If you want to append to the default list, set AdditionalAcronyms instead.)
+
----------------
Eugene.Zelenko wrote:
> Please limit string length to 80 symbols.
Thanks, fixed.
================
Comment at: docs/clang-tidy/checks/objc-property-declaration.rst:47
+
+.. option:: AdditionalAcronyms
+
----------------
hokein wrote:
> 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".
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42261
More information about the cfe-commits
mailing list