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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2921424 , @thakis wrote:

> Any news here? This has been broken for over a day now.

Hi @thakis, 
YES. I just create a new differential diff for it, 
https://reviews.llvm.org/D107325, could you help me to review it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2920570 , @aeubanks wrote:

> you can reopen the revision via "Add Action..."

Hi @aeubanks , thank you for the suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Any news here? This has been broken for over a day now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

you can reopen the revision via "Add Action..."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

In D86671#2920294 , @dougpuob wrote:

> In D86671#2919077 , @thakis wrote:
>
>> This breaks tests on windows: http://45.33.8.238/win/43180/step_8.txt
>>
>> "The command line is too long" Maybe some arts could be passed via a 
>> response file instead?
>
> Hi @thakis :
> Thank you.
>
> I'm a newbie. I tried to arc diff but arc shows `ERR_CLOSED: This revision 
> has already been closed.`. Should I create a new differential diff for it?

Yes :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2919077 , @thakis wrote:

> This breaks tests on windows: http://45.33.8.238/win/43180/step_8.txt
>
> "The command line is too long" Maybe some arts could be passed via a response 
> file instead?

Hi @thakis :
Thank you.

I'm a newbie. I tried to arc diff but arc shows `ERR_CLOSED: This revision has 
already been closed.`. Should I create a new differential diff for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks tests on windows: http://45.33.8.238/win/43180/step_8.txt

"The command line is too long" Maybe some arts could be passed via a response 
file instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2837818 , @gnossier wrote:

> Is this ready to merge soon?

Hi  @gnossier:
I'm waiting for feedbacks from reviewer.

Hi @aaron.ballman:
Nathan is helping me to review this patch, but seems he is not here recently. 
Do you have any suggestion about how to keep the ball rolling in this 
situation? If it was done, can I apply the right to land it by self?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Grant Nossier via Phabricator via cfe-commits]

gnossier added a comment.

Is this ready to merge soon?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

@njames93, Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2754542 , @njames93 wrote:

> In D86671#2750957 , @dougpuob wrote:
>
>> Hi @njames93:
>> Could you do me a favor? Because it is my first patch, something I'm not 
>> sure. I'm confused about can I land this patch now? I read the "LLVM 
>> Code-Review Policy and Practices" document, it said patches can be landed if 
>> received a LGTM, but seems you are still reviewing.
>
> If you have made significant changes (excluding what a reviewer asks when 
> giving an LGTM) Its best to get those further changes also reviewed.

Thank you for your reply and suggestion in code, I will try it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

I think the best method is to

In D86671#2750957 , @dougpuob wrote:

> Hi @njames93:
> Could you do me a favor? Because it is my first patch, something I'm not 
> sure. I'm confused about can I land this patch now? I read the "LLVM 
> Code-Review Policy and Practices" document, it said patches can be landed if 
> received a LGTM, but seems you are still reviewing.

If you have made significant changes (excluding what a reviewer asks when 
giving an LGTM) Its best to get those further changes also reviewed.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:348
+getFileStyleFromOptions(const ClangTidyCheck::OptionsView ,
+ClangTidyContext ) {
+  IdentifierNamingCheck::HungarianNotationOption HNOption;

Rather than passing the ClangTidyContext, Make this function a method of 
IdentifierNamingCheck. Then you call create the Diag without worrying about the 
Context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Hi @njames93:
Could you do me a favor? Because it is my first patch, something I'm not sure. 
I'm confused about can I land this patch now? I read the "LLVM Code-Review 
Policy and Practices" document, it said patches can be landed if received a 
LGTM, but seems you are still reviewing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Hi @njames93

I tried to create a class, `HungarianNotation`, and put all related functions 
into the class. Then I can call `configurationDiag()` in 
`HungarianNotation::checkOptionValid()` function.
How about this way?

The new class.

  struct HungarianNotation {
  public:
HungarianNotation(ClangTidyContext *Context = nullptr);
  
bool checkOptionValid(int StyleKindIndex, StringRef StyleString,
  bool HasValue);
...

Use the configurationDiag() in checkOptionValid() function

  bool HungarianNotation::checkOptionValid(int StyleKindIndex,
   StringRef StyleString, bool 
HasValue) {
..
if (HasValue)
  Context->configurationDiag("invalid identifier naming option '%0'")
  << StyleString;
  
return false;
  }

Call the checkOptionValid() here in getFileStyleFromOptions().

  static IdentifierNamingCheck::FileStyle
  getFileStyleFromOptions(const ClangTidyCheck::OptionsView ,
  ClangTidyContext ) {
  ...
  StyleString.append("HungarianPrefix");
  auto HPTOpt =
  Options.get(StyleString);
  HN.checkOptionValid(I, StyleString, HPTOpt.hasValue());
  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/ClangTidyCheck.h:124-126
   DiagnosticBuilder
   configurationDiag(StringRef Description,
 DiagnosticIDs::Level Level = DiagnosticIDs::Warning);

njames93 wrote:
> You don't need a ClangTidyContext, this function is all that's needed.
But the `getFileStyleFromOptions()` is a static function in 
`IdentifierNamingCheck.cpp` file. It can't access `configurationDiag()` 
directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/ClangTidyCheck.h:124-126
   DiagnosticBuilder
   configurationDiag(StringRef Description,
 DiagnosticIDs::Level Level = DiagnosticIDs::Warning);

You don't need a ClangTidyContext, this function is all that's needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Hi @njames93,

That's ok. You have helped me a lots. Thank you.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:450
+if (!isHungarianNotationSupportedStyle(I) && HPTOpt.hasValue())
+  Options.diagnoseInvalidConfigOption(StyleString);
+StyleString.resize(StyleSize);

njames93 wrote:
> 
To use configurationDiag() function in getFileStyleFromOptions(), seems it 
needs to pass a ClangTidyContext object to getFileStyleFromOptions(). How about 
changing like the following?

```
getFileStyleFromOptions(const ClangTidyCheck::OptionsView ,
ClangTidyContext ) {
...

if (!isHungarianNotationSupportedStyle(I) && HPTOpt.hasValue())
  Context.configurationDiag("invalid identifier naming option '%0'")
  << StyleString;

...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Sorry, I thought this landed months ago. I'll take a proper look again next 
week.




Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:158
+/// name \p LocalName shoud not present.
+void diagnoseInvalidConfigOption(StringRef LocalName) const;
+

This isn't really needed, you can just call `configurationDiag` directly.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:450
+if (!isHungarianNotationSupportedStyle(I) && HPTOpt.hasValue())
+  Options.diagnoseInvalidConfigOption(StyleString);
+StyleString.resize(StyleSize);




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Ping @njames93


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Addressed comments by @njames93. Including adding warning message for 
unsupported options in config file, refining code in getFileStyleFromOptions(), 
and for consistent reason to use llvm::yaml::parseBool() function instead of 
checking On/Off string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2456161 , @njames93 wrote:

> One last point, is there a way to validate that these options are only set 
> where it makes sense. If someone tries to use say 
> `MacroDefinitionHungarianPrefix` That could be warning worthy?

Hi @njames93, it's a good idea. I am trying to do it, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

One last point, is there a way to validate that these options are only set 
where it makes sense. If someone tries to use say 
`MacroDefinitionHungarianPrefix` That could be warning worthy?




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:435-462
+std::string HPrefixKey = (StyleString + "HungarianPrefix").str();
+using HungarianPrefixType = IdentifierNamingCheck::HungarianPrefixType;
+auto HPTVal = HungarianPrefixType::HPT_Off;
+std::string HPrefixVal = Options.get(HPrefixKey, "");
+if (!HPrefixVal.empty()) {
+  if (auto HPTypeVal = Options.get(HPrefixKey))
+HPTVal = HPTypeVal.get();





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:398-404
+  std::string OptionVal = StrMap.lookup(OptionKey);
+  llvm::transform(OptionVal, OptionVal.begin(), toupper);
+
+  if (OptionVal == "1" || OptionVal == "TRUE" || OptionVal == "ON")
+return true;
+
+  return false;

njames93 wrote:
> There's no need to build a string and transform it here.
Scratch this, in D92756 support was added for all boolean options in clang-tidy 
to use the full YAML boolean spec, I'd advise calling the same function here to 
keep everything consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob marked an inline comment as done.
dougpuob added a comment.

Hi @njames93, thank you for your review suggestions, I have improved them and 
against my change to the main branch.

I encounter a problem on the Buildbot for Windows only. Several people 
encounter also to the same problem that their build failed at an unrelated 
regression test (llvm\test\CodeGen\XCore\threads.ll).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

This needs rebasing against main. Can't be applied cleanly in its current state.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:398-404
+  std::string OptionVal = StrMap.lookup(OptionKey);
+  llvm::transform(OptionVal, OptionVal.begin(), toupper);
+
+  if (OptionVal == "1" || OptionVal == "TRUE" || OptionVal == "ON")
+return true;
+
+  return false;

There's no need to build a string and transform it here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 309767.
dougpuob added a comment.

- Improved code review suggestions from @njames93. Including move the 
IdentifierNamingCheck::HNOption variable to 
IdentifierNamingCheck::FileStyle::HNOption, use try_emplace() instead of 
insert() and lookup().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -84,19 +84,26 @@
   struct FileStyle {
 FileStyle() : IsActive(false), IgnoreMainLikeFunctions(false) {}
 FileStyle(SmallVectorImpl> &,
-  bool IgnoreMainLike)
-: Styles(std::move(Styles)), IsActive(true),
-  IgnoreMainLikeFunctions(IgnoreMainLike) {}
+  HungarianNotationOption HNOption, bool IgnoreMainLike)
+: Styles(std::move(Styles)), HNOption(std::move(HNOption)),
+  IsActive(true), IgnoreMainLikeFunctions(IgnoreMainLike) {}
 
 ArrayRef> getStyles() const {
   assert(IsActive);
   return Styles;
 }
+
+const HungarianNotationOption () const {
+  assert(IsActive);
+  return HNOption;
+}
+
 bool isActive() const { return IsActive; }
 bool isIgnoringMainLikeFunction() const { return IgnoreMainLikeFunctions; }
 
   private:
 SmallVector, 0> Styles;
+HungarianNotationOption HNOption;
 bool IsActive;
 bool IgnoreMainLikeFunctions;
   };
@@ -121,8 +128,6 @@
   const std::string CheckName;
   const bool GetConfigPerFile;
   const bool IgnoreFailedSplit;
-
-  IdentifierNamingCheck::HungarianNotationOption HNOption;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -230,22 +230,16 @@
 IdentifierNamingCheck::HungarianNotationOption ) {
 
   // Options
-  static constexpr std::pair Options[] = {
+  static constexpr std::pair General[] = {
   {"TreatStructAsClass", "false"}};
-  for (const auto  : Options) {
-std::string Val = HNOption.General.lookup(Opt.first);
-if (Val.empty())
-  HNOption.General.insert({Opt.first, Opt.second.str()});
-  }
+  for (const auto  : General)
+HNOption.General.try_emplace(G.first, G.second);
 
   // Derived types
   static constexpr std::pair DerivedTypes[] = {
   {"Array", "a"}, {"Pointer", "p"}, {"FunctionPointer", "fn"}};
-  for (const auto  : DerivedTypes) {
-std::string Val = HNOption.DerivedType.lookup(Other.first);
-if (Val.empty())
-  HNOption.DerivedType.insert({Other.first, Other.second.str()});
-  }
+  for (const auto  : DerivedTypes)
+HNOption.DerivedType.try_emplace(DT.first, DT.second);
 
   // C strings
   static constexpr std::pair CStrings[] = {
@@ -253,11 +247,8 @@
   {"char[]", "sz"},
   {"wchar_t*", "wsz"},
   {"wchar_t[]", "wsz"}};
-  for (const auto  : CStrings) {
-std::string Val = HNOption.CString.lookup(CStr.first);
-if (Val.empty())
-  HNOption.CString.insert({CStr.first, CStr.second.str()});
-  }
+  for (const auto  : CStrings)
+HNOption.CString.try_emplace(CStr.first, CStr.second);
 
   // clang-format off
   static constexpr std::pair PrimitiveTypes[] = {
@@ -305,11 +296,8 @@
 {"long","l"   },
 {"ptrdiff_t",   "p"   }};
   // clang-format on
-  for (const auto  : PrimitiveTypes) {
-std::string Val = HNOption.PrimitiveType.lookup(Type.first);
-if (Val.empty())
-  HNOption.PrimitiveType.insert({Type.first, Type.second.str()});
-  }
+  for (const auto  : PrimitiveTypes)
+HNOption.PrimitiveType.try_emplace(PT.first, PT.second);
 
   // clang-format off
   static constexpr std::pair UserDefinedTypes[] = {
@@ -343,11 +331,8 @@
   {"UINT64",  "u64" },
   {"PVOID",   "p"   } };
   // clang-format on
-  for (const auto  : UserDefinedTypes) {
-std::string Val = HNOption.UserDefinedType.lookup(Type.first);
-if (Val.empty())
-  HNOption.UserDefinedType.insert({Type.first, Type.second.str()});
-  }
+  for (const auto  : UserDefinedTypes)
+HNOption.UserDefinedType.try_emplace(UDT.first, UDT.second);
 }
 
 static constexpr StringRef HNOpts[] = {"TreatStructAsClass"};
@@ -363,14 +348,14 @@
 Buffer.assign({Section, "General.", Opt});
 std::string Val = Options.get(Buffer, "");
 if (!Val.empty())
-  HNOption.General[Opt] = Val;
+  

[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:235-239
+  for (const auto  : Options) {
+std::string Val = HNOption.General.lookup(Opt.first);
+if (Val.empty())
+  HNOption.General.insert({Opt.first, Opt.second.str()});
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:244-248
+  for (const auto  : DerivedTypes) {
+std::string Val = HNOption.DerivedType.lookup(Other.first);
+if (Val.empty())
+  HNOption.DerivedType.insert({Other.first, Other.second.str()});
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:256-260
+  for (const auto  : CStrings) {
+std::string Val = HNOption.CString.lookup(CStr.first);
+if (Val.empty())
+  HNOption.CString.insert({CStr.first, CStr.second.str()});
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:308-312
+  for (const auto  : PrimitiveTypes) {
+std::string Val = HNOption.PrimitiveType.lookup(Type.first);
+if (Val.empty())
+  HNOption.PrimitiveType.insert({Type.first, Type.second.str()});
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:347-348
+  for (const auto  : UserDefinedTypes) {
+std::string Val = HNOption.UserDefinedType.lookup(Type.first);
+if (Val.empty())
+  HNOption.UserDefinedType.insert({Type.first, Type.second.str()});





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:366
+if (!Val.empty())
+  HNOption.General[Opt] = Val;
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:373
+if (!Val.empty())
+  HNOption.DerivedType[Type] = Val;
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:386
+if (!Val.empty())
+  HNOption.CString[CStr.first] = Val;
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:392-401
+std::string Type = PrimType.str();
+if (!Val.empty()) {
+  if (Type.find('-') != std::string::npos) {
+for (size_t I = 0; I < Type.length(); ++I) {
+  if (Type[I] == '-')
+Type.replace(I, 1, " ");
+}





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:407-409
+std::string Type = WDType.str();
+if (!Val.empty())
+  HNOption.UserDefinedType[Type] = Val;





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:124-125
   const bool IgnoreFailedSplit;
+
+  IdentifierNamingCheck::HungarianNotationOption HNOption;
 };

This should be part of the `FileStyle` struct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM! Thank you for the new functionality! You may want to wait a bit before 
landing to see if @njames93 has any remaining comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob marked 5 inline comments as done.
dougpuob added a comment.

Hi @aaron.ballman and @Eugene.Zelenko, thank you for your suggestions. I will 
improve them and upload my diff later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:614
+  if (CRD->isUnion())
+return "";
+

Returning {} as default initializer will be better. Same in other places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:616-619
+  if (CRD->isStruct() && 
!isHungarianNotationOptionEnabled("TreatStructAsClass",
+   HNOption.General)) {
+return "";
+  }





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:671-672
+
+  if (clang::Decl::Kind::EnumConstant == ND->getKind() ||
+  clang::Decl::Kind::Function == ND->getKind()) {
+return "";





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:740
+// Remove redundant tailing.
+const static std::list TailsOfMultiWordType = {
+" int", " char", " double", " long", " short"};

Similar to what was suggested above.



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

njames93 wrote:
> 
This comment doesn't appear to have been handled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob marked 82 inline comments as done.
dougpuob added a comment.

Hi @aaron.ballman, thank you for your feedback. I will update my change later. 
Unrelated change were mixed with other commits when I against git master. I did 
it again then the problem was gone. I found the command, `arc diff master 
--preview`, knowing exactly changes before uploading diff by arc.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:254
+  static constexpr std::pair CStrings[] = {
+  {"char*", "sz"}, {"char", "sz"}, {"wchar_t*", "wsz"}, {"wchar_t", 
"wsz"}};
+  for (const auto  : CStrings) {

aaron.ballman wrote:
> 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?
I improved it. It will look like the following:
```
static constexpr std::pair CStrings[] = {
  {"char*", "sz"}, {"char[]", "sz"}, {"wchar_t*", "wsz"}, {"wchar_t[]", 
"wsz"}};
```



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:376-380
+  static constexpr std::pair HNCStrings[] = {
+  {"CharPrinter", "char*"},
+  {"CharArray", "char"},
+  {"WideCharPrinter", "wchar_t*"},
+  {"WideCharArray", "wchar_t"}};

aaron.ballman wrote:
> Similar question here as above, but less pressing because we at least have 
> the word "array" nearby.
Improved here too. It will change to:

```
  static constexpr std::pair HNCStrings[] = {
  {"CharPrinter", "char*"},
  {"CharArray", "char[]"},
  {"WideCharPrinter", "wchar_t*"},
  {"WideCharArray", "wchar_t[]"}};
```



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:431
 static IdentifierNamingCheck::FileStyle
-getFileStyleFromOptions(const ClangTidyCheck::OptionsView ) {
+getFileStyleFromOptions(const ClangTidyCheck::OptionsView ,
+IdentifierNamingCheck::HungarianNotationOption ) {

aaron.ballman wrote:
> Formatting
OK. I checked all the range including single-line if statements, and passing 
StringRef directly(not its reference).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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 CStrings[] = {
+  {"char*", "sz"}, {"char", "sz"}, {"wchar_t*", "wsz"}, {"wchar_t", 
"wsz"}};
+  for (const auto  : 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 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  : OptionVal)
+C = toupper(C);
+





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:431
 static IdentifierNamingCheck::FileStyle
-getFileStyleFromOptions(const ClangTidyCheck::OptionsView ) {
+getFileStyleFromOptions(const ClangTidyCheck::OptionsView ,
+IdentifierNamingCheck::HungarianNotationOption ) {

Formatting



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:591-592
+for (const auto  : 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  : 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  : 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(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(ND)) {
  ...
} else if (const auto *CRD = dyn_cast(ND)) {
  ...
} else if (isa(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  : 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 , const StringRef , const Decl *D,
+  const IdentifierNamingCheck::NamingStyle ,





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1038
 static std::string
-fixupWithStyle(StringRef Name,
-   const IdentifierNamingCheck::NamingStyle ) {
-  const std::string Fixed = fixupWithCase(
-  Name, 

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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 HNOpts = {"TreatStructAsClass"};
+  for (auto const  : 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 HNOpts = {"TreatStructAsClass"};
+  for (auto const  : 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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 ) {
+  std::string Section = "HungarianNotation.";
+





Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:368
+
+  std::vector HNOpts = {"TreatStructAsClass"};
+  for (auto const  : 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 HNOpts = {"TreatStructAsClass"};
+  for (auto const  : 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 StrMap) {





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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:441-456
+static IdentifierNamingCheck::HungarianPrefixOption
+parseHungarianPrefix(std::string OptionVal) {
+  for (auto  : OptionVal)
+C = toupper(C);
+
+  if (std::string::npos != OptionVal.find("LOWER_CASE"))
+return IdentifierNamingCheck::HungarianPrefixOption::HPO_LowerCase;

njames93 wrote:
> This isn't really needed if you have the mapping defined, `Options.get` works 
> with enums, just look at how CaseType is parsed and stored. If you want to 
> map multiple strings to a single enum constant that can also work by putting 
> both strings in the mapping.
> This method also validates inputs and will print out an error if a user 
> supplies a value that can't be converted.
Good idea. Thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:460
 getNamingStyles(const ClangTidyCheck::OptionsView ) {
+  static IdentifierNamingCheck::HungarianNotationOption HNOption;
+  HNOption.clearAll();

njames93 wrote:
> This function can be called multiple times per translation unit when looking 
> through header files if `GetConfigPerFile` is enabled. Making this static 
> will mean that each file thats read could potentially alter other files style 
> configuration.
> Maybe a smarter way about this is rather than this function returning a 
> vector of naming styles, it returns a struct which contains the Hungarian 
> options and a vector of the styles. Doing this would probably also mean you 
> don't need to store a reference to this in the `NamingStyle`.
I removed the static variable from the function and a reference in 
`NamingStyle`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:441-456
+static IdentifierNamingCheck::HungarianPrefixOption
+parseHungarianPrefix(std::string OptionVal) {
+  for (auto  : OptionVal)
+C = toupper(C);
+
+  if (std::string::npos != OptionVal.find("LOWER_CASE"))
+return IdentifierNamingCheck::HungarianPrefixOption::HPO_LowerCase;

This isn't really needed if you have the mapping defined, `Options.get` works 
with enums, just look at how CaseType is parsed and stored. If you want to map 
multiple strings to a single enum constant that can also work by putting both 
strings in the mapping.
This method also validates inputs and will print out an error if a user 
supplies a value that can't be converted.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:460
 getNamingStyles(const ClangTidyCheck::OptionsView ) {
+  static IdentifierNamingCheck::HungarianNotationOption HNOption;
+  HNOption.clearAll();

This function can be called multiple times per translation unit when looking 
through header files if `GetConfigPerFile` is enabled. Making this static will 
mean that each file thats read could potentially alter other files style 
configuration.
Maybe a smarter way about this is rather than this function returning a vector 
of naming styles, it returns a struct which contains the Hungarian options and 
a vector of the styles. Doing this would probably also mean you don't need to 
store a reference to this in the `NamingStyle`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Can you rebase this against trunk please, having trouble testing it out locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Reply code review suggestions. I will upload my change later.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:483
+  std::move(CaseOptional), std::move(Prefix), std::move(Postfix),
+  std::move(HPOpt), HNOption});
+} else {

aaron.ballman wrote:
> Would it make sense to declare `HNOption` as a typical local variable (no 
> need for the `clearAll()` API since you can default construct properly), and 
> pass an rvalue reference rather than an lvalue reference (so call 
> std::move()) here, as with the other arguments)?
OK~ I moved the `HNOption` to IdentifierNamingCheck class as its private data 
member. Then give value by constructor initializer list (instead of 
`clearAll()`).



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:549
+  for (const auto  : HNOption.CString) {
+auto Key = CStr.getKey().str();
+if (ModifiedTypeName.find(Key) == 0) {

aaron.ballman wrote:
> You shouldn't use `auto` when the type isn't spelled out in the 
> initialization. (Same below)
OK~ Thank you. I will check them.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:631
+
+  if (CRD->isUnion()) {
 return "";

aaron.ballman wrote:
> Elide braces around single-line if statements, per our usual coding 
> guidelines.
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:635-636
 
-  if (clang::Decl::Kind::EnumConstant == Decl->getKind() ||
-  clang::Decl::Kind::CXXMethod == Decl->getKind()||
-  clang::Decl::Kind::Function == Decl->getKind()) {
+  if (CRD->isStruct()) {
+if (!isHungarianNotationOptionEnabled("TreatStructAsClass",
+  HNOption.General)) {

aaron.ballman wrote:
> Combine into a single `if`?
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:644
+  bool IsAbstractClass = false;
+  for (const auto *Method : CRD->methods()) {
+if (Method->isPure()) {

aaron.ballman wrote:
> No need to do this manually, you can use `CXXRecordData::isAbstract()` since 
> it's already computed.
Nice, thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:651-658
+  std::string Prefix;
+  if (IsAbstractClass) {
+  Prefix = "I";
+  } else {
+  Prefix = "C";
+  }
+

aaron.ballman wrote:
> `return CRD->isAbstract() ? "I" : "C";`
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:662-666
+  std::string Name = ECD->getType().getAsString();
+  if (std::string::npos != Name.find("enum")) {
+Name = Name.substr(strlen("enum"), Name.length() - strlen("enum"));
+Name = Name.erase(0, Name.find_first_not_of(" "));
+  }

aaron.ballman wrote:
> This is a bit worrying -- can you not look at the `DeclContext` for the 
> `EnumConstantDecl` to find the parent `EnumDecl` and ask for its name 
> directly?
YES. I get the its `QualType` from `EnumConstantDecl::getType()`. Then get 
`EnumDecl` name via `QualType::getAsString()` (if enum tag name is "DataType", 
the string I get is "enum DataType").

```
static std::string getHungarianNotationEnumPrefix(const EnumConstantDecl *ECD) {
  std::string Name = ECD->getType().getAsString();
  // ...
}
```



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:703
+
+static std::string getDeclTypeName(const clang::NamedDecl *ND) {
+  const auto VD = dyn_cast(ND);

aaron.ballman wrote:
> You can drop the `clang::` bit.
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:704
+static std::string getDeclTypeName(const clang::NamedDecl *ND) {
+  const auto VD = dyn_cast(ND);
+  if (!VD) {

aaron.ballman wrote:
> When using type deduction, the coding style is to put all pointers and 
> qualifiers on the declaration as well (so we don't have to infer them from 
> context), so this should be `const auto *`.
OK, thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:806
+static std::string getHungarianNotationPrefix(
+const clang::Decl *D,
+IdentifierNamingCheck::HungarianNotationOption ) {

aaron.ballman wrote:
> Can drop the `clang::` bit.
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:808-809
+IdentifierNamingCheck::HungarianNotationOption ) {
+  const auto ND = dyn_cast(D);
+  if (!ND) {
+return "";

aaron.ballman wrote:
> `const auto *` and elide braces (I'll stop pointing these out, can you do a 
> pass over the whole check?)
Sure! it's what 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:482
   Styles.emplace_back(IdentifierNamingCheck::NamingStyle{
-  std::move(CaseOptional), std::move(Prefix), std::move(Postfix)});
-else
+  std::move(CaseOptional), std::move(Prefix), std::move(Postfix),
+  std::move(HPOpt), HNOption});

Drive-by note that's not something you need to address: these calls to 
`std::move()` for prefix and postfix are rather suspect given that the 
parameters in the constructor are both `const std::string&`.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:483
+  std::move(CaseOptional), std::move(Prefix), std::move(Postfix),
+  std::move(HPOpt), HNOption});
+} else {

Would it make sense to declare `HNOption` as a typical local variable (no need 
for the `clearAll()` API since you can default construct properly), and pass an 
rvalue reference rather than an lvalue reference (so call std::move()) here, as 
with the other arguments)?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:549
+  for (const auto  : HNOption.CString) {
+auto Key = CStr.getKey().str();
+if (ModifiedTypeName.find(Key) == 0) {

You shouldn't use `auto` when the type isn't spelled out in the initialization. 
(Same below)



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:631
+
+  if (CRD->isUnion()) {
 return "";

Elide braces around single-line if statements, per our usual coding guidelines.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:635-636
 
-  if (clang::Decl::Kind::EnumConstant == Decl->getKind() ||
-  clang::Decl::Kind::CXXMethod == Decl->getKind()||
-  clang::Decl::Kind::Function == Decl->getKind()) {
+  if (CRD->isStruct()) {
+if (!isHungarianNotationOptionEnabled("TreatStructAsClass",
+  HNOption.General)) {

Combine into a single `if`?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:644
+  bool IsAbstractClass = false;
+  for (const auto *Method : CRD->methods()) {
+if (Method->isPure()) {

No need to do this manually, you can use `CXXRecordData::isAbstract()` since 
it's already computed.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:651-658
+  std::string Prefix;
+  if (IsAbstractClass) {
+  Prefix = "I";
+  } else {
+  Prefix = "C";
+  }
+

`return CRD->isAbstract() ? "I" : "C";`



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:662-666
+  std::string Name = ECD->getType().getAsString();
+  if (std::string::npos != Name.find("enum")) {
+Name = Name.substr(strlen("enum"), Name.length() - strlen("enum"));
+Name = Name.erase(0, Name.find_first_not_of(" "));
+  }

This is a bit worrying -- can you not look at the `DeclContext` for the 
`EnumConstantDecl` to find the parent `EnumDecl` and ask for its name directly?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:703
+
+static std::string getDeclTypeName(const clang::NamedDecl *ND) {
+  const auto VD = dyn_cast(ND);

You can drop the `clang::` bit.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:704
+static std::string getDeclTypeName(const clang::NamedDecl *ND) {
+  const auto VD = dyn_cast(ND);
+  if (!VD) {

When using type deduction, the coding style is to put all pointers and 
qualifiers on the declaration as well (so we don't have to infer them from 
context), so this should be `const auto *`.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:806
+static std::string getHungarianNotationPrefix(
+const clang::Decl *D,
+IdentifierNamingCheck::HungarianNotationOption ) {

Can drop the `clang::` bit.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:808-809
+IdentifierNamingCheck::HungarianNotationOption ) {
+  const auto ND = dyn_cast(D);
+  if (!ND) {
+return "";

`const auto *` and elide braces (I'll stop pointing these out, can you do a 
pass over the whole check?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Hi @aaron.ballman,

Thank you for the suggestion.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:237
+static void getHungarianNotationDefaultConfig(
+std::shared_ptr HNOption) {
+

aaron.ballman wrote:
> It seems like this function should take `HNOption` as a reference rather than 
> a `shared_ptr`.
OK!



Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:87
+HungarianPrefixOption HungarianPrefixOpt;
+std::shared_ptr
+HungarianNotationOption;

aaron.ballman wrote:
> I'd like to avoid using a `shared_ptr` here if we can avoid it -- do we 
> expect this to be super expensive to copy by value (I would imagine it'll be 
> an expensive copy but we don't make copies all that often, but maybe my 
> intuition is wrong)?
Good idea. A reference object is good enough. I will change it at next commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:237
+static void getHungarianNotationDefaultConfig(
+std::shared_ptr HNOption) {
+

It seems like this function should take `HNOption` as a reference rather than a 
`shared_ptr`.



Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:87
+HungarianPrefixOption HungarianPrefixOpt;
+std::shared_ptr
+HungarianNotationOption;

I'd like to avoid using a `shared_ptr` here if we can avoid it -- do we expect 
this to be super expensive to copy by value (I would imagine it'll be an 
expensive copy but we don't make copies all that often, but maybe my intuition 
is wrong)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2342016 , @njames93 wrote:

> In D86671#2341838 , @dougpuob wrote:
>
>> Hi @njames93,
>>
>> It's a smart idea, the rework for it is worth. There is a special case if 
>> lowercase name with Hungarian prefix, it possibly makes variable ambiguous, 
>> like the `Case1`. How about separating them and `aNy_CasE` with an 
>> underscore, like `Case2` ?
>>
>>   // Case1
>>   bool bRIGHT_LEVEL; // UPPER_CASE
>>   bool bRightLevel;  // CamelCase
>>   bool bRight_Level; // Camel_Snake_Case
>>   bool baNy_CasE;// aNy_CasE
>>   bool bright_level; // lower_case
>>   bool brightLevel;  // camelBack
>>   bool bright_Level; // camel_Snake_Back
>>   .^^ <-- right? bright?
>>   
>>   // Case2
>>   bool bRIGHT_LEVEL; // UPPER_CASE
>>   bool bRightLevel;  // CamelCase
>>   bool bRight_Level; // Camel_Snake_Case
>>   bool b_aNy_CasE;   // aNy_CasE
>>   bool b_right_level;// lower_case
>>   bool b_rightLevel; // camelBack
>>   bool b_right_Level;// camel_Snake_Back
>>   .^^^ <-- add an underscore
>
> That still has hidden surprises. Maybe instead of a bool, an enum is used for 
> controlling hungarian prefix (Off|On|...).
> Can't think of a good name for the third option but it would do the inserting 
> of '_' (bright_level ->b_right_level) or capitalising the first word of the 
> identifier (brightLevel -> bRightLevel).

Maybe it doesn't need a new name, how about (`Off|On|lower_case|camelBack`) or 
(`Off|On|sz_lower_case|szCamelBack`)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D86671#2341838 , @dougpuob wrote:

> Hi @njames93,
>
> It's a smart idea, the rework for it is worth. There is a special case if 
> lowercase name with Hungarian prefix, it possibly makes variable ambiguous, 
> like the `Case1`. How about separating them and `aNy_CasE` with an 
> underscore, like `Case2` ?
>
>   // Case1
>   bool bRIGHT_LEVEL; // UPPER_CASE
>   bool bRightLevel;  // CamelCase
>   bool bRight_Level; // Camel_Snake_Case
>   bool baNy_CasE;// aNy_CasE
>   bool bright_level; // lower_case
>   bool brightLevel;  // camelBack
>   bool bright_Level; // camel_Snake_Back
>   .^^ <-- right? bright?
>   
>   // Case2
>   bool bRIGHT_LEVEL; // UPPER_CASE
>   bool bRightLevel;  // CamelCase
>   bool bRight_Level; // Camel_Snake_Case
>   bool b_aNy_CasE;   // aNy_CasE
>   bool b_right_level;// lower_case
>   bool b_rightLevel; // camelBack
>   bool b_right_Level;// camel_Snake_Back
>   .^^^ <-- add an underscore

That still has hidden surprises. Maybe instead of a bool, an enum is used for 
controlling hungarian prefix (Off|On|...).
Can't think of a good name for the third option but it would do the inserting 
of '_' (bright_level ->b_right_level) or capitalising the first word of the 
identifier (brightLevel -> bRightLevel).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2330689 , @njames93 wrote:

> Is this diff been created incorrectly again?
>
> Taking a step back, Is Hungarian notation really a case style, Seems to me 
> its mainly about the prefix and a user may want `DWORD dwUPPER_CASE`, Right 
> now there is no way of adopting that.
> Maybe extend the options for hungarian type decls to
>
>   Case
>   Prefix
>   Suffix
>   HungarianPrefix
>
> `HungarianPrefix` would likely be a bool and if enabled, it would be 
> prepended to `Prefix`
> I could see a situation like this
>
>   [Options]
>   // VariableCase: UPPER_CASE
>   // VariablePrefix: PRE_
>   // VariableSuffix: _POST
>   // VariableHungarianPrefix: true
>   
>   DWORD MyVariable; -> DWORD dwPRE_MY_VARIABLE_POST;
>
> This would give users full control of exactly how they want their 
> declarations with no hidden surprises.
>
> Granted this approach would require a little rework but it would satisfy more 
> users.

Hi @njames93,

It's a smart idea, the rework for it is worth. There is a special case if 
lowercase name with Hungarian prefix, it possibly makes variable ambiguous, 
like the `Case1`. How about separating them and `aNy_CasE` with an underscore, 
like `Case2` ?

  // Case1
  bool bRIGHT_LEVEL; // UPPER_CASE
  bool bRightLevel;  // CamelCase
  bool bRight_Level; // Camel_Snake_Case
  bool baNy_CasE;// aNy_CasE
  bool bright_level; // lower_case
  bool brightLevel;  // camelBack
  bool bright_Level; // camel_Snake_Back
  .^^ <-- right? bright?
  
  // Case2
  bool bRIGHT_LEVEL; // UPPER_CASE
  bool bRightLevel;  // CamelCase
  bool bRight_Level; // Camel_Snake_Case
  bool b_aNy_CasE;   // aNy_CasE
  bool b_right_level;// lower_case
  bool b_rightLevel; // camelBack
  bool b_right_Level;// camel_Snake_Back
  .^^^ <-- add an underscore


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Is this diff been created incorrectly again?

Taking a step back, Is Hungarian notation really a case style, Seems to me its 
mainly about the prefix and a user may want `DWORD dwUPPER_CASE`, Right now 
there is no way of adopting that.
Maybe extend the options for hungarian type decls to

  Case
  Prefix
  Suffix
  HungarianPrefix

`HungarianPrefix` would likely be a bool and if enabled, it would be 
prepended to `Prefix`
I could see a situation like this

  [Options]
  // VariableCase: UPPER_CASE
  // VariablePrefix: PRE_
  // VariableSuffix: _POST
  // VariableHungarianPrefix: true
  
  DWORD MyVariable; -> DWORD dwPRE_MY_VARIABLE_POST;

This would give users full control of exactly how they want their declarations 
with no hidden surprises.

Granted this approach would require a little rework but it would satisfy more 
users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Hi @aaron.ballman and @njames93,
I addressed your code review suggestions and supported Hungarian Notation 
prefix for decl of enum and class(option) at latest patch. Unfortunately, I 
encountered a problem that new patch failed on remote BuildBot, it showed the 
"No such file or directory" error message when it was trying to call 
apply_patch2.py!_apply_diff(). Do you have any idea what is going on? Do you 
suggest I create a new Diff(new diff id) for it ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 297443.
dougpuob added a comment.

- Support to add Class prefix for Hungarian Notation.
- Support to add Enum prefix for Hungarian Notation.
- Support `unsigned long long`, `ULONG`, and `HANDLE` types and others.
- Support options for Hungarian Notation in config file.
- Added more test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation-cfgfile.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation-cfgfile.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation-cfgfile.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation-cfgfile.cpp
@@ -72,34 +72,34 @@
 // RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.long-int, value: li   }, \
 // RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.long, value: l}, \
 // RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.ptrdiff_t   , value: p}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.BOOL, value: b}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.BOOLEAN , value: b}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.BYTE, value: by   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.CHAR, value: c}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.UCHAR   , value: uc   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.SHORT   , value: s}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.USHORT  , value: us   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.WORD, value: w}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.DWORD   , value: dw   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.DWORD32 , value: dw32 }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.DWORD64 , value: dw64 }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.LONG, value: l}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.ULONG   , value: ul   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.ULONG32 , value: ul32 }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.ULONG64 , value: ul64 }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.ULONGLONG   , value: ull  }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.HANDLE  , value: h}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.INT , value: i}, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.INT8, value: i8   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.INT16   , value: i16  }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.INT32   , value: i32  }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.INT64   , value: i64  }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.UINT, value: ui   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.UINT8   , value: u8   }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.UINT16  , value: u16  }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.UINT32  , value: u32  }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.UINT64  , value: u64  }, \
-// RUN: { key: readability-identifier-naming.HungarianNotation.PrimitiveType.PVOID   , 

[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/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:25
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: 
CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: 
szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: 
szHungarianNotation }, \

aaron.ballman wrote:
> dougpuob wrote:
> > njames93 wrote:
> > > Class names shouldn't use hungarian notation.
> > OK~ I have classified CheckOptions, and all test cases one by one in the 
> > next diff.
> > 
> > ```
> > // RUN:   -config='{ CheckOptions: [ \
> > // RUN: { key: readability-identifier-naming.ClassMemberCase
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.ConstantCase   
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.ConstantMemberCase 
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.ConstantParameterCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: 
> > readability-identifier-naming.ConstantPointerParameterCase , value: 
> > szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.ConstexprVariableCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.GlobalConstantCase 
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.GlobalConstantPointerCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.GlobalVariableCase 
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.LocalConstantCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.LocalConstantPointerCase   
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.LocalPointerCase   
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.LocalVariableCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.MemberCase 
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.ParameterCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.PointerParameterCase   
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.PrivateMemberCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.StaticConstantCase 
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.StaticVariableCase 
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.StructCase 
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.UnionCase  
> >   , value: szHungarianNotation }, \
> > // RUN: { key: readability-identifier-naming.VariableCase   
> >   , value: szHungarianNotation }  \
> > // RUN:   ]}'
> > ```
> > Class names shouldn't use hungarian notation.
> 
> That may be debatable as I've definitely seen `C` used as a prefix for class 
> names and `I` used as a prefix for pure virtual class names (interfaces). 
> Doing a quick search on Google brings up evidence that this isn't uncommon.
Agree both of you because I saw them in different projects. I will add this 
feature as an option (default is off, user can enable it in .clang-tidy).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:25
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: 
CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: 
szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: 
szHungarianNotation }, \

dougpuob wrote:
> njames93 wrote:
> > Class names shouldn't use hungarian notation.
> OK~ I have classified CheckOptions, and all test cases one by one in the next 
> diff.
> 
> ```
> // RUN:   -config='{ CheckOptions: [ \
> // RUN: { key: readability-identifier-naming.ClassMemberCase  
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.ConstantCase 
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.ConstantMemberCase   
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.ConstantParameterCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.ConstantPointerParameterCase 
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.ConstexprVariableCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.GlobalConstantCase   
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.GlobalConstantPointerCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.GlobalVariableCase   
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.LocalConstantCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.LocalConstantPointerCase 
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.LocalPointerCase 
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.LocalVariableCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.MemberCase   
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.ParameterCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.PointerParameterCase 
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.PrivateMemberCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.StaticConstantCase   
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.StaticVariableCase   
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.StructCase   
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.UnionCase
> , value: szHungarianNotation }, \
> // RUN: { key: readability-identifier-naming.VariableCase 
> , value: szHungarianNotation }  \
> // RUN:   ]}'
> ```
> Class names shouldn't use hungarian notation.

That may be debatable as I've definitely seen `C` used as a prefix for class 
names and `I` used as a prefix for pure virtual class names (interfaces). Doing 
a quick search on Google brings up evidence that this isn't uncommon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2304337 , @njames93 wrote:

> Not strictly relevant here, but this does open up the idea of enforcing the 
> style where an enum constant is prefixed by the initials of the enum name.

I like this idea. There is a case when `EnumConstantPrefix` and 
`EnumConstantCase=szHungarianNotation` options are set, it may take similar 
affect(which will be the first) or be overwritten? I can have it a try later.

Showing my conception as the following:

  // [Before]
  typedef enum {
  RevValid = -1,
  RevNone  = 0, 
  RevCrlReason = 1, 
  RevHold  = 2, 
  RevKeyCompromise = 3, 
  RevCaCompromise  = 4  
  } REVINFO_TYPE;
  
  // [After]
  typedef enum {
  rtRevValid = -1
  rtRevNone  = 0,
  rtRevCrlReason = 1,
  rtRevHold  = 2,
  rtRevKeyCompromise = 3,
  rtRevCaCompromise  = 4 
  } REVINFO_TYPE;
  
  // [After] EnumConstantPrefix first case
  // EnumConstantCase=snHungarianNotation
  // EnumConstantPrefix=pre_
  typedef enum {
  pre_rtRevValid = -1
  pre_rtRevNone  = 0,
  pre_rtRevCrlReason = 1,
  pre_rtRevHold  = 2,
  pre_rtRevKeyCompromise = 3,
  pre_rtRevCaCompromise  = 4 
  } REVINFO_TYPE;




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:25
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: 
CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: 
szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: 
szHungarianNotation }, \

njames93 wrote:
> Class names shouldn't use hungarian notation.
OK~ I have classified CheckOptions, and all test cases one by one in the next 
diff.

```
// RUN:   -config='{ CheckOptions: [ \
// RUN: { key: readability-identifier-naming.ClassMemberCase  , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.ConstantCase , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.ConstantMemberCase   , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.ConstantParameterCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.ConstantPointerParameterCase , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.ConstexprVariableCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.GlobalConstantCase   , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.GlobalConstantPointerCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.GlobalVariableCase   , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.LocalConstantCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.LocalConstantPointerCase , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.LocalPointerCase , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.LocalVariableCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.MemberCase   , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.ParameterCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.PointerParameterCase , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.PrivateMemberCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.StaticConstantCase   , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.StaticVariableCase   , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.StructCase   , 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.UnionCase, 
value: szHungarianNotation }, \
// RUN: { key: readability-identifier-naming.VariableCase , 
value: szHungarianNotation }  \
// RUN:   ]}'
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Not strictly relevant here, but this does open up the idea of enforcing the 
style where an enum constant is prefixed by the initials of the enum name.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:25
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: 
CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: 
szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: 
szHungarianNotation }, \

Class names shouldn't use hungarian notation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2293128 , @aaron.ballman 
wrote:

> In D86671#2284078 , @dougpuob wrote:
>
>> Hi @aaron.ballman
>> About changing  `size_t nLength` to `cbLength`. I searched MSVC folders with 
>> `size_t`, many names of variable start with `n`, or `i` in MFC related 
>> files. So I prefer to keep it starts with `n`. Another side to name starts 
>> with `cb`, I found variables like cbXxx are defined with ULONG, DWORD, or 
>> UCHAR type.
>
> I think the important point is that `cb` is used for APIs that specify a 
> count of bytes, whereas `i`, `n`, and `l` are used for integers (there is no 
> specific prefix for `size_t` that I'm aware of or that MSDN documents, so the 
> lack of consistency there is not too surprising). That's why I mentioned that 
> using a heuristic approach may allow you to identify the parameters that are 
> intended to be a count of bytes so that you can use the more specific prefix 
> if there's more specific contextual information available.

Hi @aaron.ballman

1. **Making `sz` as a prefix to the `char*` parameters of `strncpy()` and 
`strncat()` functions.** I read the code of `strncpy` function, seems 
null-terminated to parameters is essential. I thought those parameters to 
strXxx() are also able to the parameter of `strlen` function knowing the length 
of a C string(it can be passed to be a parameter or assigned to be a variable, 
those point to the identical memory. strXxx functions tell the end by null 
character in the memory). Moreover, I searched source code of OpenSSL, I found 
the project uses most of `char*` as C string, and data buffer to retrieve data 
with `unsigned char*`.  `unsigned char` is a primitive type in C89 (is portable 
than `uint8_t`). Maybe current mechanism is a good way to hint users that 
`unsigned char*` is more explicit than `char*` if they want to treat it as a 
buffer to retrieve data.

2. **Adding heuristics to pick the correct prefix.** Do you mean that use the 
heuristics approach to give naming suggestion as warning message, or correct it 
with the `--fix` option? Is there any existing in this project can be a sample 
for me? If my thought about the heuristics is correct. The information of 
parameters to functions I can query its **type**, **name**, and **location**. 
As we have discussed that the declaration type is insufficient to tell `sz` for 
`char*`, or `cb` for count of bytes instead `i`, `n`, or `l`, so the **name** 
and **location** provide more information for comparing names with string 
mapping tables and location relationship between parameters/variables. Take 
`FILE * fopen ( const char * filename, const char * mode );` for example(C 
String). It will change `filename` to `szFilename` if the `name` string is in 
the mapping table, change `mode` to `pcMode` if `mode` string is not in the 
mapping table. Take `size_t fread ( void * ptr, size_t size, size_t count, FILE 
* stream );` for example(count of bytes). It will change `size` to `cbSize`, 
because its previous parameter is a void pointer.

  I can smell that there is always exceptional cases, and not good for 
maintainability.

3. **Changing `i`, `n`, and `l` to `cb` for APIs that specify a count of 
bytes.** I have no idea how to distinguish when should add the `cb` instead of 
integer types for heuristics. If I was the user, I wish clang-tidy don't change 
integer types from `cbXxx` to `iCbXxx` or `nCbXxx`. Keep `cb` with default or 
an option in config file, like `IgnoreParameterPrefix`, `IgnoreVariablePrefix`.

4. **Why not use Decl->getType()->getAsString()** I printed the differences as 
a log for your reference. 
(https://gist.github.com/dougpuob/e5a76db6e2c581ba003afec3236ab6ac#file-clang-tidy-readability-identifier-naming-hungarian-notation-log-L191)
 Previous I mentioned "if users haven't specific the include directories, the 
checking result may look messy". This concern will not happened because the 
`clang-diagnostic-error`, if users didn't specific header directories, 
clang-tidy will show the error (L414 
).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D86671#2284078 , @dougpuob wrote:

> Hi @aaron.ballman
> About changing  `size_t nLength` to `cbLength`. I searched MSVC folders with 
> `size_t`, many names of variable start with `n`, or `i` in MFC related files. 
> So I prefer to keep it starts with `n`. Another side to name starts with 
> `cb`, I found variables like cbXxx are defined with ULONG, DWORD, or UCHAR 
> type.

I think the important point is that `cb` is used for APIs that specify a count 
of bytes, whereas `i`, `n`, and `l` are used for integers (there is no specific 
prefix for `size_t` that I'm aware of or that MSDN documents, so the lack of 
consistency there is not too surprising). That's why I mentioned that using a 
heuristic approach may allow you to identify the parameters that are intended 
to be a count of bytes so that you can use the more specific prefix if there's 
more specific contextual information available.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:236
+  PrefixStr = "fn"; // Function Pointer
+} else if (QT->isPointerType()) {
+  // clang-format off

dougpuob wrote:
> aaron.ballman wrote:
> > I'm not certain how valid it is to look at just the type and decide that 
> > it's a null-terminated string. For instance, the following is not an 
> > uncommon pattern: `void something(const char *buffer, size_t length);` and 
> > it would be a bit strange to change that into `szBuffer` as that would give 
> > an indication that the buffer is null terminated. You could look at 
> > surrounding code for additional information though, like other parameters 
> > in a function declaration. As an aside, this sort of heuristic search may 
> > also allow you to change `length` into `cbLength` instead of `nLength` for 
> > conventions like the Microsoft one.
> > 
> > However, for local variables, I don't see how you could even come up with a 
> > heuristic like you could with parameters, so I'm not certain what the right 
> > answer is here.
> OK (size_t nLength --> cbLength)
> 
> About the `void something(const char *buffer, size_t length)` pattern. `char` 
> is a special type which can express signed char and unsigned char.  One can 
> express C string(ASCII), another expresses general data buffer(in hex). I 
> think you are mentioning when it is a general data buffer. I agree with you 
> if it changed the name from `buffer` to `szBuffer` for general data buffer is 
> strange. I prefer to use `uint8_t` or `unsigned char` instead.
> 
> How about adding a new config option for it? like, `  - { key: 
> readability-identifier-naming.HungarainNotation.DontChangeCharPointer   , 
> value: 1 }` Users can make decision for their projects. For consistency with 
> HN, the default will still be changed to `szBuffer` in your case.
> 
> If I add the option, does it make sense to you from your side?
I'm not certain that approach would make sense because the decision needs to be 
made on a case-by-case basis. Taking the C standard library as an example set 
of APIs, a function like `char *strncpy(char * restrict s1, const char * 
restrict s2, size_t n);` is one where this functionality should not change `s1` 
or `s2` to be `szS1` or `szS2` because the `sz` modifier indicates a 
null-terminated string, but these strings do not have to be null terminated. 
However, an API like `char *strncat(char * restrict s1, const char * restrict 
s2, size_t n);` should change `s1` to be `szS1` because that string is required 
to be null terminated. In both cases, the declarations use `char`.

`uint8_t` and friends are newer datatypes (added in C99) and are not required 
to be present (so they're not portable to all implementations), which may 
account for buffer-based APIs still using `char`.

I think adding some heuristics to the check may help some of these situations 
pick the correct prefix, but I'm worried that cases like the two above are 
going to be almost intractable for the check. I don't have a good idea of how 
prevalent that kind of issue will be, though. One thing that would help me feel 
more comfortable would be to look at some data about how the check performs on 
some various code bases. e.g., pick some large or popular libraries that use C, 
C++, or both (Win32 SDK, LLVM headers, OpenSSL, etc), run the check over it, 
and check the results to see how often the prefix is right compared to how 
often it's wrong. If we find it's mostly right and only gets it wrong a small 
percentage of the time, then we've likely found a good compromise for the 
check's behavior. If we find it gets it wrong too often, then the kinds of 
tweaks needed may be more obvious.

Btw, the reason I think it's important to be careful about false positives for 
this particular check is because Hungarian notation is intended to convey 
semantic information in some cases (like whether a buffer is null terminated or 
not) 

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Hi @aaron.ballman
About changing  `size_t nLength` to `cbLength`. I searched MSVC folders with 
`size_t`, many names of variable start with `n`, or `i` in MFC related files. 
So I prefer to keep it starts with `n`. Another side to name starts with `cb`, 
I found variables like cbXxx are defined with ULONG, DWORD, or UCHAR type.

Maybe make them as options in config (.clang-tidy) is more practical. The 
prefixes in the default table follows its abbreviation or acronym of data type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Replied comments by @aaron.ballman




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:216
+{"long","l"},
+{"long long",   "ll"},
+{"unsigned long",   "ul"},

aaron.ballman wrote:
> `unsigned long long` -> `ull`
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:225
+{"WORD","w"},
+{"DWORD",   "dw"}};
+  // clang-format on

aaron.ballman wrote:
> `ULONG` -> `ul`
> `HANDLE` -> `h`
> 
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:236
+  PrefixStr = "fn"; // Function Pointer
+} else if (QT->isPointerType()) {
+  // clang-format off

aaron.ballman wrote:
> I'm not certain how valid it is to look at just the type and decide that it's 
> a null-terminated string. For instance, the following is not an uncommon 
> pattern: `void something(const char *buffer, size_t length);` and it would be 
> a bit strange to change that into `szBuffer` as that would give an indication 
> that the buffer is null terminated. You could look at surrounding code for 
> additional information though, like other parameters in a function 
> declaration. As an aside, this sort of heuristic search may also allow you to 
> change `length` into `cbLength` instead of `nLength` for conventions like the 
> Microsoft one.
> 
> However, for local variables, I don't see how you could even come up with a 
> heuristic like you could with parameters, so I'm not certain what the right 
> answer is here.
OK (size_t nLength --> cbLength)

About the `void something(const char *buffer, size_t length)` pattern. `char` 
is a special type which can express signed char and unsigned char.  One can 
express C string(ASCII), another expresses general data buffer(in hex). I think 
you are mentioning when it is a general data buffer. I agree with you if it 
changed the name from `buffer` to `szBuffer` for general data buffer is 
strange. I prefer to use `uint8_t` or `unsigned char` instead.

How about adding a new config option for it? like, `  - { key: 
readability-identifier-naming.HungarainNotation.DontChangeCharPointer   , 
value: 1 }` Users can make decision for their projects. For consistency with 
HN, the default will still be changed to `szBuffer` in your case.

If I add the option, does it make sense to you from your side?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:309
+
+  if (PtrCount > 0) {
+for (size_t Idx = 0; Idx < PtrCount; Idx++) {

aaron.ballman wrote:
> No need for this `if` statement, the `for` loop won't run anyway if `PtrCount 
> == 0`.
OK! redundant code.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:319
+std::string
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+  const ValueDecl *ValDecl = dyn_cast(Decl);

aaron.ballman wrote:
> `ND` instead of `Decl`.
> 
> The function name doesn't really help me to understand why you'd call this as 
> opposed to getting the type information as a string from the `NamedDecl` 
> itself. I'm a bit worried about maintaining this code as the language evolves 
> -- Clang will get new keywords, and someone will have to remember to come 
> update this code. Could you get away with using 
> `Decl->getType()->getAsString()` and working with that rather than going back 
> to the original source code and trying to parse manually?
OK, I should do it. (ND instead of Decl.)

Use `Decl->getType()->getAsString()` is a good way. But HN is a strongly-typed 
notation, if users haven't specific the include directories, the checking 
result may look messy (it will be changed to unexpected type). So I parse a 
string with `SourceLocation`, just for user-friendly.

I understand your consideration, maintaining-friendly is also important. I can 
implement it with `Decl->getType()->getAsString()`, if my explanation can't 
convince you still. It is also fine to me. Or you think it just need a better 
appropriate function name?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:320
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+  const ValueDecl *ValDecl = dyn_cast(Decl);
+  if (!ValDecl) {

aaron.ballman wrote:
> `const auto *` since the type is spelled out in the initialization.
OK.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:554
+  case IdentifierNamingCheck::CT_HungarianNotation: {
+const NamedDecl *ND = dyn_cast(InputDecl);
+const std::string TypePrefix =

aaron.ballman wrote:
> `const auto *` because the type is spelled out in the initialization.
OK.

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:216
+{"long","l"},
+{"long long",   "ll"},
+{"unsigned long",   "ul"},

`unsigned long long` -> `ull`



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:225
+{"WORD","w"},
+{"DWORD",   "dw"}};
+  // clang-format on

`ULONG` -> `ul`
`HANDLE` -> `h`




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:236
+  PrefixStr = "fn"; // Function Pointer
+} else if (QT->isPointerType()) {
+  // clang-format off

I'm not certain how valid it is to look at just the type and decide that it's a 
null-terminated string. For instance, the following is not an uncommon pattern: 
`void something(const char *buffer, size_t length);` and it would be a bit 
strange to change that into `szBuffer` as that would give an indication that 
the buffer is null terminated. You could look at surrounding code for 
additional information though, like other parameters in a function declaration. 
As an aside, this sort of heuristic search may also allow you to change 
`length` into `cbLength` instead of `nLength` for conventions like the 
Microsoft one.

However, for local variables, I don't see how you could even come up with a 
heuristic like you could with parameters, so I'm not certain what the right 
answer is here.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:309
+
+  if (PtrCount > 0) {
+for (size_t Idx = 0; Idx < PtrCount; Idx++) {

No need for this `if` statement, the `for` loop won't run anyway if `PtrCount 
== 0`.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:319
+std::string
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+  const ValueDecl *ValDecl = dyn_cast(Decl);

`ND` instead of `Decl`.

The function name doesn't really help me to understand why you'd call this as 
opposed to getting the type information as a string from the `NamedDecl` 
itself. I'm a bit worried about maintaining this code as the language evolves 
-- Clang will get new keywords, and someone will have to remember to come 
update this code. Could you get away with using 
`Decl->getType()->getAsString()` and working with that rather than going back 
to the original source code and trying to parse manually?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:320
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+  const ValueDecl *ValDecl = dyn_cast(Decl);
+  if (!ValDecl) {

`const auto *` since the type is spelled out in the initialization.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:554
+  case IdentifierNamingCheck::CT_HungarianNotation: {
+const NamedDecl *ND = dyn_cast(InputDecl);
+const std::string TypePrefix =

`const auto *` because the type is spelled out in the initialization.



Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:39
 
+  std::string getDeclTypeName(const clang::NamedDecl *Decl) const;
   void storeOptions(ClangTidyOptions::OptionMap ) override;

Can you go with `ND` (or something else) instead of `Decl` since that's a type 
name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2259492 , @njames93 wrote:

> In D86671#2259443 , @dougpuob wrote:
>
>> In D86671#2259364 , @njames93 wrote:
>>
>>> Did you upload this incorrectly again, context is missing and seems to be a 
>>> relative diff from a previous version of this patch?
>>
>> Sorry for it, I think it's my bad. It is possible that I manually merged the 
>> last master(github) with changes then updated them both via web interface ...
>>
>> Can I fix it if switch back to the base (`14948a0`) then merge all my 
>> changes, then update the diff again via web interface? Or do you have any 
>> better suggestion?
>>
>> I am curious about how do you know this mistake? You got error messages with 
>> `arc patch D86671` ?
>
> The no context is easy to spot as phab says context not available. Its easy 
> to find knowing that there is no mention of hungarian notation in the trunk 
> version of IdentifierNamingCheck.cpp, yet there is mention of that in the 
> before diff.
>
> The way I do my patches is I create a branch from the current master. Then 
> all commits go into that branch. When its time to update the PR I can just do 
> a diff from  to .
> Though I do use arcanist for my patches
>
>   arc diff master
>
> arcanist will check to see if the current branch has tags for PR and 
> automatically update that PR. Otherwise it will create a new PR.
> If it goes to create a new PR instead of updating an existing one you can 
> pass update
>
>   arc diff master --update D86671

@njames93, thank you. I updated three updates with the way you told me, seems 
work fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 291371.
dougpuob added a comment.

Fixed crash on Windows when run regression test (llvm-lit for 
`readability-identifier-naming.cpp` file).

This is because over range parameter made ctor of std::string copying out of 
range memory from source. The over range parameter came from 
SourceManager::getCharacterData() function with the SourceLocation returning 
value of ValDecl->getLocation().

Find the terminated char insteads of calling `ValDecl->getLocation()` function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -0,0 +1,263 @@
+typedef signed char int8_t; // NOLINT
+typedef short   int16_t;// NOLINT
+typedef longint32_t;// NOLINT
+typedef long long   int64_t;// NOLINT
+typedef unsigned char   uint8_t;// NOLINT
+typedef unsigned short  uint16_t;   // NOLINT
+typedef unsigned long   uint32_t;   // NOLINT
+typedef unsigned long long  uint64_t;   // NOLINT
+#ifndef _WIN32
+typedef unsigned long long  size_t; // NOLINT
+#endif
+typedef longintptr_t;   // NOLINT
+typedef unsigned long   uintptr_t;  // NOLINT
+typedef long intptrdiff_t;  // NOLINT
+typedef unsigned char   BYTE;   // NOLINT
+typedef unsigned short  WORD;   // NOLINT
+typedef unsigned long   DWORD;  // NOLINT
+typedef int BOOL;   // NOLINT
+typedef BYTEBOOLEAN;// NOLINT
+#define NULL(0) // NOLINT
+
+// RUN: clang-tidy %s -checks=readability-identifier-naming \
+// RUN:   -config="{CheckOptions: [\
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.MemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ClassMemberCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ConstantMemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.VariableCase   , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ParameterCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalPointerCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalVariableCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalFunctionCase , value: CamelCase } \
+// RUN:   ]}"
+
+class UnlistedClass { public: mutable int ValInt; };
+// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: invalid case style for member 'ValInt' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}class UnlistedClass { public: mutable int iValInt; };
+
+UnlistedClass cUnlisted2;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'cUnlisted2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass Unlisted2;
+
+UnlistedClass objUnlistedClass3;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'objUnlistedClass3' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass UnlistedClass3;
+
+typedef int INDEX;
+INDEX iIndex = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'iIndex' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}INDEX Index = 0;
+
+struct DataBuffer {
+mutable size_t Size;
+};
+// CHECK-MESSAGES: :[[@LINE-2]]:20: warning: invalid case style for member 'Size' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}mutable size_t nSize;
+
+int  = iIndex;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for global variable 'RefValueIndex' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int  = Index;
+
+typedef void (*FUNC_PTR_HELLO)();
+FUNC_PTR_HELLO Hello = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global pointer 'Hello' [readability-identifier-naming]
+// CHECK-FIXES: 

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 290726.
dougpuob added a comment.

- Fixed lint warnings and regression test failures on Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -0,0 +1,261 @@
+typedef signed char int8_t;  // NOLINT
+typedef short int16_t;   // NOLINT
+typedef long int32_t;// NOLINT
+typedef long long int64_t;   // NOLINT
+typedef unsigned char uint8_t;   // NOLINT
+typedef unsigned short uint16_t; // NOLINT
+typedef unsigned long uint32_t;  // NOLINT
+typedef unsigned long long uint64_t; // NOLINT
+typedef unsigned long long size_t;   // NOLINT
+typedef long intptr_t;   // NOLINT
+typedef unsigned long uintptr_t; // NOLINT
+typedef long int ptrdiff_t;  // NOLINT
+typedef unsigned char BYTE;  // NOLINT
+typedef unsigned short WORD; // NOLINT
+typedef unsigned long DWORD; // NOLINT
+typedef int BOOL;// NOLINT
+typedef BYTE BOOLEAN;// NOLINT
+#define NULL (0) // NOLINT
+
+// RUN: clang-tidy %s -checks=readability-identifier-naming \
+// RUN:   -config="{CheckOptions: [\
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.MemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ClassMemberCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ConstantMemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.VariableCase   , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ParameterCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalPointerCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalVariableCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalFunctionCase , value: CamelCase } \
+// RUN:   ]}"
+
+class UnlistedClass { public: mutable int ValInt; };
+// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: invalid case style for member 'ValInt' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}class UnlistedClass { public: mutable int iValInt; };
+
+UnlistedClass cUnlisted2;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'cUnlisted2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass Unlisted2;
+
+UnlistedClass objUnlistedClass3;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'objUnlistedClass3' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass UnlistedClass3;
+
+typedef int INDEX;
+INDEX iIndex = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'iIndex' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}INDEX Index = 0;
+
+struct DataBuffer {
+mutable size_t Size;
+};
+// CHECK-MESSAGES: :[[@LINE-2]]:20: warning: invalid case style for member 'Size' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}mutable size_t nSize;
+
+int  = iIndex;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for global variable 'RefValueIndex' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int  = Index;
+
+typedef void (*FUNC_PTR_HELLO)();
+FUNC_PTR_HELLO Hello = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global pointer 'Hello' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}FUNC_PTR_HELLO fnHello = NULL;
+
+void *ValueVoidPtr = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global pointer 'ValueVoidPtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void *pValueVoidPtr = NULL;
+
+ptrdiff_t PtrDiff = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: invalid case style for global variable 'PtrDiff' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}ptrdiff_t pPtrDiff = NULL;
+
+const char 

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 290483.
dougpuob added a comment.

This is a test with `arc diff master --update D86671` command.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -0,0 +1,266 @@
+typedef signed char int8_t; // NOLINT
+typedef short   int16_t;// NOLINT
+typedef longint32_t;// NOLINT
+typedef long long   int64_t;// NOLINT
+
+typedef unsigned char   uint8_t;// NOLINT
+typedef unsigned short  uint16_t;   // NOLINT
+typedef unsigned long   uint32_t;   // NOLINT
+typedef unsigned long long  uint64_t;   // NOLINT
+
+typedef unsigned intsize_t; // NOLINT
+typedef longintptr_t;   // NOLINT
+typedef unsigned long   uintptr_t;  // NOLINT
+typedef long intptrdiff_t;  // NOLINT
+
+typedef unsigned char   BYTE;   // NOLINT
+typedef unsigned short  WORD;   // NOLINT
+typedef unsigned long   DWORD;  // NOLINT
+
+typedef int BOOL;   // NOLINT
+typedef BYTEBOOLEAN;// NOLINT
+
+#define NULL(0) // NOLINT
+
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.MemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ClassMemberCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ConstantMemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.VariableCase   , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ParameterCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalPointerCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalVariableCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalFunctionCase , value: CamelCase }, \
+// RUN:   ]}"
+
+class UnlistedClass { public: mutable int ValInt; };
+// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: invalid case style for member 'ValInt' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}class UnlistedClass { public: mutable int iValInt; };
+
+UnlistedClass cUnlisted2;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'cUnlisted2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass Unlisted2;
+
+UnlistedClass objUnlistedClass3;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'objUnlistedClass3' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass UnlistedClass3;
+
+typedef int INDEX;
+INDEX iIndex = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global variable 'iIndex' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}INDEX Index = 0;
+
+struct DataBuffer {
+mutable size_t Size;
+};
+// CHECK-MESSAGES: :[[@LINE-2]]:20: warning: invalid case style for member 'Size' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}mutable size_t nSize;
+
+int  = iIndex;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for global variable 'RefValueIndex' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int  = Index;
+
+typedef void (*FUNC_PTR_HELLO)();
+FUNC_PTR_HELLO Hello = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global pointer 'Hello' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}FUNC_PTR_HELLO fnHello = NULL;
+
+void *ValueVoidPtr = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for global pointer 'ValueVoidPtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void *pValueVoidPtr = NULL;
+
+ptrdiff_t PtrDiff = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: invalid case style for global variable 'PtrDiff' 

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D86671#2259443 , @dougpuob wrote:

> In D86671#2259364 , @njames93 wrote:
>
>> Did you upload this incorrectly again, context is missing and seems to be a 
>> relative diff from a previous version of this patch?
>
> Sorry for it, I think it's my bad. It is possible that I manually merged the 
> last master(github) with changes then updated them both via web interface ...
>
> Can I fix it if switch back to the base (`14948a0`) then merge all my 
> changes, then update the diff again via web interface? Or do you have any 
> better suggestion?
>
> I am curious about how do you know this mistake? You got error messages with 
> `arc patch D86671` ?

The no context is easy to spot as phab says context not available. Its easy to 
find knowing that there is no mention of hungarian notation in the trunk 
version of IdentifierNamingCheck.cpp, yet there is mention of that in the 
before diff.

The way I do my patches is I create a branch from the current master. Then all 
commits go into that branch. When its time to update the PR I can just do a 
diff from  to .
Though I do use arcanist for my patches

  arc diff master

arcanist will check to see if the current branch has tags for PR and 
automatically update that PR. Otherwise it will create a new PR.
If it goes to create a new PR instead of updating an existing one you can pass 
update

  arc diff master --update D86671


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2259364 , @njames93 wrote:

> Did you upload this incorrectly again, context is missing and seems to be a 
> relative diff from a previous version of this patch?

Sorry for it, I think it's my bad. It is possible that I manually merged the 
last master(github) with changes then updated them both via web interface ...

Can I fix it if switch back to the base (`14948a0`) then merge all my changes, 
then update the diff again via web interface? Or do you have any better 
suggestion?

I am curious about how do you know this mistake? You got error messages with 
`arc patch D86671` ?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Did you upload this incorrectly again, context is missing and seems to be a 
relative diff from a previous version of this patch?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 290099.
dougpuob added a comment.

Improved suggestions of code review. (All suggestions from aaron.ballman)

1. Changed variables named with `Decl` to `InputDecl`.
2. Changed `TypeName.length()` --> `!TypeName.empty()`.
3. Added partial Microsft data types to the `HungarianNotationTable`.
4. Added `long long`, `long double`, `ptrdiff_t` to the 
`HungarianNotationTable`.
5. Added `char8_t`, `char16_t`, `char32_t` to the `HungarianNotationTable`. 
Variables name with `char8_t*` type start with `pc8`, Eg. `char8_t *pc8Value = 
0;`
6. Supported name of function pointer start with `fn`.
7. Supported to remove the `restrict` keyword in 
IdentifierNamingCheck::getDeclTypeName().
8. Removed `const_cast` from Keywords list.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -1,120 +1,266 @@
-#include 
-#include 
+typedef signed char int8_t; // NOLINT
+typedef short   int16_t;// NOLINT
+typedef longint32_t;// NOLINT
+typedef long long   int64_t;// NOLINT
+
+typedef unsigned char   uint8_t;// NOLINT
+typedef unsigned short  uint16_t;   // NOLINT
+typedef unsigned long   uint32_t;   // NOLINT
+typedef unsigned long long  uint64_t;   // NOLINT
+
+typedef unsigned intsize_t; // NOLINT
+typedef longintptr_t;   // NOLINT
+typedef unsigned long   uintptr_t;  // NOLINT
+typedef long intptrdiff_t;  // NOLINT
+
+typedef unsigned char   BYTE;   // NOLINT
+typedef unsigned short  WORD;   // NOLINT
+typedef unsigned long   DWORD;  // NOLINT
+
+typedef int BOOL;   // NOLINT
+typedef BYTEBOOLEAN;// NOLINT
+
+#define NULL(0) // NOLINT
 
 // RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
 // RUN:   -config="{CheckOptions: [\
-// RUN:   {key: readability-identifier-naming.VariableCase, value: szHungarianNotation}, \
+// RUN: {key: readability-identifier-naming.FunctionCase   , value: CamelCase },   \
+// RUN: {key: readability-identifier-naming.ClassCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.TypedefCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.MemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ClassMemberCase, value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ConstantMemberCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.VariableCase   , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.ParameterCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalPointerCase  , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalVariableCase , value: szHungarianNotation }, \
+// RUN: {key: readability-identifier-naming.GlobalFunctionCase , value: CamelCase }, \
 // RUN:   ]}"
 
-class UnlistedClass {};
-UnlistedClass Unlisted1;
-// CHECK-NOT: :[[@LINE-2]]
+class UnlistedClass { public: mutable int ValInt; };
+// CHECK-MESSAGES: :[[@LINE-1]]:43: warning: invalid case style for member 'ValInt' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}class UnlistedClass { public: mutable int iValInt; };
 
 UnlistedClass cUnlisted2;
-// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for variable 'cUnlisted2' [readability-identifier-naming]
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'cUnlisted2' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}UnlistedClass Unlisted2;
 
 UnlistedClass objUnlistedClass3;
-// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for variable 'objUnlistedClass3' [readability-identifier-naming]
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for global variable 'objUnlistedClass3' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}UnlistedClass UnlistedClass3;
 
 typedef int INDEX;
 INDEX iIndex = 0;
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'iIndex' [readability-identifier-naming]

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:208
+{"wchar_t", "wc"},
+{"short",   "s"},
+{"signed",  "i"},

dougpuob wrote:
> aaron.ballman wrote:
> > It's been a long while since I thought about Hungarian notation, but I 
> > thought this was prefixed with `w` because it's word-sized (and `dw` for 
> > double word size)?
> > 
> > FWIW: 
> > https://docs.microsoft.com/en-us/windows/win32/stg/coding-style-conventions
> It is a good question.
> 
> Microsoft redefines them, so I would like to add them as part of mapping 
> table, `HungarianNotationTable`. That means the map include primitive types 
> and partial windows data types. 
> 
> ```
> // Windows Data Type
> {"BOOL","b"},   // typedef int BOOL;
> {"BOOLEAN", "b"},   // typedef BYTE BOOLEAN;
> {"BYTE","by"},  // typedef unsigned char BYTE;
> {"WORD","w"},   // typedef unsigned short WORD;
> {"DWORD",   "dw"},  // typedef unsigned long DWORD;
> ```
> 
> The result will like the following,
> ```
> WORD wVid = 0x8086;
> DWORD dwVidDid = 0x80861234;
> ```
I think this would be user-friendly behavior, though I'm curious if we'll run 
into any conflicting notations this way.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:479
+const NamedDecl *pNamedDecl = dyn_cast(pDecl);
+const std::string TypePrefix =
+getHungarianNotationTypePrefix(Type.str(), pNamedDecl);

dougpuob wrote:
> aaron.ballman wrote:
> > `pNamedDecl` can be null, which could crash.
> Make sense. If it was you, will you check it at the caller or in the callee? 
> and could I know the reason?
I'd handle it in `getHungarianNotationTypePrefix()` along with the check for an 
empty type name as both seem like they're checking for a degenerate case.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:485
+  if (Idx == 0) {
+const bool LowerAlnum =
+std::all_of(Words[Idx].begin(), Words[Idx].end(),

dougpuob wrote:
> aaron.ballman wrote:
> > FWIW, we don't typically use top-level `const` on value types.
> OK. But I am curious about it. Is it a rule in this project? or it is a flaw?
Not really a flaw (there's nothing wrong with top-level `const` on value 
types), but sort of a rule. Our coding style basically says to try to match the 
local styles used by the project. We've never really done top-level `const` 
anywhere else in the project, so any time someone introduces it, I usually ask 
for it to be removed. Also, as much as I love `const` correctness, many of our 
APIs are not super `const` correct (we're getting much better though), so it 
can also make code somewhat awkward when you hit an older API that accepts 
something non-`const`.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:21
+ - ``aNy_CasE``,
+ - ``szHungarianNotation``.
 

dougpuob wrote:
> aaron.ballman wrote:
> > We should probably document what prefixes we use for Hungarian notation, 
> > since there may be some alternatives in the wild. (It's not clear to me 
> > whether we should make the prefixes configurable or not.)
> Agree with you consideration.
> 
> 1. I will add more detail about the table, `HungarianNotationTable` .
> 
> 2. I would like treat it as a default table for this moment. Because I don't 
> know does it make sense that make user can customized it in `.clang-tidy` 
> file. The priority in config is higher than default table.  Show my idea as 
> the following,
> 
> ```
> Checks: '-*,readability-identifier-naming'
> CheckOptions:
>   - { key: readability-identifier-naming.VariableCase   , 
> value: szHungarianNotion }
>   - { key: readability-identifier-naming.HungarianWordList.uint8, 
> value: u8}
>   - { key: readability-identifier-naming.HungarianWordList.uint16   , 
> value: u16   }
>   - { key: readability-identifier-naming.HungarianWordList.uint32   , 
> value: u32   }
>   - { key: readability-identifier-naming.HungarianWordList.uint64   , 
> value: u64   }
>   - { key: readability-identifier-naming.HungarianWordList.unsigned_long, 
> value: ul}
>   - { key: readability-identifier-naming.HungarianWordList.long_long, 
> value: ll}
> ```
> 
> How about it?
I think allowing the prefixes to be configurable is a good idea and I agree 
that we should have a sensible default table (what you've proposed so far seems 
like a reasonable default to me). One thing we should be sure we support (and 
have tests for) is the case where the user wants to use the default table but 
change only one prefix from it. e.g., they like the defaults but change 

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Please no worry to give me your suggestions and feedback.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:185
+getHungarianNotationTypePrefix(const std::string ,
+   const NamedDecl *Decl) {
+  if (0 == TypeName.length()) {

aaron.ballman wrote:
> Please don't use `Decl` as the identifier since it mirrors the name of a type.
OK~ I will fix it. Thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:186
+   const NamedDecl *Decl) {
+  if (0 == TypeName.length()) {
+return TypeName;

aaron.ballman wrote:
> `if (TypeName.empty())`
OK~ I will fix it. Thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:208
+{"wchar_t", "wc"},
+{"short",   "s"},
+{"signed",  "i"},

aaron.ballman wrote:
> It's been a long while since I thought about Hungarian notation, but I 
> thought this was prefixed with `w` because it's word-sized (and `dw` for 
> double word size)?
> 
> FWIW: 
> https://docs.microsoft.com/en-us/windows/win32/stg/coding-style-conventions
It is a good question.

Microsoft redefines them, so I would like to add them as part of mapping table, 
`HungarianNotationTable`. That means the map include primitive types and 
partial windows data types. 

```
// Windows Data Type
{"BOOL","b"},   // typedef int BOOL;
{"BOOLEAN", "b"},   // typedef BYTE BOOLEAN;
{"BYTE","by"},  // typedef unsigned char BYTE;
{"WORD","w"},   // typedef unsigned short WORD;
{"DWORD",   "dw"},  // typedef unsigned long DWORD;
```

The result will like the following,
```
WORD wVid = 0x8086;
DWORD dwVidDid = 0x80861234;
```



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:211
+{"unsigned","u"},
+{"long","l"}};
+  // clang-format on

aaron.ballman wrote:
> How about: `long long`, `long double`, `ptrdiff_t`, and `void` (for `void *` 
> parameters)?
OK~ I will add them. Thank you. The `void*` and `ptrdiff_t` starts with `p`.







Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:222
+  // clang-format off
+  const static llvm::StringMap NullString = {
+{"char*", "sz"},

aaron.ballman wrote:
> Are there special prefixes for `char8_t *`, `char16_t *`, or `char32_t *`?
I will add them too. Thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:288
+  }
+
+  return PrefixStr;

aaron.ballman wrote:
> How about function pointers?
Nice consideration, thank you. I will add the feature. function pointers will 
start with `fn`.

I will add a test like this,
```
typedef void (*FUNC_PTR_HELLO)();
FUNC_PTR_HELLO Hello = NULL;
// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for variable 
'Hello' [readability-identifier-naming]
// CHECK-FIXES: {{^}}FUNC_PTR_HELLO fnHello = NULL;
```



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:310
+// Qualifier
+"const", "volatile",
+// Storage class specifiers

aaron.ballman wrote:
> `restrict`?
I will check it again. Thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:314
+// Constexpr specifiers
+"constexpr", "constinit", "const_cast", "consteval"};
+

aaron.ballman wrote:
> `const_cast` is not a constexpr specifier?
I will check it again. Thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:384
+static std::string fixupWithCase(const StringRef , const StringRef ,
+ const Decl *pDecl,
  IdentifierNamingCheck::CaseType Case) {

aaron.ballman wrote:
> `pDecl` doesn't match our usual conventions. ;-)
Oops! I will fix it later.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:478
+  case IdentifierNamingCheck::CT_HungarianNotation: {
+const NamedDecl *pNamedDecl = dyn_cast(pDecl);
+const std::string TypePrefix =

aaron.ballman wrote:
> Same here.
I will fix too.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:479
+const NamedDecl *pNamedDecl = dyn_cast(pDecl);
+const std::string TypePrefix =
+getHungarianNotationTypePrefix(Type.str(), pNamedDecl);

aaron.ballman wrote:
> `pNamedDecl` can be null, which could crash.
Make sense. If it was you, will you check it at the caller or in the callee? 
and could 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:185
+getHungarianNotationTypePrefix(const std::string ,
+   const NamedDecl *Decl) {
+  if (0 == TypeName.length()) {

Please don't use `Decl` as the identifier since it mirrors the name of a type.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:186
+   const NamedDecl *Decl) {
+  if (0 == TypeName.length()) {
+return TypeName;

`if (TypeName.empty())`



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:208
+{"wchar_t", "wc"},
+{"short",   "s"},
+{"signed",  "i"},

It's been a long while since I thought about Hungarian notation, but I thought 
this was prefixed with `w` because it's word-sized (and `dw` for double word 
size)?

FWIW: 
https://docs.microsoft.com/en-us/windows/win32/stg/coding-style-conventions



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:211
+{"unsigned","u"},
+{"long","l"}};
+  // clang-format on

How about: `long long`, `long double`, `ptrdiff_t`, and `void` (for `void *` 
parameters)?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:222
+  // clang-format off
+  const static llvm::StringMap NullString = {
+{"char*", "sz"},

Are there special prefixes for `char8_t *`, `char16_t *`, or `char32_t *`?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:288
+  }
+
+  return PrefixStr;

How about function pointers?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:310
+// Qualifier
+"const", "volatile",
+// Storage class specifiers

`restrict`?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:314
+// Constexpr specifiers
+"constexpr", "constinit", "const_cast", "consteval"};
+

`const_cast` is not a constexpr specifier?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:384
+static std::string fixupWithCase(const StringRef , const StringRef ,
+ const Decl *pDecl,
  IdentifierNamingCheck::CaseType Case) {

`pDecl` doesn't match our usual conventions. ;-)



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:478
+  case IdentifierNamingCheck::CT_HungarianNotation: {
+const NamedDecl *pNamedDecl = dyn_cast(pDecl);
+const std::string TypePrefix =

Same here.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:479
+const NamedDecl *pNamedDecl = dyn_cast(pDecl);
+const std::string TypePrefix =
+getHungarianNotationTypePrefix(Type.str(), pNamedDecl);

`pNamedDecl` can be null, which could crash.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:483
+for (size_t Idx = 0; Idx < Words.size(); Idx++) {
+  // Skip first part if it's a lowercase string
+  if (Idx == 0) {

Missing a full stop at the end of the comment.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:485
+  if (Idx == 0) {
+const bool LowerAlnum =
+std::all_of(Words[Idx].begin(), Words[Idx].end(),

FWIW, we don't typically use top-level `const` on value types.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:563
+   const IdentifierNamingCheck::NamingStyle ,
+   const Decl *Decl) {
   const std::string Fixed = fixupWithCase(

Similar naming issue here with `Decl` (elsewhere as well, I'll stop bringing 
this one up).



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:21
+ - ``aNy_CasE``,
+ - ``szHungarianNotation``.
 

We should probably document what prefixes we use for Hungarian notation, since 
there may be some alternatives in the wild. (It's not clear to me whether we 
should make the prefixes configurable or not.)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:1
+#include 
+#include 

Test cases should not include system headers (that makes the test sensitive to 
the system which is a problem). You should lift the declarations you need out 
of 

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 288938.
dougpuob added a comment.

Improved suggestions of code review.

1. Moved release notes to right place. [Eugene.Zelenko]
2. Added new casting types to doc(readability-identifier-naming.rst) 
[Eugene.Zelenko]
3. Moved partial code to a new function, 
IdentifierNamingCheck::getDeclTypeName(). [njames93]


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -0,0 +1,120 @@
+#include 
+#include 
+
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: readability-identifier-naming.VariableCase, value: szHungarianNotation}, \
+// RUN:   ]}"
+
+class UnlistedClass {};
+UnlistedClass Unlisted1;
+// CHECK-NOT: :[[@LINE-2]]
+
+UnlistedClass cUnlisted2;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for variable 'cUnlisted2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass Unlisted2;
+
+UnlistedClass objUnlistedClass3;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for variable 'objUnlistedClass3' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}UnlistedClass UnlistedClass3;
+
+typedef int INDEX;
+INDEX iIndex = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'iIndex' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}INDEX Index = 0;
+
+const char *NamePtr = "Name";
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for variable 'NamePtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}const char *szNamePtr = "Name";
+
+const char NameArray[] = "Name";
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: invalid case style for variable 'NameArray' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}const char szNameArray[] = "Name";
+
+void *BufferPtr1 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'BufferPtr1' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void *pBufferPtr1 = NULL;
+
+void **BufferPtr2 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'BufferPtr2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void **ppBufferPtr2 = NULL;
+
+void **pBufferPtr3 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'pBufferPtr3' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void **ppBufferPtr3 = NULL;
+
+int8_t ValueI8 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'ValueI8' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int8_t i8ValueI8 = 0;
+
+int16_t ValueI16 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI16' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int16_t i16ValueI16 = 0;
+
+int32_t ValueI32 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI32' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int32_t i32ValueI32 = 0;
+
+int64_t ValueI64 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI64' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int64_t i64ValueI64 = 0;
+
+uint8_t ValueU8 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueU8' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint8_t u8ValueU8 = 0;
+
+uint16_t ValueU16 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU16' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint16_t u16ValueU16 = 0;
+
+uint32_t ValueU32 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU32' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint32_t u32ValueU32 = 0;
+
+uint64_t ValueU64 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU64' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint64_t u64ValueU64 = 0;
+
+float ValueFloat = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'ValueFloat' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}float fValueFloat = 0;
+
+double ValueDouble = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'ValueDouble' [readability-identifier-naming]
+// 

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2246570 , @njames93 wrote:

> As Hungarian notation enforces prefixes as well as casing styles it would be 
> wise to warn on cases where the style is Hungarian but a prefix has also been 
> set.
>
> Another issue is this current set up will let you define any decls style as 
> hungarian, this doesn't make sense for most decl types. 
> Potentially some validation should happen for that too,

Hi @njames93:
If decl types were not in the `HungarianNotationTable` table, current 
implementation will treat it as `CamelCase`(UpperCamel), because the prefix is 
empty.




Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h:127
   virtual llvm::Optional
-  GetDeclFailureInfo(const NamedDecl *Decl, const SourceManager ) const = 0;
+  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+ const SourceManager ) const = 0;

njames93 wrote:
> I'm not a fan of changing the base class for this support. The Type paramater 
> can be inferred using the Decl. 
> You can `dyn_cast` the Decl to a `ValueDecl` and then get its type from that.
Agree with you. This change is possible to make it without changing the 
interface of function. Improve it in next patch. Thank you.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:73
 
+- Added: Support variables could be checked with Hungarian Notation which the
+  prefix encodes the actual data type of the variable.

Eugene.Zelenko wrote:
> dougpuob wrote:
> > Eugene.Zelenko wrote:
> > > See examples for entry format below.
> > Make it like the following ?
> > 
> > ```
> > Changes in existing checks
> > ^^
> > 
> > - Improved :doc:`readability-identifier-naming
> >   ` check.
> > 
> >   Added an option `GetConfigPerFile` to support including files which use
> >   different naming styles.
> > 
> > - Improved :doc:`readability-identifier-naming
> >   
> > ` 
> > check.  
> > 
> >   Support variables could be checked with Hungarian Notation which the 
> > prefix
> >   encodes the actual data type of the variable.
> > ```
> Yes, but link must be `clang-tidy/checks/readability-identifier-naming`, 
> because it refer to documentation file, not regression test.
 OK~ Fix it in the next patch. Thank you.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

As Hungarian notation enforces prefixes as well as casing styles it would be 
wise to warn on cases where the style is Hungarian but a prefix has also been 
set.

Another issue is this current set up will let you define any decls style as 
hungarian, this doesn't make sense for most decl types. 
Potentially some validation should happen for that too,




Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h:127
   virtual llvm::Optional
-  GetDeclFailureInfo(const NamedDecl *Decl, const SourceManager ) const = 0;
+  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+ const SourceManager ) const = 0;

I'm not a fan of changing the base class for this support. The Type paramater 
can be inferred using the Decl. 
You can `dyn_cast` the Decl to a `ValueDecl` and then get its type from that.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D86671#2246486 , @dougpuob wrote:

> In D86671#2246480 , @Eugene.Zelenko 
> wrote:
>
>> By the word, you must mention new option in documentation too.
>
> Hi @Eugene.Zelenko,
> Is the `clang-tools-extra/docs/ReleaseNotes.rst` file that you mentioned?
>
> If YES, seems pretty simple with one line at the end for the new casing type, 
> like:
>
>   Casing types include:
>   
>- ``lower_case``,
>- ``UPPER_CASE``,
>- ``camelBack``,
>- ``CamelCase``,
>- ``camel_Snake_Back``,
>- ``Camel_Snake_Case``,
>- ``aNy_CasE``,
>- ``szHungarianNotation``. 
>
> Is it right? and any more about document I have to do?

Yes, you are correct.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2246480 , @Eugene.Zelenko 
wrote:

> By the word, you must mention new option in documentation too.

Hi @Eugene.Zelenko,
Is the `clang-tools-extra/docs/ReleaseNotes.rst` file that you mentioned?

If YES, seems pretty simple with one line at the end for the new casing type, 
like:

  Casing types include:
  
   - ``lower_case``,
   - ``UPPER_CASE``,
   - ``camelBack``,
   - ``CamelCase``,
   - ``camel_Snake_Back``,
   - ``Camel_Snake_Case``,
   - ``aNy_CasE``,
   - ``szHungarianNotation``. 

Is it right? and any more about document I have to do?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

By the word, you must mention new option in documentation too.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:73
 
+- Added: Support variables could be checked with Hungarian Notation which the
+  prefix encodes the actual data type of the variable.

dougpuob wrote:
> Eugene.Zelenko wrote:
> > See examples for entry format below.
> Make it like the following ?
> 
> ```
> Changes in existing checks
> ^^
> 
> - Improved :doc:`readability-identifier-naming
>   ` check.
> 
>   Added an option `GetConfigPerFile` to support including files which use
>   different naming styles.
> 
> - Improved :doc:`readability-identifier-naming
>   ` 
> check.  
> 
>   Support variables could be checked with Hungarian Notation which the prefix
>   encodes the actual data type of the variable.
> ```
Yes, but link must be `clang-tidy/checks/readability-identifier-naming`, 
because it refer to documentation file, not regression test.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob marked an inline comment as done.
dougpuob added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:73
 
+- Added: Support variables could be checked with Hungarian Notation which the
+  prefix encodes the actual data type of the variable.

Eugene.Zelenko wrote:
> See examples for entry format below.
Make it like the following ?

```
Changes in existing checks
^^

- Improved :doc:`readability-identifier-naming
  ` check.

  Added an option `GetConfigPerFile` to support including files which use
  different naming styles.

- Improved :doc:`readability-identifier-naming
  ` 
check.  

  Support variables could be checked with Hungarian Notation which the prefix
  encodes the actual data type of the variable.
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:73
 
+- Added: Support variables could be checked with Hungarian Notation which the
+  prefix encodes the actual data type of the variable.

See examples for entry format below.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob marked 8 inline comments as done.
dougpuob added a comment.

I fixed all review suggestions from @aaron.ballman, @Eugene.Zelenko, and 
@njames93, Thank you. But I still can't sure I did everything correct.

I encountered a problem when I use `arc diff` or `arc diff --update D86671`, 
Arcanist will create a new Review(D86838 ) to 
Phabricator. This makes me to update the diff on the web interface.

Another behavior I can't sure do I did it right. I will pull the latest master 
then create a new local branch(my-new-branch) then manually merge the diff from 
previous branch(my-prev-branch). After this I use `git diff HEAD~1 -U99 > 
mypatch.patch`, then update the diff, `mypatch.patch` file to Phabricator. Is 
it right? Or is there a smarter way to do it?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 288812.
dougpuob added a comment.
Herald added a subscriber: danielkiss.

Improved for suggestions of code review.

1. Moved release notes to `Changes in existing checks` section. Eugene.Zelenko]
2. Modified release notes can be describe user-facing. [Eugene.Zelenko]
3. Added newline at the end to 
`readability-identifier-naming-hungarain-notion.cpp` file. [Eugene.Zelenko]
4. Removed unrelated change. [Nathan James]
5. Renamed getDeclFailureInfo() --> GetDeclFailureInfo() for local consistency. 
[Aaron Ballman]


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -0,0 +1,103 @@
+#include 
+#include 
+
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: readability-identifier-naming.VariableCase, value: szHungarianNotation}, \
+// RUN:   ]}"
+
+const char *NamePtr = "Name";
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for variable 'NamePtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}const char *szNamePtr = "Name";
+
+const char NameArray[] = "Name";
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: invalid case style for variable 'NameArray' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}const char szNameArray[] = "Name";
+
+void *BufferPtr1 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'BufferPtr1' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void *pBufferPtr1 = NULL;
+
+void **BufferPtr2 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'BufferPtr2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void **ppBufferPtr2 = NULL;
+
+void **pBufferPtr3 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'pBufferPtr3' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void **ppBufferPtr3 = NULL;
+
+int8_t ValueI8 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'ValueI8' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int8_t i8ValueI8 = 0;
+
+int16_t ValueI16 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI16' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int16_t i16ValueI16 = 0;
+
+int32_t ValueI32 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI32' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int32_t i32ValueI32 = 0;
+
+int64_t ValueI64 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI64' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int64_t i64ValueI64 = 0;
+
+uint8_t ValueU8 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueU8' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint8_t u8ValueU8 = 0;
+
+uint16_t ValueU16 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU16' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint16_t u16ValueU16 = 0;
+
+uint32_t ValueU32 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU32' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint32_t u32ValueU32 = 0;
+
+uint64_t ValueU64 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU64' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint64_t u64ValueU64 = 0;
+
+float ValueFloat = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'ValueFloat' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}float fValueFloat = 0;
+
+double ValueDouble = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'ValueDouble' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}double dValueDouble = 0;
+
+char ValueChar = 'c';
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for variable 'ValueChar' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}char cValueChar = 'c';
+
+bool ValueBool = true;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h:44
   llvm::Optional
-  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+  getDeclFailureInfo(const StringRef , const NamedDecl *Decl,
  const SourceManager ) const override;

dougpuob wrote:
> dougpuob wrote:
> > aaron.ballman wrote:
> > > Given that the other functions in the class use the wrong style of 
> > > casing, we should probably leave this declaration alone so it doesn't 
> > > become locally inconsistent.
> > Do you mean create another function with three parameters for it?
> I think I got it. Keep consistent even it is wrong style of casting. 
Yes, exactly!



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:428
 
   case IdentifierNamingCheck::CT_HungarianNotation: {
 const NamedDecl *pNamedDecl = dyn_cast(pDecl);

dougpuob wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > I feel like I must be missing something because I don't have this enum in 
> > > my copy of ToT despite pulling this morning. Is the patch missing some 
> > > content?
> > I feel like this is an incremental diff based on the first version of this 
> > patch rather than a diff from trunk. 
> @aaron.ballman and @njames93, the diff is based on first version not from 
> master(trunk).
> 
> Is it the best way that I should merge with the latest master(trunk) every 
> time before updating the diffs ?
Patches are typically a diff against trunk unless you have a series of related 
patches that build on top of one another (in which case, Phab has a way to mark 
related reviews so reviewers can still track the full context of the patch 
set). So when you create the diff, it should typically be against trunk.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/bugprone/ReservedIdentifierCheck.h:44
   llvm::Optional
-  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+  getDeclFailureInfo(const StringRef , const NamedDecl *Decl,
  const SourceManager ) const override;

dougpuob wrote:
> aaron.ballman wrote:
> > Given that the other functions in the class use the wrong style of casing, 
> > we should probably leave this declaration alone so it doesn't become 
> > locally inconsistent.
> Do you mean create another function with three parameters for it?
I think I got it. Keep consistent even it is wrong style of casting. 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob marked 2 inline comments as not done.
dougpuob added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h:44
   llvm::Optional
-  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+  getDeclFailureInfo(const StringRef , const NamedDecl *Decl,
  const SourceManager ) const override;

aaron.ballman wrote:
> Given that the other functions in the class use the wrong style of casing, we 
> should probably leave this declaration alone so it doesn't become locally 
> inconsistent.
Do you mean create another function with three parameters for it?



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:428
 
   case IdentifierNamingCheck::CT_HungarianNotation: {
 const NamedDecl *pNamedDecl = dyn_cast(pDecl);

njames93 wrote:
> aaron.ballman wrote:
> > I feel like I must be missing something because I don't have this enum in 
> > my copy of ToT despite pulling this morning. Is the patch missing some 
> > content?
> I feel like this is an incremental diff based on the first version of this 
> patch rather than a diff from trunk. 
@aaron.ballman and @njames93, the diff is based on first version not from 
master(trunk).

Is it the best way that I should merge with the latest master(trunk) every time 
before updating the diffs ?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:428
 
   case IdentifierNamingCheck::CT_HungarianNotation: {
 const NamedDecl *pNamedDecl = dyn_cast(pDecl);

aaron.ballman wrote:
> I feel like I must be missing something because I don't have this enum in my 
> copy of ToT despite pulling this morning. Is the patch missing some content?
I feel like this is an incremental diff based on the first version of this 
patch rather than a diff from trunk. 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h:44
   llvm::Optional
-  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+  getDeclFailureInfo(const StringRef , const NamedDecl *Decl,
  const SourceManager ) const override;

Given that the other functions in the class use the wrong style of casing, we 
should probably leave this declaration alone so it doesn't become locally 
inconsistent.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:428
 
   case IdentifierNamingCheck::CT_HungarianNotation: {
 const NamedDecl *pNamedDecl = dyn_cast(pDecl);

I feel like I must be missing something because I don't have this enum in my 
copy of ToT despite pulling this morning. Is the patch missing some content?



Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:69
   llvm::Optional
-  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+  getDeclFailureInfo(const StringRef , const NamedDecl *Decl,
  const SourceManager ) const override;

Same here.



Comment at: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h:127
   virtual llvm::Optional
-  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+  getDeclFailureInfo(const StringRef , const NamedDecl *Decl,
  const SourceManager ) const = 0;

And same here.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D86671#2244236 , @dougpuob wrote:

> In D86671#2243788 , @Eugene.Zelenko 
> wrote:
>
>> It'll be good idea to add test case.
>
> Hi @Eugene.Zelenko,
> I have created a `readability-identifier-naming-hungarain-notion.cpp`  file 
> and several test cases for regression testing. Is it regression testing?
>
> Find it here (https://reviews.llvm.org/differential/changeset/?ref=2137882)

Yes, this is regression test. Please add newline at the end.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2243788 , @Eugene.Zelenko 
wrote:

> It'll be good idea to add test case.

Hi @Eugene.Zelenko,
I have created a `readability-identifier-naming-hungarain-notion.cpp`  file and 
several test cases for regression testing. Is it regression testing?

Find it here (https://reviews.llvm.org/differential/changeset/?ref=2137882)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

In D86671#2243980 , @njames93 wrote:

> Can you make sure you upload diffs with full context (-U9). Or using 
> arcanist it will be done automatically.
>
> Make sure the diff is up to date with trunk
>
> Remove any changes that aren't related to this patch, they just make this 
> look noisy.

Hi @njames93,

1. It is my first time to use Phabricator and Arcanist, I will check how to 
make full context (-U9).
2. Do you mean that always sync with the latest master?
3. OK.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:317
+  if (Style.Case == IdentifierNamingCheck::CaseType::CT_HungarianNotation) {
+const auto TypePrefix = getHungarianNotationTypePrefix(Type.str(), Decl);
+if (TypePrefix.length() > 0) {

Eugene.Zelenko wrote:
> Please don't use auto when type is not specified explicitly in same statement 
> or iterator.
Got it, I fixed it in the next commit. Thank you.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:430
+const NamedDecl *pNamedDecl = dyn_cast(pDecl);
+const auto TypePrefix =
+getHungarianNotationTypePrefix(Type.str(), pNamedDecl);

Eugene.Zelenko wrote:
> Please don't use auto when type is not specified explicitly in same statement 
> or iterator.
Got it, I fixed it in the next commit. Thank you.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:70
 
+* Added: Add IdentifierNamingCheck::CaseType, CT_HungarianNotation, supporting 
naming check with Hungarian notation.
+

Eugene.Zelenko wrote:
> Please move to `Changes in existing checks` section (in alphabetical order).
> 
> I think statement should describe user-facing changes.
OK~ Fix it in next commit.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:76
 
-- Improved :doc:`readability-identifier-naming
+- Improved: doc:`readability-identifier-naming
   ` check.

Eugene.Zelenko wrote:
> Unrelated change.
OK~ Fix it on next commit.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Can you make sure you upload diffs with full context (-U9). Or using 
arcanist it will be done automatically.

Make sure the diff is up to date with trunk

Remove any changes that aren't related to this patch, they just make this look 
noisy.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It'll be good idea to add test case.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:70
 
+* Added: Add IdentifierNamingCheck::CaseType, CT_HungarianNotation, supporting 
naming check with Hungarian notation.
+

Please move to `Changes in existing checks` section (in alphabetical order).

I think statement should describe user-facing changes.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:76
 
-- Improved :doc:`readability-identifier-naming
+- Improved: doc:`readability-identifier-naming
   ` check.

Unrelated change.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 288520.
dougpuob added a comment.
Herald added a subscriber: mgehre.

Improved suggestions from code review and clang-tidy.

1. Add keyword `const` to variables which checked via clang-tidy.
2. Add log to clang-tools-extra/docs/ReleaseNotes.rst. [Suggestion from 
Eugene.Zelenko]
3. Don't use `auto` with variables are not specified explicitly. [Suggestion 
from Eugene.Zelenko]


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,10 +67,13 @@
 Improvements to clang-tidy
 --
 
+* Added: Add IdentifierNamingCheck::CaseType, CT_HungarianNotation, supporting naming check with Hungarian notation.
+
+
 Changes in existing checks
 ^^
 
-- Improved :doc:`readability-identifier-naming
+- Improved: doc:`readability-identifier-naming
   ` check.
 
   Added an option `GetConfigPerFile` to support including files which use
Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
===
--- clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
@@ -32,7 +32,7 @@
 
   /// Derived classes should not implement any matching logic themselves; this
   /// class will do the matching and call the derived class'
-  /// GetDeclFailureInfo() and GetMacroFailureInfo() for determining whether a
+  /// getDeclFailureInfo() and GetMacroFailureInfo() for determining whether a
   /// given identifier passes or fails the check.
   void registerMatchers(ast_matchers::MatchFinder *Finder) override final;
   void
@@ -124,7 +124,7 @@
   /// Overridden by derived classes, returns information about if and how a Decl
   /// failed the check. A 'None' result means the Decl did not fail the check.
   virtual llvm::Optional
-  GetDeclFailureInfo(const StringRef , const NamedDecl *Decl,
+  getDeclFailureInfo(const StringRef , const NamedDecl *Decl,
  const SourceManager ) const = 0;
 
   /// Overridden by derived classes, returns information about if and how a
Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
===
--- clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
@@ -400,11 +400,11 @@
 
   // Get type text of variable declarations.
   const auto  = Decl->getASTContext().getSourceManager();
-  const char *szBegin = SrcMgr.getCharacterData(Decl->getBeginLoc());
-  const char *szCurr = SrcMgr.getCharacterData(Decl->getLocation());
-  const intptr_t iPtrLen = szCurr - szBegin;
-  if (iPtrLen > 0) {
-std::string Type(szBegin, iPtrLen);
+  const char *Begin = SrcMgr.getCharacterData(Decl->getBeginLoc());
+  const char *Curr = SrcMgr.getCharacterData(Decl->getLocation());
+  const intptr_t StrLen = Curr - Begin;
+  if (StrLen > 0) {
+std::string Type(Begin, StrLen);
 
 const static std::list Keywords = {
 // Qualifier
@@ -415,23 +415,23 @@
 "constexpr", "constinit", "const_cast", "consteval"};
 
 // Remove keywords
-for (const auto  : Keywords) {
-  for (size_t pos = 0;
-   (pos = Type.find(kw, pos)) != std::string::npos;) {
-Type.replace(pos, kw.length(), "");
+for (const auto  : Keywords) {
+  for (size_t Pos = 0;
+   (Pos = Type.find(Kw, Pos)) != std::string::npos;) {
+Type.replace(Pos, Kw.length(), "");
   }
 }
 
 // Replace spaces with single space
-for (size_t pos = 0; (pos = Type.find("  ", pos)) != std::string::npos;
- pos += strlen(" ")) {
-  Type.replace(pos, strlen("  "), " ");
+for (size_t Pos = 0; (Pos = Type.find("  ", Pos)) != std::string::npos;
+ Pos += strlen(" ")) {
+  Type.replace(Pos, strlen("  "), " ");
 }
 
 // Replace " *" with "*"
-for (size_t pos = 0; (pos = Type.find(" *", pos)) != std::string::npos;
- pos += strlen("*")) {
-  Type.replace(pos, strlen(" *"), "*");
+for (size_t Pos = 0; (Pos = Type.find(" 

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please fix Clang-tidy warnings and mention changes in Release Notes.




Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:317
+  if (Style.Case == IdentifierNamingCheck::CaseType::CT_HungarianNotation) {
+const auto TypePrefix = getHungarianNotationTypePrefix(Type.str(), Decl);
+if (TypePrefix.length() > 0) {

Please don't use auto when type is not specified explicitly in same statement 
or iterator.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:430
+const NamedDecl *pNamedDecl = dyn_cast(pDecl);
+const auto TypePrefix =
+getHungarianNotationTypePrefix(Type.str(), pNamedDecl);

Please don't use auto when type is not specified explicitly in same statement 
or iterator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob updated this revision to Diff 288302.
dougpuob added a comment.

Fixed typos and add new Case Type, szHungarianNotation in doc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86671

Files:
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp
@@ -0,0 +1,103 @@
+#include 
+#include 
+
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: readability-identifier-naming.VariableCase, value: szHungarianNotation}, \
+// RUN:   ]}"
+
+const char *NamePtr = "Name";
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: invalid case style for variable 'NamePtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}const char *szNamePtr = "Name";
+
+const char NameArray[] = "Name";
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: invalid case style for variable 'NameArray' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}const char szNameArray[] = "Name";
+
+void *BufferPtr1 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'BufferPtr1' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void *pBufferPtr1 = NULL;
+
+void **BufferPtr2 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'BufferPtr2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void **ppBufferPtr2 = NULL;
+
+void **pBufferPtr3 = NULL;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'pBufferPtr3' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}void **ppBufferPtr3 = NULL;
+
+int8_t ValueI8 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'ValueI8' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int8_t i8ValueI8 = 0;
+
+int16_t ValueI16 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI16' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int16_t i16ValueI16 = 0;
+
+int32_t ValueI32 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI32' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int32_t i32ValueI32 = 0;
+
+int64_t ValueI64 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueI64' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int64_t i64ValueI64 = 0;
+
+uint8_t ValueU8 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for variable 'ValueU8' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint8_t u8ValueU8 = 0;
+
+uint16_t ValueU16 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU16' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint16_t u16ValueU16 = 0;
+
+uint32_t ValueU32 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU32' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint32_t u32ValueU32 = 0;
+
+uint64_t ValueU64 = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for variable 'ValueU64' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}uint64_t u64ValueU64 = 0;
+
+float ValueFloat = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for variable 'ValueFloat' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}float fValueFloat = 0;
+
+double ValueDouble = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for variable 'ValueDouble' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}double dValueDouble = 0;
+
+char ValueChar = 'c';
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for variable 'ValueChar' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}char cValueChar = 'c';
+
+bool ValueBool = true;
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: invalid case style for variable 'ValueBool' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}bool bValueBool = true;
+
+int ValueInt = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: invalid case style for variable 'ValueInt' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}int iValueInt = 0;
+
+size_t ValueSize = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style