[Lldb-commits] [PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

Aaron Ballman via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Nov 19 10:48:17 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 lldb-commits mailing list