This is an archive of the discontinued LLVM Phabricator instance.

Differential D42261

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

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

Details

Reviewers
Wizard
hokein
klimek
Commits
rGf94c10d91a95: [clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms
rL323130: [clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms
rCTE323130: [clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms
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
Herald added a subscriber: cfe-commits. · View Herald TranscriptJan 18 2018, 12:52 PM
Harbormaster completed remote builds in B13989: Diff 130479.Jan 18 2018, 12:53 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.Jan 18 2018, 2:40 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 updated this revision to Diff 130657.Jan 19 2018, 10:30 AM
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.
benhamilton added a comment.Jan 19 2018, 10:30 AM

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.

Harbormaster completed remote builds in B14035: Diff 130657.Jan 19 2018, 10:31 AM
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.
benhamilton updated this revision to Diff 130661.Jan 19 2018, 10:32 AM
  • 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
Closed by commit rL323130: [clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms (authored by benhamilton). · Explain WhyJan 22 2018, 7:49 AM
Closed by commit rCTE323130: [clang-tidy objc-property-declaration] New option IncludeDefaultAcronyms (authored by benhamilton). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: llvm-commits. · View Herald TranscriptJan 22 2018, 7:49 AM

Revision Contents

PathSize
clang-tools-extra/
trunk/
clang-tidy/
objc/
3 lines
145 lines
docs/
clang-tidy/
checks/
22 lines
test/
clang-tidy/
15 lines
5 lines

Diff 130898

clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.h

Show All 28 Lines
class PropertyDeclarationCheck : public ClangTidyCheck { class PropertyDeclarationCheck : public ClangTidyCheck {
public: public:
PropertyDeclarationCheck(StringRef Name, ClangTidyContext *Context); PropertyDeclarationCheck(StringRef Name, ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void storeOptions(ClangTidyOptions::OptionMap &Options) override; void storeOptions(ClangTidyOptions::OptionMap &Options) override;
private: private:
const std::vector<std::string> SpecialAcronyms; const std::vector<std::string> SpecialAcronyms;
const bool IncludeDefaultAcronyms;
}; };
} // namespace objc } // namespace objc
} // namespace tidy } // namespace tidy
} // namespace clang } // namespace clang
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_OBJC_PROPERTY_DECLARATION_H

clang-tools-extra/trunk/clang-tidy/objc/PropertyDeclarationCheck.cpp

//===--- PropertyDeclarationCheck.cpp - clang-tidy-------------------------===// //===--- PropertyDeclarationCheck.cpp - clang-tidy-------------------------===//
// //
// The LLVM Compiler Infrastructure // The LLVM Compiler Infrastructure
// //
// This file is distributed under the University of Illinois Open Source // This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details. // License. See LICENSE.TXT for details.
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "PropertyDeclarationCheck.h" #include "PropertyDeclarationCheck.h"
#include "../utils/OptionsUtils.h" #include "../utils/OptionsUtils.h"
#include "clang/AST/ASTContext.h" #include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchFinder.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringExtras.h"
#include "llvm/Support/Regex.h" #include "llvm/Support/Regex.h"
#include <algorithm> #include <algorithm>
using namespace clang::ast_matchers; using namespace clang::ast_matchers;
namespace clang { namespace clang {
namespace tidy { namespace tidy {
namespace objc { namespace objc {
namespace { namespace {
/// The acronyms are from /// The acronyms are from
/// https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE /// https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE
/// ///
/// Keep this list sorted. /// Keep this list sorted.
constexpr char DefaultSpecialAcronyms[] = constexpr llvm::StringLiteral DefaultSpecialAcronyms[] = {
"ACL;" "ACL",
"API;" "API",
"ARGB;" "ARGB",
"ASCII;" "ASCII",
"BGRA;" "BGRA",
"CMYK;" "CMYK",
"DNS;" "DNS",
"FPS;" "FPS",
"FTP;" "FTP",
"GIF;" "GIF",
"GPS;" "GPS",
"HD;" "HD",
"HDR;" "HDR",
"HTML;" "HTML",
"HTTP;" "HTTP",
"HTTPS;" "HTTPS",
"HUD;" "HUD",
"ID;" "ID",
"JPG;" "JPG",
"JS;" "JS",
"LAN;" "LAN",
"LZW;" "LZW",
"MDNS;" "MDNS",
"MIDI;" "MIDI",
"OS;" "OS",
"PDF;" "PDF",
"PIN;" "PIN",
"PNG;" "PNG",
"POI;" "POI",
"PSTN;" "PSTN",
"PTR;" "PTR",
"QA;" "QA",
"QOS;" "QOS",
"RGB;" "RGB",
"RGBA;" "RGBA",
"RGBX;" "RGBX",
"ROM;" "ROM",
"RPC;" "RPC",
"RTF;" "RTF",
"RTL;" "RTL",
"SDK;" "SDK",
"SSO;" "SSO",
"TCP;" "TCP",
"TIFF;" "TIFF",
"TTS;" "TTS",
"UI;" "UI",
"URI;" "URI",
"URL;" "URL",
"VC;" "VC",
"VOIP;" "VOIP",
"VPN;" "VPN",
"VR;" "VR",
"WAN;" "WAN",
"XML"; "XML",
};
/// For now we will only fix 'CamelCase' property to /// For now we will only fix 'CamelCase' property to
/// 'camelCase'. For other cases the users need to /// 'camelCase'. For other cases the users need to
/// come up with a proper name by their own. /// come up with a proper name by their own.
/// FIXME: provide fix for snake_case to snakeCase /// FIXME: provide fix for snake_case to snakeCase
FixItHint generateFixItHint(const ObjCPropertyDecl *Decl) { FixItHint generateFixItHint(const ObjCPropertyDecl *Decl) {
if (isupper(Decl->getName()[0])) { if (isupper(Decl->getName()[0])) {
auto NewName = Decl->getName().str(); auto NewName = Decl->getName().str();
NewName[0] = tolower(NewName[0]); NewName[0] = tolower(NewName[0]);
return FixItHint::CreateReplacement( return FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())), CharSourceRange::getTokenRange(SourceRange(Decl->getLocation())),
llvm::StringRef(NewName)); llvm::StringRef(NewName));
} }
return FixItHint(); return FixItHint();
} }
std::string validPropertyNameRegex(const std::vector<std::string> &Acronyms) { std::string validPropertyNameRegex(const std::vector<std::string> &EscapedAcronyms) {
std::vector<std::string> EscapedAcronyms;
EscapedAcronyms.reserve(Acronyms.size());
// In case someone defines a custom prefix which includes a regex
// special character, escape all the prefixes.
std::transform(Acronyms.begin(), Acronyms.end(),
std::back_inserter(EscapedAcronyms), [](const std::string& s) {
return llvm::Regex::escape(s); });
// Allow any of these names: // Allow any of these names:
// foo // foo
// fooBar // fooBar
// url // url
// urlString // urlString
// URL // URL
// URLString // URLString
// bundleID // bundleID
return std::string("::((") + return std::string("::((") +
llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") + llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") +
")[A-Z]?)?[a-z]+[a-z0-9]*([A-Z][a-z0-9]+)*" + "(" + ")[A-Z]?)?[a-z]+[a-z0-9]*([A-Z][a-z0-9]+)*" + "(" +
llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") + ")?$"; llvm::join(EscapedAcronyms.begin(), EscapedAcronyms.end(), "|") + ")?$";
} }
} // namespace } // namespace
PropertyDeclarationCheck::PropertyDeclarationCheck(StringRef Name, PropertyDeclarationCheck::PropertyDeclarationCheck(StringRef Name,
ClangTidyContext *Context) ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), : ClangTidyCheck(Name, Context),
SpecialAcronyms(utils::options::parseStringList( SpecialAcronyms(
Options.get("Acronyms", DefaultSpecialAcronyms))) {} utils::options::parseStringList(Options.get("Acronyms", ""))),
IncludeDefaultAcronyms(Options.get("IncludeDefaultAcronyms", true)) {}
void PropertyDeclarationCheck::registerMatchers(MatchFinder *Finder) { void PropertyDeclarationCheck::registerMatchers(MatchFinder *Finder) {
std::vector<std::string> EscapedAcronyms;
if (IncludeDefaultAcronyms) {
EscapedAcronyms.reserve(llvm::array_lengthof(DefaultSpecialAcronyms) +
SpecialAcronyms.size());
// No need to regex-escape the default acronyms.
EscapedAcronyms.insert(EscapedAcronyms.end(),
std::begin(DefaultSpecialAcronyms),
std::end(DefaultSpecialAcronyms));
} else {
EscapedAcronyms.reserve(SpecialAcronyms.size());
}
// In case someone defines a prefix which includes a regex
// special character, regex-escape all the user-defined prefixes.
std::transform(SpecialAcronyms.begin(), SpecialAcronyms.end(),
std::back_inserter(EscapedAcronyms),
[](const std::string &s) { return llvm::Regex::escape(s); });
Finder->addMatcher( Finder->addMatcher(
objcPropertyDecl( objcPropertyDecl(
// the property name should be in Lower Camel Case like // the property name should be in Lower Camel Case like
// 'lowerCamelCase' // 'lowerCamelCase'
unless(matchesName(validPropertyNameRegex(SpecialAcronyms)))) unless(matchesName(validPropertyNameRegex(EscapedAcronyms))))
.bind("property"), .bind("property"),
this); this);
} }
void PropertyDeclarationCheck::check(const MatchFinder::MatchResult &Result) { void PropertyDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
const auto *MatchedDecl = const auto *MatchedDecl =
Result.Nodes.getNodeAs<ObjCPropertyDecl>("property"); Result.Nodes.getNodeAs<ObjCPropertyDecl>("property");
assert(MatchedDecl->getName().size() > 0); assert(MatchedDecl->getName().size() > 0);
diag(MatchedDecl->getLocation(), diag(MatchedDecl->getLocation(),
"property name '%0' should use lowerCamelCase style, according to " "property name '%0' should use lowerCamelCase style, according to "
"the Apple Coding Guidelines") "the Apple Coding Guidelines")
<< MatchedDecl->getName() << generateFixItHint(MatchedDecl); << MatchedDecl->getName() << generateFixItHint(MatchedDecl);
} }
void PropertyDeclarationCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { void PropertyDeclarationCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "Acronyms", Options.store(Opts, "Acronyms",
utils::options::serializeStringList(SpecialAcronyms)); utils::options::serializeStringList(SpecialAcronyms));
Options.store(Opts, "IncludeDefaultAcronyms", IncludeDefaultAcronyms);
} }
} // namespace objc } // namespace objc
} // namespace tidy } // namespace tidy
} // namespace clang } // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/objc-property-declaration.rst

Show All 31 Lines
The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757 The corresponding style rule: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingIvarsAndTypes.html#//apple_ref/doc/uid/20001284-1001757
Options Options
------- -------
.. option:: Acronyms .. option:: Acronyms
Semicolon-separated list of acronyms that can be used as a prefix Semicolon-separated list of custom acronyms that can be used as a prefix
or a suffix of property names. or a suffix of property names.
If unset, defaults to "ACL;API;ARGB;ASCII;BGRA;CMYK;DNS;FPS;FTP;GIF;GPS;HD;HDR;HTML;HTTP;HTTPS;HUD;ID;JPG;JS;LAN;LZW;MDNS;MIDI;OS;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;RGBX;ROM;RPC;RTF;RTL;SDK;SSO;TCP;TIFF;TTS;UI;URI;URL;VC;VOIP;VPN;VR;WAN;XML". By default, appends to the list of default acronyms (
``IncludeDefaultAcronyms`` set to ``1``).
If ``IncludeDefaultAcronyms`` is set to ``0``, instead replaces the
default list of acronyms.
.. option:: IncludeDefaultAcronyms
Integer value (defaults to ``1``) to control whether the default
acronyms are included in the list of acronyms.
If set to ``1``, the value in ``Acronyms`` is appended to the
default list of acronyms:
``ACL;API;ARGB;ASCII;BGRA;CMYK;DNS;FPS;FTP;GIF;GPS;HD;HDR;HTML;HTTP;HTTPS;
HUD;ID;JPG;JS;LAN;LZW;MDNS;MIDI;OS;PDF;PIN;PNG;POI;PSTN;PTR;QA;QOS;RGB;RGBA;
RGBX;ROM;RPC;RTF;RTL;SDK;SSO;TCP;TIFF;TTS;UI;URI;URL;VC;VOIP;VPN;VR;WAN;XML``.
If set to ``0``, the value in ``Acronyms`` replaces the default list
of acronyms.

clang-tools-extra/trunk/test/clang-tidy/objc-property-declaration-additional.m

// RUN: %check_clang_tidy %s objc-property-declaration %t \
// RUN: -config='{CheckOptions: \
// RUN: [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}]}' \
// RUN: --
@class NSString;
@interface Foo
@property(assign, nonatomic) int AbcNotRealPrefix;
// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'AbcNotRealPrefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
// CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix;
@property(assign, nonatomic) int ABCCustomPrefix;
@property(strong, nonatomic) NSString *ABC_custom_prefix;
// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'ABC_custom_prefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
@property(assign, nonatomic) int GIFShouldIncludeStandardAcronym;
@end

clang-tools-extra/trunk/test/clang-tidy/objc-property-declaration-custom.m

// RUN: %check_clang_tidy %s objc-property-declaration %t \ // RUN: %check_clang_tidy %s objc-property-declaration %t \
// RUN: -config='{CheckOptions: \ // RUN: -config='{CheckOptions: \
// RUN: [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}]}' \ // RUN: [{key: objc-property-declaration.Acronyms, value: "ABC;TGIF"}, \
// RUN: {key: objc-property-declaration.IncludeDefaultAcronyms, value: 0}]}' \
// RUN: -- // RUN: --
@class NSString; @class NSString;
@interface Foo @interface Foo
@property(assign, nonatomic) int AbcNotRealPrefix; @property(assign, nonatomic) int AbcNotRealPrefix;
// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'AbcNotRealPrefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration] // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'AbcNotRealPrefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
// CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix; // CHECK-FIXES: @property(assign, nonatomic) int abcNotRealPrefix;
@property(assign, nonatomic) int ABCCustomPrefix; @property(assign, nonatomic) int ABCCustomPrefix;
@property(strong, nonatomic) NSString *ABC_custom_prefix; @property(strong, nonatomic) NSString *ABC_custom_prefix;
// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'ABC_custom_prefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration] // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: property name 'ABC_custom_prefix' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
@property(assign, nonatomic) int GIFIgnoreStandardAcronym;
// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: property name 'GIFIgnoreStandardAcronym' should use lowerCamelCase style, according to the Apple Coding Guidelines [objc-property-declaration]
@end @end