aaron.ballman added inline comments.
================
Comment at: clang-tidy/bugprone/BugproneTidyModule.cpp:64
+ CheckFactories.registerCheck<StreamInt8Check>(
+ "bugprone-stream-int8");
CheckFactories.registerCheck<StringConstructorCheck>(
----------------
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.
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:25
+ "StreamTypes",
+ "std::basic_ostream"))),
+ AliasTypes(utils::options::parseStringList(Options.get(
----------------
This should be `::std::basic_ostream` just in case someone does something awful
like put `namespace std` inside another namespace.
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:48
+ anyOf(asString("signed char"),
+ asString("const signed char"),
+ asString("unsigned char"),
----------------
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).
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:56-57
+bool StreamInt8Check::isBugproneAlias(StringRef Name) const {
+ return std::find(AliasTypes.begin(), AliasTypes.end(), Name)
+ != AliasTypes.end();
+}
----------------
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.
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:63
+
+ for (auto Typedef = Offender->getType().getTypePtr()->getAs<TypedefType>();
Typedef; ) {
+ auto Name = Typedef->getDecl()->getNameAsString();
----------------
`const auto *`?
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:64
+ for (auto Typedef = Offender->getType().getTypePtr()->getAs<TypedefType>();
Typedef; ) {
+ auto Name = Typedef->getDecl()->getNameAsString();
+ if (isBugproneAlias(Name)) {
----------------
Should not use `auto` here.
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:67
+ diag(Offender->getLocStart(),
+ "streaming %0; did you mean to stream an int?")
+ << Name << Offender->getSourceRange();
----------------
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".
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:68
+ "streaming %0; did you mean to stream an int?")
+ << Name << Offender->getSourceRange();
+ break;
----------------
If you pass in `Name`, then it won't be properly quoted. You should pass
`Typedef->getDecl()` (but not repeat the expression from above).
================
Comment at: clang-tidy/bugprone/StreamInt8Check.cpp:72-73
+
+ QualType Underlying = Typedef->getDecl()->getUnderlyingType();
+ Typedef = Underlying.getTypePtr()->getAs<TypedefType>();
+ }
----------------
You can hoist this into the `for` loop.
================
Comment at: clang-tidy/bugprone/StreamInt8Check.h:19
+
+/// Checks that objects of types int8_t or uint8_t aren't streamed to ostreams,
+/// where the intended behavior is likely to stream them as ints
----------------
types -> type
================
Comment at: docs/ReleaseNotes.rst:63
+
+ Checks that objects of type ``int8_t`` or ``uint8_t`` aren't streamed, if
those
+ are typedefs to ``signed char`` or ``unsigned char``, as this likely intends
to
----------------
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.)
================
Comment at: docs/clang-tidy/checks/bugprone-stream-int8.rst:6-8
+Checks that objects of type ``int8_t`` or ``uint8_t`` aren't streamed, if
those
+are typedefs to ``signed char`` or ``unsigned char``, as this likely intends to
+be streaming these types as ``int`` s instead.
----------------
Same wording suggestions as above.
https://reviews.llvm.org/D41740
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
