Page MenuHomePhabricator

[clang-tidy] Add `bugprone-reserved-identifier` to flag usages of reserved C++ names
AbandonedPublic

Authored by logan-5 on Jan 4 2020, 12:53 PM.

Details

Summary

This commit adds bugprone-reserved-identifier, which flags usages of __names _Like ::_this, which are illegal in user code. The check can also be inverted to flag any names that are not reserved, which would be useful to e.g. standard library implementors.

A large part of this diff is factoring out a big chunk of logic from readability-identifier-naming into a shared base class that is also used by this new check. I strongly considered simply adding this new check's logic to readability-identifier-naming instead of introducing a new separate check, but I decided that this check's goals are orthogonal to that one: this one seeks to flag UB, where readability-identifier-naming is concerned with style. It is also desirable to be able to enable/disable one and not the other and configure them separately.

The new base class will hopefully make adding any further new "renamer" checks in the future a breeze.

Adding some libc++ reviewers to see if they think this would be useful for their workflow in its current form, or if they have any feedback regarding a different approach. I admit the "Whitelist" option for not flagging legitimate identifiers is a bit cumbersome, but I wasn't sure of a better way.

Diff Detail

Event Timeline

logan-5 created this revision.Jan 4 2020, 12:53 PM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a subscriber: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2020, 1:33 PM

I like this change, but I don't feel qualified to fully review the patch.

I wonder what happens if the project already contains a suggested fix, for example:

#define __FOO(X) ...
#define _FOO(X) __FOO(X)
#define FOO(X) _FOO(X)

will it suggest:

#define FOO(X) ...
#define FOO(X) FOO(X)
#define FOO(X) FOO(X)

?

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
71

Name.size() > 1 doesn't catch _ in the global namespace. Catching _ will probably also cause issues with the automatic renaming.
What about renaming __?

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
22

Why suggesting __Helper instead of _Helper ?

57

Likewise why not _Up ?

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
69

Should it not suggest to change this to namespace ns ?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
50

Please separate with empty line.

61

Please separate with empty line.

73

Please separate with empty line.

115

Please don't use auto when type is spelled in same statement or iterator.

118

Please don't use auto when type is spelled in same statement or iterator.

121

Please don't use auto when type is spelled in same statement or iterator.

147

Please separate with empty line.

153

Please separate with empty line.

169

Please separate with empty line.

170

Unnecessary empty line.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
14

Please include vector and llvm/Optional.h.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
646–648

Please separate with empty line.

674–680

Please separate with empty line.

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
11

Unnecessary empty line.

60

Please separate with empty line.

66

Please separate with empty line.

90

Please separate with empty line.

133

Please don't use auto when type is spelled in same statement or iterator.

166

Please don't use auto when type is spelled in same statement or iterator.

209

Please don't use auto when type is spelled in same statement or iterator.

214

Please don't use auto when type is spelled in same statement or iterator.

230

Could it be const? Same below.

259

Please don't use auto when type is spelled in same statement or iterator.

275

Please don't use auto when type is spelled in same statement or iterator.

350

Just {}. Same below.

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
2

Please merge two lines and make sure resulting line is 80 characters wide.

15

Please include string, utility, DenseSet, Optional.

125

Please separate with empty line.

clang-tools-extra/docs/ReleaseNotes.rst
100

Please synchronize with first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst
23

Unnecessary empty line.

31

Please enclose 0 in single back-ticks.

clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-reserved-identifier/system/system-header.h
34

Please add newline.

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
71

Please add newline.

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
184

Please add newline.

Mordante added a subscriber: eugene.Jan 4 2020, 2:02 PM

@eugene Please don't use auto when type is spelled in same statement or iterator. do you mean type is not spelled ?

I wonder what happens if the project already contains a suggested fix, for example:

#define __FOO(X) ...
#define _FOO(X) __FOO(X)
#define FOO(X) _FOO(X)

will it suggest:

#define FOO(X) ...
#define FOO(X) FOO(X)
#define FOO(X) FOO(X)

?

Yes, it will. This issue was already present in the logic I factored out of readability-identifier-naming, so I chose not to address it for this PR. A future patch should add logic to ensure that the renaming checks don't recommend correcting multiple identifiers to the same thing.

@Eugene.Zelenko +1 to @Mordante 's question. Otherwise, happy to address most of those nits, though I think I will gently push back on a couple of them (in the inline comments).

logan-5 added inline comments.Jan 4 2020, 2:38 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp
22

I mostly didn't see a reason to make the check opinionated on how to uglify names, though I suppose now that you mention it, a smaller change to the name when possible is arguably better.

@eugene Please don't use auto when type is spelled in same statement or iterator. do you mean type is not spelled ?

Yes, you are correct. Sorry for mistake.

i think it would be easier to review if there are two patches, one refactoring and one new check.

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
60

I think the kind should be an enum instead of stringly typing.

logan-5 marked an inline comment as done.Jan 4 2020, 2:49 PM

@Eugene.Zelenko never mind about pushing back on the nits; I had misread some of them. They all sound good, will address.

clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
71

Good point. Perhaps the check should catch these but simply not propose a fix.

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
69

That's a fair point. I'll point out (again) that this is an existing case missed by the logic factored out of readability-identifier-naming, so it might be better suited for a separate patch?

logan-5 updated this revision to Diff 236210.Jan 4 2020, 5:37 PM
logan-5 marked 39 inline comments as done.

Addressed formatting issues, and tweaked handling of the names __ and ::_: the check now flags these but doesn't suggest a fix.

logan-5 marked an inline comment as done.Jan 4 2020, 5:51 PM
logan-5 added inline comments.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
60

I can't disagree that the string is a little gross, but since the interpretation of the string is for use by the derived classes, I don't think an enum is flexible enough.

logan-5 updated this revision to Diff 236216.Jan 4 2020, 8:41 PM

Change double-underscore fix-it finding algorithm to correctly collapse any number of >=2 underscores in a row, not just exactly 2 (or multiples of 2).

Mordante added inline comments.Jan 5 2020, 5:20 AM
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
71

Agreed.

clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp
69

Ok. I think a separate patch will be better.

I'm taking @JonasToth's suggestion and splitting this into two patches, one for the refactor, followed by one for the new check. The refactor patch is here: https://reviews.llvm.org/D72284. Thanks everyone for your feedback so far.

logan-5 abandoned this revision.Jan 6 2020, 9:36 AM