This is an archive of the discontinued LLVM Phabricator instance.

[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

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

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

OK~ I moved the HNOption to IdentifierNamingCheck class as its private data member. Then give value by constructor initializer list (instead of clearAll()).

517–518

OK.

518

OK~ Thank you. I will check them.

519

OK.

521

OK.

521–522

Sure! it's what should I do, you have helped me a lots.

526

Nice, thank you.

533–540

OK.

544–548

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();
  // ...
}
585

OK.

586

OK, thank you.

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

njames93 added inline comments.Oct 31 2020, 2:58 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
229

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.

434–449

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.

dougpuob updated this revision to Diff 302086.Oct 31 2020, 5:45 AM
  • Removed clearAll() function and made the HPOption variable passing by value(origin is std::move()).
  • Moved static HNOption variable from static getNamingStyles() function to IdentifierNamingCheck class a private data member.
  • Replaced auto keyword with explicit std::string.
  • Elided braces around single-line.
  • Used CXXRecordDecl::isAbstract() directly.
  • changed const auto to const auto * for pointer objects.
  • Enhanced test case for enum with two EnumConstant decls.
  • Removed unnecessary clang:: namespace for variables.
  • Executed arc lint
  • Final changes.
  • Executed arc lint
dougpuob added inline comments.Nov 1 2020, 4:58 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
229

I removed the static variable from the function and a reference in NamingStyle.

434–449

Good idea. Thank you.

dougpuob updated this revision to Diff 302192.Nov 1 2020, 5:00 PM
  1. Applied all patches with the trunk(1800b44651c19b11e7680f908344d5751e8d2246).
  2. Moved HNOption pointer out of NamingStyle data structure.
  3. Removed parseHungarianPrefix() function, use Options.get() instead.
njames93 added inline comments.Nov 1 2020, 6:06 PM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
123

Is this line needed?

233

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

358
360

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

361–362

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

416
707
dougpuob added inline comments.Nov 4 2020, 7:34 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
123

I will remove it. Thank you.

233

Thank you. I will change it.

360

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?

361–362

Good idea, thank you.

dougpuob updated this revision to Diff 302842.Nov 4 2020, 7:37 AM
  1. Fixed build failure on BuildBot. (Touch empty data from Options.get())
  2. Changed llvm::StringMap with std::pair in getHungarianNotationDefaultConfig().
  3. Moved HNOpts and HNDerviedTypes declarations out of getHungarianNotationDefaultConfig().
  4. Reused SmallString<128> for string concatenation.
  5. Removed redundant m(HungarianNotation) in #define NAMING_KEYS(m).
dougpuob updated this revision to Diff 302856.Nov 4 2020, 8:38 AM
  • Fixed warnings of Clang-Tidy.
dougpuob updated this revision to Diff 304885.Nov 12 2020, 10:10 AM
  • Shortened namespace with type alias.
  • Removed namespace for StringRef.
dougpuob updated this revision to Diff 306411.Nov 19 2020, 7:32 AM

Against git master.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 19 2020, 7:32 AM
aaron.ballman added inline comments.Nov 19 2020, 10:48 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
237

Usual style is to elide braces for single-line if statements (applies elsewhere in the patch as well).

254

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?

376–380

Similar question here as above, but less pressing because we at least have the word "array" nearby.

421–422
424–427

Formatting

586–587
597–598
662
673–675

No need to check for CXXMethodDecl because those inherit from FunctionDecl anyway.

689

I think EOL should probably be const char * and can remove remove all these const_cast<>?

776

I think this switch should be replaced with:

if (const auto *ECD = dyn_cast<EnumConstantDecl>(ND)) {
  ...
} else if (const auto *CRD = dyn_cast<CXXRecordDecl>(ND)) {
  ...
} else if (isa<VarDecl, FieldDecl, RecordDecl>(ND)) {
  ...
}
781

Can remove all of the if (ND) in this method -- we already early return if ND == nullptr.

821–822
845
1009–1027
1317–1318

There's no need to pass a StringRef by const reference -- they pass like a pointer type already.

1365

And can remove the declaration of TypeName above.

Also, can you correct the formatting issues, elsewhere as well.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
139

Might as well clear up this lint warning.

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp
55 ↗(On Diff #306411)

This change looks unrelated to the patch?

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
363 ↗(On Diff #306411)

Same here?

clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp
41 ↗(On Diff #306411)

And these changes as well?

clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
115

For all of these, I'd recommend a slight wording tweak to:

When enabled, the check ensures that the declared identifier will have a Hungarian notation prefix based on the declared type.
2521–2522

I would remove this paragraph as it doesn't really add much value (it effectively says that the option works well with other options, but that's already expected).

2569
2572
2575
2578–2579
lldb/include/lldb/Target/Process.h
565 ↗(On Diff #306411)

All the changes in this file are unrelated.

lldb/source/Target/Process.cpp
532 ↗(On Diff #306411)

Unrelated changes.

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
254

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"}};
376–380

Improved here too. It will change to:

static constexpr std::pair<StringRef, StringRef> HNCStrings[] = {
    {"CharPrinter", "char*"},
    {"CharArray", "char[]"},
    {"WideCharPrinter", "wchar_t*"},
    {"WideCharArray", "wchar_t[]"}};
424–427

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

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
614–617
669–670
707

This comment doesn't appear to have been handled.

738

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
612

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
430–434
439–443
451–455
503–507
542–543
561
568
581
587–596
602–604
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
138–139

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
398–404

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
398–404

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.

435–462

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 ↗(On Diff #342126)

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

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

Hi @njames93,

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

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

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 ↗(On Diff #342126)

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 ↗(On Diff #342126)

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
425

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?