Page MenuHomePhabricator

[clang-tidy] Adding a new bugprone check for streaming objects of type int8_t or uint8_t
Needs RevisionPublic

Authored by BRevzin on Jan 4 2018, 1:33 PM.

Details

Summary

Adds a check to clang-tidy to warn when streaming objects of type int8_t or uint8_t, when the likely intended behavior is to promote to int first.

Diff Detail

Event Timeline

BRevzin created this revision.Jan 4 2018, 1:33 PM
BRevzin set the repository for this revision to rCTE Clang Tools Extra.Jan 4 2018, 2:41 PM

So admittedly, I don't have any experience in clang-tidy. This check is intended to catch what is to us a really annoying source of error (and I'm probably not the only one?). I suspect there are far better ways of writing this check than the one that I pretty much guess-and-checked my way into.

Quuxplusone added inline comments.
clang-tidy/bugprone/StreamInt8Check.cpp
45

I don't know clang-tidy style either, but might it be more appropriate to say something like "value of type int8_t will be printed as character, not number"? I had to go all the way down to the test cases in this patch before it occurred to me what the actual problem being diagnosed here was.

And speaking of "printed": do you care about streaming *in* values of these types with ">>"?

Eugene.Zelenko added inline comments.
clang-tidy/bugprone/StreamInt8Check.cpp
2

Please add real description, use space after it and shorten to 80 symbols.

26

May be match should be configurable, so other streams could be analyzed too? How about LLVM's raw_ostream?

37

Please don't use auto in cases where return type could not be easily deduced. See modernize-use-auto. Same for auto Underlying.

clang-tidy/bugprone/StreamInt8Check.h
2

Please add real description and use space after it.

20

Please write real description here.

docs/ReleaseNotes.rst
62

Please move to new checks section in alphabetical order and add one statement description (usually first statement from documentation).

test/clang-tidy/bugprone-stream-int8.cpp
33

Please use meaningful name or main.

Could you please add a test case with a template that reduces the type to int8 or uint8?

JonasToth added inline comments.Jan 4 2018, 11:28 PM
clang-tidy/bugprone/StreamInt8Check.cpp
27

You can use bind for the expr and get Offender from it.

42

The duplicated diag can be removed. You can use placeholders in diag and make the if include both types.
See https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp#L246

I think having a list of possible typedefnames would be nice. Some projects might not use the standard ones (e.g. Qt?).

docs/ReleaseNotes.rst
62

Since the branch occured it might be better to rebase first. Then the Release Notes are empty.

BRevzin updated this revision to Diff 128768.Jan 5 2018, 10:01 AM

Updates based on review comments - and rebased off of latest so as to get the ReleaseNotes right. Added options so that the user can provide the list of stream types and int typedef types, as desired, defaulting to just basic_ostream and int8_t/uint8_t.

BRevzin marked an inline comment as done.Jan 5 2018, 10:10 AM

Could you please add a test case with a template that reduces the type to int8 or uint8?

I don't actually know how to do that. I tried a few things, but getting the type of the expression through a template gets me directly to unsigned char, not to uint8_t.

Could you please add a test case with a template that reduces the type to int8 or uint8?

I don't actually know how to do that. I tried a few things, but getting the type of the expression through a template gets me directly to unsigned char, not to uint8_t.

Yes that is possible. Templates ignore the typedefnames. I think that needs to be considered. It might be relevant for a templated operator<<.

It would be nice to have these cases in tests to document the issue. A note in the docs wouldn't hurt too.

JonasToth added inline comments.Jan 6 2018, 3:08 AM
clang-tidy/bugprone/StreamInt8Check.h
20

Missing full stop in comment

aaron.ballman added inline comments.Jan 6 2018, 9:36 AM
clang-tidy/bugprone/BugproneTidyModule.cpp
64

I don't think this is a good name for the check, especially because the check can be configured for types other than integers. How about "bugprone-suspicious-stream-value-type"? If you rename the check name, you should also update the name of the files and check accordingly.

clang-tidy/bugprone/StreamInt8Check.cpp
25

This should be ::std::basic_ostream just in case someone does something awful like put namespace std inside another namespace.

48

It'd be nice to have a qualifier-agnostic way of expressing this (there are other qualifiers, like volatile, and we don't want to have to list them all out unless it's important for the check functionality).

56–57

You can use llvm::find(AliasTypes, Name) here instead.

It might also make sense to use an llvm::StringSet instead of a vector of strings that you have to do a linear search over.

63

const auto *?

64

Should not use auto here.

67

This diagnostic doesn't really explain what the issue is. It should probably be reworded to something more along the lines of "%0 will not undergo integral promotion prior to streaming; cast to 'int' to prevent streaming a character".

68

If you pass in Name, then it won't be properly quoted. You should pass Typedef->getDecl() (but not repeat the expression from above).

72–73

You can hoist this into the for loop.

clang-tidy/bugprone/StreamInt8Check.h
19

types -> type

docs/ReleaseNotes.rst
63

Extra space between the comma and "if".

I'd reword this a bit to:

Checks that an object of type int8_t or uint8_t is not streamed. Those types are usually a typedef to a character type that are printed as a character rather than an integer. The user likely intends to stream such a value as an int instead.

(Or something like this.)

docs/clang-tidy/checks/bugprone-stream-int8.rst
6–8

Same wording suggestions as above.

alexfh requested changes to this revision.Mar 14 2018, 8:20 AM
This revision now requires changes to proceed.Mar 14 2018, 8:20 AM