Page MenuHomePhabricator

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

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

Is this ready to merge soon?

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?

thakis added a subscriber: thakis.Aug 1 2021, 11:21 PM

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?

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?

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

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

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

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

Hi @aeubanks , thank you for the suggestion.

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?