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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:130
     m(ObjcIvar) \
+    m(HungarianNotation) \
 
----------------
njames93 wrote:
> Is this line needed?
I will remove it. Thank you.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:241
+  // Options
+  const static llvm::StringMap<std::string> Options = {
+      {"TreatStructAsClass", "false"}};
----------------
njames93 wrote:
> 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
Thank you. I will change it.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:368
+
+  std::vector<std::string> HNOpts = {"TreatStructAsClass"};
+  for (auto const &Opt : HNOpts) {
----------------
njames93 wrote:
> 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
I will move `HNOpts` and `HNDerviedTypes` outward to the top of the `getHungarianNotationDefaultConfig()` function. If the arrays are defined as static, is there any difference btw inside or outside of function, or did I misunderstand your meaning?  


================
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, "");
----------------
njames93 wrote:
> 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
Good idea, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671