[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 19 10:48:18 PST 2020
aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:237
+ std::string Val = HNOption.General.lookup(Opt.first);
+ if (Val.empty()) {
+ HNOption.General.insert({Opt.first, Opt.second.str()});
----------------
Usual style is to elide braces for single-line `if` statements (applies elsewhere in the patch as well).
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:254
+ static constexpr std::pair<StringRef, StringRef> CStrings[] = {
+ {"char*", "sz"}, {"char", "sz"}, {"wchar_t*", "wsz"}, {"wchar_t", "wsz"}};
+ for (const auto &CStr : CStrings) {
----------------
One thing that confused me was the plain `char` and `wchar_t` entries -- those are for arrays of `char` and `wchar_t`, aren't they? Can we use `char[]` to make that more clear? If not, can you add a comment to clarify?
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:376-380
+ static constexpr std::pair<StringRef, StringRef> HNCStrings[] = {
+ {"CharPrinter", "char*"},
+ {"CharArray", "char"},
+ {"WideCharPrinter", "wchar_t*"},
+ {"WideCharArray", "wchar_t"}};
----------------
Similar question here as above, but less pressing because we at least have the word "array" nearby.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:421-422
+ std::string OptionVal = StrMap.lookup(OptionKey);
+ for (auto &C : OptionVal)
+ C = toupper(C);
+
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:431
static IdentifierNamingCheck::FileStyle
-getFileStyleFromOptions(const ClangTidyCheck::OptionsView &Options) {
+getFileStyleFromOptions(const ClangTidyCheck::OptionsView &Options,
+ IdentifierNamingCheck::HungarianNotationOption &HNOption) {
----------------
Formatting
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:591-592
+ for (const auto &Type : HNOption.PrimitiveType) {
+ std::string Key = Type.getKey().str();
+ if (ModifiedTypeName == Key) {
+ PrefixStr = Type.getValue();
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:602-603
+ for (const auto &Type : HNOption.UserDefinedType) {
+ std::string Key = Type.getKey().str();
+ if (ModifiedTypeName == Key) {
+ PrefixStr = Type.getValue();
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:667
+ std::string Initial;
+ for (auto const &Word : Words) {
+ Initial += tolower(Word[0]);
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:678-680
+ if (clang::Decl::Kind::EnumConstant == ND->getKind() ||
+ clang::Decl::Kind::CXXMethod == ND->getKind() ||
+ clang::Decl::Kind::Function == ND->getKind()) {
----------------
No need to check for `CXXMethodDecl` because those inherit from `FunctionDecl` anyway.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:694
+ // character instead of the `getEndLoc()` function.
+ char *EOL = const_cast<char *>(strchr(Begin, '\n'));
+ if (!EOL) {
----------------
I think `EOL` should probably be `const char *` and can remove remove all these `const_cast<>`?
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:781
+ std::string Prefix;
+ switch (ND->getKind()) {
+ case clang::Decl::Kind::Var:
----------------
I think this `switch` should be replaced with:
```
if (const auto *ECD = dyn_cast<EnumConstantDecl>(ND)) {
...
} else if (const auto *CRD = dyn_cast<CXXRecordDecl>(ND)) {
...
} else if (isa<VarDecl, FieldDecl, RecordDecl>(ND)) {
...
}
```
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:786
+ case clang::Decl::Kind::Record:
+ if (ND) {
+ std::string TypeName = getDeclTypeName(ND);
----------------
Can remove all of the `if (ND)` in this method -- we already early return if `ND == nullptr`.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:826-827
+ for (const auto &Str : Map) {
+ bool Found = (Str.getValue() == CorrectName);
+ if (Found) {
+ Words.assign(Words.begin() + 1, Words.end());
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:874
+static std::string
+fixupWithCase(const StringRef &Type, const StringRef &Name, const Decl *D,
+ const IdentifierNamingCheck::NamingStyle &Style,
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1038
static std::string
-fixupWithStyle(StringRef Name,
- const IdentifierNamingCheck::NamingStyle &Style) {
- const std::string Fixed = fixupWithCase(
- Name, Style.Case.getValueOr(IdentifierNamingCheck::CaseType::CT_AnyCase));
+fixupWithStyle(const StringRef &Type, const StringRef &Name,
+ const IdentifierNamingCheck::NamingStyle &Style,
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1348
static llvm::Optional<RenamerClangTidyCheck::FailureInfo> getFailureInfo(
- StringRef Name, SourceLocation Location,
+ const StringRef &Type, const StringRef &Name, const NamedDecl *ND,
+ SourceLocation Location,
----------------
There's no need to pass a `StringRef` by const reference -- they pass like a pointer type already.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1389
+ return getFailureInfo(
+ TypeName, Decl->getName(), Decl, Loc, FileStyle.getStyles(), HNOption,
+ findStyleKind(Decl, FileStyle.getStyles(), FileStyle.isIgnoringMainLikeFunction()), SM,
----------------
And can remove the declaration of `TypeName` above.
Also, can you correct the formatting issues, elsewhere as well.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:125
+
+ IdentifierNamingCheck::HungarianNotationOption HNOption;
};
----------------
Might as well clear up this lint warning.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:55
return std::string(llvm::formatv(
- "Add using-declaration for {0} and remove qualifier.", Name));
+ "Add using-declaration for {0} and remove qualifier", Name));
}
----------------
This change looks unrelated to the patch?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:363
std::string title() const override {
- return "Move function body to out-of-line.";
+ return "Move function body to out-of-line";
}
----------------
Same here?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp:41
Expected<Effect> apply(const Selection &Inputs) override;
- std::string title() const override;
+ std::string title() const override {
+ return "Remove using namespace, re-qualify names instead";
----------------
And these changes as well?
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:110
+
+ When set to `true` the check will ensure the name will add Hungarian
+ Notation prefix for the given data type. The default value is `Off`.
----------------
For all of these, I'd recommend a slight wording tweak to:
```
When enabled, the check ensures that the declared identifier will have a Hungarian notation prefix based on the declared type.
```
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2264-2265
+
+This tool supports all casing types that means not only CamelCase starts with
+Hungarian Notation, others either.
+
----------------
I would remove this paragraph as it doesn't really add much value (it effectively says that the option works well with other options, but that's already expected).
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2312
+**HungarianNotation.CString.***
+ Options for NULL terminated string, you can customized your prefix here.
+
----------------
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2315
+**HungarianNotation.DerivedType.***
+ Options for derived types, you can customized your prefix here.
+
----------------
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2318
+**HungarianNotation.PrimitiveType.***
+ Options for primitive types, you can customized your prefix here.
+
----------------
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:2321-2322
+**HungarianNotation.UserDefinedType.***
+ Options for redefined types(Microsoft data types), you can customized your
+ prefix here.
+
----------------
================
Comment at: lldb/include/lldb/Target/Process.h:565
+ /// This does not change the pid of underlying process.
+ lldb::pid_t GetID() const { return m_pid; }
+
----------------
All the changes in this file are unrelated.
================
Comment at: lldb/source/Target/Process.cpp:532
const UnixSignalsSP &unix_signals_sp)
- : ProcessProperties(this), UserID(LLDB_INVALID_PROCESS_ID),
+ : ProcessProperties(this),
Broadcaster((target_sp->GetDebugger().GetBroadcasterManager()),
----------------
Unrelated changes.
================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:167
if (isTypeLegal(VT)) {
+ setOperationAction(ISD::ABS, VT, Legal);
+
----------------
All the changes in this file are unrelated.
================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.h:54
- // Integer absolute.
- IABS,
-
----------------
Unrelated changes.
================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.td:835
let CCValues = 0xF, CompareZeroCCMask = 0x8 in {
- def LPR : UnaryRR <"lpr", 0x10, z_iabs, GR32, GR32>;
- def LPGR : UnaryRRE<"lpgr", 0xB900, z_iabs, GR64, GR64>;
+ def LPR : UnaryRR <"lpr", 0x10, abs, GR32, GR32>;
+ def LPGR : UnaryRRE<"lpgr", 0xB900, abs, GR64, GR64>;
----------------
All the changes in this file are unrelated.
================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrVector.td:574
def VLP : UnaryVRRaGeneric<"vlp", 0xE7DF>;
- def VLPB : UnaryVRRa<"vlpb", 0xE7DF, z_viabs8, v128b, v128b, 0>;
- def VLPH : UnaryVRRa<"vlph", 0xE7DF, z_viabs16, v128h, v128h, 1>;
- def VLPF : UnaryVRRa<"vlpf", 0xE7DF, z_viabs32, v128f, v128f, 2>;
- def VLPG : UnaryVRRa<"vlpg", 0xE7DF, z_viabs64, v128g, v128g, 3>;
+ def VLPB : UnaryVRRa<"vlpb", 0xE7DF, abs, v128b, v128b, 0>;
+ def VLPH : UnaryVRRa<"vlph", 0xE7DF, abs, v128h, v128h, 1>;
----------------
Unrelated changes.
================
Comment at: llvm/lib/Target/SystemZ/SystemZOperators.td:669
// Negative integer absolute.
-def z_inegabs : PatFrag<(ops node:$src), (ineg (z_iabs node:$src))>;
-
-// Integer absolute, matching the canonical form generated by DAGCombiner.
-def z_iabs32 : PatFrag<(ops node:$src),
- (xor (add node:$src, (sra node:$src, (i32 31))),
- (sra node:$src, (i32 31)))>;
-def z_iabs64 : PatFrag<(ops node:$src),
- (xor (add node:$src, (sra node:$src, (i32 63))),
- (sra node:$src, (i32 63)))>;
-def z_inegabs32 : PatFrag<(ops node:$src), (ineg (z_iabs32 node:$src))>;
-def z_inegabs64 : PatFrag<(ops node:$src), (ineg (z_iabs64 node:$src))>;
+def z_inegabs : PatFrag<(ops node:$src), (ineg (abs node:$src))>;
----------------
All the changes in this file are unrelated.
================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:15
#include "llvm/Transforms/Scalar/ConstraintElimination.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
----------------
All the changes in this file are unrelated.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86671/new/
https://reviews.llvm.org/D86671
More information about the cfe-commits
mailing list