This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add modernize-type-traits check
ClosedPublic

Authored by njames93 on Nov 2 2022, 4:37 PM.

Details

Summary

This check will look for usages of standard library type traits of the form traits<...>::type and traits<...>::value and convert them into traits_t<...> and traits_v<...> respectively.
This expands on the work in D135404 by supporting dependent traits with no instantiations as well as types.

Diff Detail

Event Timeline

njames93 created this revision.Nov 2 2022, 4:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 4:37 PM
njames93 requested review of this revision.Nov 2 2022, 4:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 4:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.Nov 2 2022, 4:51 PM
clang-tools-extra/docs/ReleaseNotes.rst
121

Please use double back-ticks for language constructs.

clang-tools-extra/docs/clang-tidy/checks/modernize/type-traits.rst
7

Ditto.

27

Excessive newline.

njames93 updated this revision to Diff 473028.Nov 3 2022, 2:04 PM
njames93 marked 3 inline comments as done.

Address documentation comments

njames93 updated this revision to Diff 473427.Nov 5 2022, 7:17 AM

Fix various crashes on real world code

tschuett added inline comments.
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
17

Why do you refuse to use nested namespaces in modernize ?

njames93 updated this revision to Diff 473439.Nov 5 2022, 9:41 AM

Fix crash when running over llvm

njames93 added inline comments.Nov 5 2022, 9:43 AM
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
17

Because the add_new_check script hasn't been updated since we went to c++17

Eugene.Zelenko added inline comments.Nov 5 2022, 9:44 AM
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
17

I think this is done for legacy reasons. This should be done for all checks and add_new_check.py.

This seems to be stuck with no reviewers, is there any way to help here?

carlosgalvezp added inline comments.Dec 19 2022, 11:22 PM
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
141

I don't see a reason why the naming convention can't be followed here?

143

Why is an internal namespace needed, when you are already have the anonymous namespace?

144

Internal functions and variables should be declared "static", outside the anonymous namespace, as per the LLVM Coding Standards. AST matchers can be put in the anonymous namespace.

182

I think clang-format should remove these braces since it's only 1 statement in the "if" condition.

clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
31

Nit: typically this is implemented inline in the header.

32

There's an ongoing effort to deprecate llvm::Optional in favor of std::optional, perhaps start using it right away in this check?

clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp
24

Typo. Shouldn't the test have failed?

njames93 marked 2 inline comments as done.Jan 12 2023, 10:36 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
141

Following the naming convention of the AST matchers, which all use camelBack

143

I think it was because the matchers use an internal namespace, probably not need and can be removed. Let me check.

182

As its more than 1 lines, keeping it in braces is cleaner.
Also clang-format passed on the pre-merge so it thinks this code is correct according to the style guide

clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
32

This patch was last update before the switch, when the virtual function still returned an llvm::Optional, when I rebase it will need changing.

clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp
24

Good catch, it won't fail the test as filecheck will just ignore it.

njames93 updated this revision to Diff 488724.Jan 12 2023, 11:18 AM
njames93 marked 2 inline comments as done.

Address comments.

njames93 marked 7 inline comments as done.Jan 12 2023, 11:25 AM
carlosgalvezp added inline comments.Jan 13 2023, 12:39 AM
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
18

I'm guessing this list was obtained in some automatic way - could we document the process for generating it, so a future maintainer knows how to do it?

105

Ditto

106–109

Please keep alphabetical order.

135–140

Why do we need these matchers at global scope, if they are only used in the registerMatchers function?

Unless they are POD types, putting objects at global scope is bugprone since they are initialized before main in an unspecified order.

clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
17

Is the goal to update all existing checks to C++17 format?

clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp
39

There should be a space in here, right? Same for the rest.

53

As a user I find it a bit confusing that there is a warning, but no fix. Should we just disable the warning on macros entirely?

Furthermore, the warning shows up at the call site, so if it's not possible to fix it will be very noisy for people to suppress it everywhere where the macro is used. Maybe add an option IgnoreMacros similar to how it's done for other checks? Alternatively, as a user I would like to be able to suppress the warning in line 54 once only instead of at the call site.

njames93 marked 6 inline comments as done.Jan 13 2023, 3:07 PM
njames93 added inline comments.
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
18

They weren't, just copied from cppreference, I'm sure there is a way to automate it, but honestly there's not much point

106–109

This is the order they appear on cppreference, so I think it makes sense to keep it like that, will make for an easier time maintaining this

135–140

These are stateless matchers which require no construction, so there will be no initialisation order issues.

They could be moved into the function that uses them without any issue though.

clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
17

It should be done separately as an NFC change

clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp
39

I'm assuming you mean between <b> and inTemplate.
If so, because we disable clang-format in the tests, the code isn't being reformatted and the replacement logic happens to get rid of the space too. As missing the space doesn't break the code and clang-format would reformat the code for us in typical use cases, its not worth worrying about.

53

I think IgnoreMacros is probably the best approach here, Getting warnings about marcos can be helpful if they are able to be fixed, but it would be nice to also silence those in cases when they can't.

njames93 updated this revision to Diff 489148.Jan 13 2023, 3:57 PM
njames93 marked 6 inline comments as done.

Add IgnoreMacros options.
Address comments.

carlosgalvezp added inline comments.Jan 14 2023, 1:12 AM
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
173

static not needed here, remove. I don't think LLVM guidelines enforce strict const correctness either so the const could be removed as well if wanted.

clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.h
17

Sounds good, just want to make sure it's something we see value in fixing (as opposed to just being "churn"). I'm happy to put up an NFC patch for that!

clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp
39

I disagree, just because the code "compiles" it doesn't mean it's good. The only time I've seen code like this is in obfuscated code competitions :)

We should not force the user to run clang-format to fix this - clang-tidy should preserve the existing space. This is not a matter of style either, it's a de-facto way of writting C++ (and many other languages): variable types and variable names in a declaration are separated by a space.

njames93 marked an inline comment as done.Jan 14 2023, 9:58 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
173

const correctness isn't enforced, but it doesn't harm

clang-tools-extra/test/clang-tidy/checkers/modernize/type-traits.cpp
39

One of the original design decisions of clang-tidy was that check authors should not worry about formatting. This is partly because not everyone can agree on a style, but the main driving factor is that clang-tidy runs clang-format on the fixes that it generates unless you explicitly tell clang-tidy not to reformat your fixes.
Given that the set of real world projects that have a clang-tidy configuration file is nearly a perfect subset of the projects that have a clang-format configuration file, it's safe to assume that fixes would always be reformatted when running clang-tidy

njames93 updated this revision to Diff 489342.Jan 15 2023, 4:00 AM
njames93 marked an inline comment as done.

Remove unneeded static keyword
Improved fix-it logic if there is a space between the template name and the < token

njames93 marked an inline comment as done.Jan 15 2023, 4:07 AM
njames93 updated this revision to Diff 489347.Jan 15 2023, 4:32 AM

Extend test cases to check inline namespace, extension namespaces and alias namespaces

LGTM, really useful check! Would be nice to get some more eyes on the implementation, it's fairly advanced for my basic AST knowledge.

Looks good to me - looking forward to this check!

clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
171

NIT: should the bound node have some meaningful non-empty name?

198

For a future release, could we add an option to allow replacing "standard type traits" defined in separate namespaces, as the BSL library does? This would be useful in my case at least.

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:11 AM
njames93 updated this revision to Diff 511702.Apr 7 2023, 8:58 AM

Rebased and sorted release notes

njames93 marked 2 inline comments as done.Apr 7 2023, 10:36 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/modernize/TypeTraitsCheck.cpp
171

Not strictly required as we only ever bind one node.

198

This could definitely be extended in the future to support extended traits.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 7 2023, 10:38 AM
This revision was automatically updated to reflect the committed changes.
njames93 marked 2 inline comments as done.