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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:130
     m(ObjcIvar) \
+    m(HungarianNotation) \
 
----------------
Is this line needed?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:241
+  // Options
+  const static llvm::StringMap<std::string> Options = {
+      {"TreatStructAsClass", "false"}};
----------------
As you never use map like access for this, shouldn't it be an array.
The same goes for all the other StringMaps in this function


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:366
+    IdentifierNamingCheck::HungarianNotationOption &HNOption) {
+  std::string Section = "HungarianNotation.";
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:368
+
+  std::vector<std::string> HNOpts = {"TreatStructAsClass"};
+  for (auto const &Opt : HNOpts) {
----------------
However for this I can see that its mapping the same options as `Options` in `getHungarianNotationDefaultConfig()`.
Maybe `HNOpts` should be removed from here, `Option` from `getHungarianNotationDefaultConfig()` taken out of function scope and iterate over that array below. 

A similar approach could be made with HNDerviedTypes


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:369-370
+  std::vector<std::string> HNOpts = {"TreatStructAsClass"};
+  for (auto const &Opt : HNOpts) {
+    std::string Key = Section + "General." + Opt;
+    std::string Val = Options.get(Key, "");
----------------
Building these temporary strings is expensive. Better off having a SmallString contsructed outside the loop and fill the string for each iteration, saved on allocations.
The same buffer can be reused below for the other loops


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:424
+static bool
+isHungarianNotationOptionEnabled(const std::string OptionKey,
+                                 llvm::StringMap<std::string> StrMap) {
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:718
+
+    const static std::list<std::string> Keywords = {
+        // Constexpr specifiers
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86671/new/

https://reviews.llvm.org/D86671