Page MenuHomePhabricator

[clang-tidy] Add new case type to check variables with Hungarian notation
AcceptedPublic

Authored by dougpuob on Aug 26 2020, 7:00 PM.

Details

Summary

Add IdentifierNamingCheck::CaseType, CT_HungarianNotation, supporting naming check with Hungarian notation.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Nov 19 2020, 10:48 AM
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
167 ↗(On Diff #306411)

All the changes in this file are unrelated.

llvm/lib/Target/SystemZ/SystemZISelLowering.h
54 ↗(On Diff #306411)

Unrelated changes.

llvm/lib/Target/SystemZ/SystemZInstrInfo.td
835 ↗(On Diff #306411)

All the changes in this file are unrelated.

llvm/lib/Target/SystemZ/SystemZInstrVector.td
574 ↗(On Diff #306411)

Unrelated changes.

llvm/lib/Target/SystemZ/SystemZOperators.td
669 ↗(On Diff #306411)

All the changes in this file are unrelated.

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
15 ↗(On Diff #306411)

All the changes in this file are unrelated.

dougpuob updated this revision to Diff 306867.Nov 21 2020, 11:57 AM

Against git master.

dougpuob marked 82 inline comments as done.Nov 22 2020, 3:43 AM

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.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
245–251

OK. I checked all the range including single-line if statements, and passing StringRef directly(not its reference).

255

I improved it. It will look like the following:

static constexpr std::pair<StringRef, StringRef> CStrings[] = {
      {"char*", "sz"}, {"char[]", "sz"}, {"wchar_t*", "wsz"}, {"wchar_t[]", "wsz"}};
377–381

Improved here too. It will change to:

static constexpr std::pair<StringRef, StringRef> HNCStrings[] = {
    {"CharPrinter", "char*"},
    {"CharArray", "char[]"},
    {"WideCharPrinter", "wchar_t*"},
    {"WideCharArray", "wchar_t[]"}};
dougpuob removed a project: Restricted Project.Nov 22 2020, 5:00 AM
dougpuob updated this revision to Diff 306908.Nov 22 2020, 5:37 AM
dougpuob marked 3 inline comments as done.
  • Improved code review suggestions from Aaron Ballman(aaron.ballman). Including document, single-line if statements, no reference with StringRef, "char"-->"char[]" for HNOption.CString, use llvm::transform insteads of for-loop to uppercase string, don't check CXXMethod in getDeclTypeName(), remove const_cast<char *> in getDeclTypeName(), use if-else insteads of switch-case(simplify src).
aaron.ballman added inline comments.Nov 23 2020, 6:06 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
920–923
975–976
1013

This comment doesn't appear to have been handled.

1044

Similar to what was suggested above.

Eugene.Zelenko added inline comments.Nov 23 2020, 6:41 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
918

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

dougpuob marked 5 inline comments as done.Nov 24 2020, 7:56 AM

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

dougpuob updated this revision to Diff 307384.Nov 24 2020, 9:20 AM
  • Improved code review suggestions from Aaron.Ballman and Eugene.Zelenko. Including return default initializer instead of empty string, use isa<>() instead of if statement with multiple conditions, use StringRef array instead of list<std::string>.
aaron.ballman accepted this revision.Dec 4 2020, 8:45 AM

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

This revision is now accepted and ready to land.Dec 4 2020, 8:45 AM
njames93 added inline comments.Dec 4 2020, 3:23 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
251–255
260–264
272–276
324–328
363–364
382
389
402
408–417
423–425
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
207–208

This should be part of the FileStyle struct.

dougpuob marked 11 inline comments as done.Dec 6 2020, 6:45 AM
dougpuob updated this revision to Diff 309767.Dec 6 2020, 6:48 AM
  • Improved code review suggestions from @njames93. Including move the IdentifierNamingCheck::HNOption variable to IdentifierNamingCheck::FileStyle::HNOption, use try_emplace() instead of insert() and lookup().
dougpuob updated this revision to Diff 309770.Dec 6 2020, 7:01 AM

arc diff with the correct base.

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

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
399–405

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

dougpuob updated this revision to Diff 311211.Dec 11 2020, 6:23 AM
  • Against git master.
  • Refined isHungarianNotationOptionEnabled() function.
  • Classify document and test cases.
dougpuob updated this revision to Diff 311267.Dec 11 2020, 9:59 AM
  • Fixed assertions by clang-format.
dougpuob marked an inline comment as done.Dec 11 2020, 10:25 PM

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).

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?

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
259–286
399–405

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.

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.

dougpuob updated this revision to Diff 313970.Dec 29 2020, 7:50 AM
  • Against git master.
  • Refined isHungarianNotationOptionEnabled() function.
  • Classify document and test cases.
  • Fixed assertions by clang-format.
  • Instead check On/Off string with llvm::yaml::parseBool() function for consistent.
  • Improved code.
  • Add new check for unsupported option.
dougpuob updated this revision to Diff 314076.Dec 29 2020, 11:01 PM
dougpuob marked 2 inline comments as done.
  • Removed unrelated comments.

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.

dougpuob updated this revision to Diff 342126.May 1 2021, 12:14 AM
  • Against trunk.
  • Refined isHungarianNotationOptionEnabled() function.
  • Changed old way that showing invalid config option in .clang-tidy config file. Now added a new function ClangTidyCheck::OptionsView::diagnoseInvalidConfigOption() instead.
  • Recovered that document content missing in .rst.

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

clang-tools-extra/clang-tidy/ClangTidyCheck.h
158

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

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
263

Hi @njames93,

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

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
263

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 &Options,
                        ClangTidyContext &Context) {
    ...

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

    ...
}
njames93 added inline comments.May 2 2021, 1:37 AM
clang-tools-extra/clang-tidy/ClangTidyCheck.h
124–126

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

dougpuob added inline comments.May 2 2021, 7:37 AM
clang-tools-extra/clang-tidy/ClangTidyCheck.h
124–126

But the getFileStyleFromOptions() is a static function in IdentifierNamingCheck.cpp file. It can't access configurationDiag() directly.

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 &Options,
                        ClangTidyContext &Context) {
    ...
    StyleString.append("HungarianPrefix");
    auto HPTOpt =
        Options.get<IdentifierNamingCheck::HungarianPrefixType>(StyleString);
    HN.checkOptionValid(I, StyleString, HPTOpt.hasValue());
    ...
dougpuob updated this revision to Diff 344060.May 10 2021, 7:43 AM
  • Removed diagnoseInvalidConfigOption() function.
  • Created a HungarianNotation class and moved related functions into there.

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.

njames93 added a comment.EditedMay 12 2021, 10:23 AM

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.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
246

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

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.

dougpuob updated this revision to Diff 345682.May 16 2021, 1:50 AM
  • Moved all static functions into IdentifierNamingCheck class, and called the configurationDiag() directly in IdentifierNamingCheck::getFileStyleFromOptions() method.