This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add a checker that suggests replacing short/long/long long with fixed-size types.
ClosedPublic

Authored by bkramer on Aug 29 2014, 6:59 AM.

Diff Detail

Event Timeline

bkramer updated this revision to Diff 13078.Aug 29 2014, 6:59 AM
bkramer retitled this revision from to [clang-tidy] Add a checker that suggests replacing short/long/long long with fixed-size types..
bkramer added reviewers: alexfh, sbenza.
bkramer added a subscriber: Unknown Object (MLST).
alexfh accepted this revision.Aug 29 2014, 7:24 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/google/IntegerTypesCheck.cpp
84

Why not static const StringRef?

94

nit:
The "Emit a warning. " part is obvious, I'd remove it.

I'd swap the other two sentences and put each starting from its own line:

// We don't add a fix-it as changing the type can easily break code, 
// e.g. when a function requires a 'long' argument on all platforms.
// QualTypes are printed with implicit quotes.
clang-tidy/google/IntegerTypesCheck.h
20

Maybe:

... with u?intXX(_t)?

?

25

I'd use a bool flag instead of TypeSuffix or at least would add a comment describing the "_t" option for standard types.

30

Mark these fields "const"?

This revision is now accepted and ready to land.Aug 29 2014, 7:24 AM
bkramer updated this revision to Diff 13082.Aug 29 2014, 7:31 AM
bkramer edited edge metadata.

Address review comments.

Diffusion closed this revision.Aug 29 2014, 7:48 AM
Diffusion updated this revision to Diff 13085.

Closed by commit rL216726 (authored by d0k).

sbenza added inline comments.Aug 29 2014, 8:43 AM
clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp
45 ↗(On Diff #13085)

The name should be more explicit.

clang-tools-extra/trunk/clang-tidy/google/IntegerTypesCheck.cpp
20 ↗(On Diff #13085)

Move this to ASTMatchers.h

clang-tools-extra/trunk/test/clang-tidy/google-runtime-int.cpp
4 ↗(On Diff #13085)

This will fail on some platforms, right? "long" is not 64 bits everywhere (which is the point of this clang-tidy check).

alexfh added inline comments.Aug 29 2014, 9:13 AM
clang-tidy/google/IntegerTypesCheck.cpp
87

static const StringRef would save some strlen calls.

Also, when I was asking about StringRef, I had a thought that it could help avoiding std::strncmp. I was wrong, so StringRef doesn't make things any better. My bad. But it was a question, not a request to change it.

98

We should somehow hint the user, which header the new types are in.

clang-tidy/google/IntegerTypesCheck.h
20

It also finds unsigned versions of these, right?

27

I think, it makes sense noting that we need "_t" for standard types defined in <stdint.h>.