[PATCH] D41740: [clang-tidy] Adding a new bugprone check for streaming objects of type int8_t or uint8_t

[Aaron Ballman via Phabricator via cfe-commits]

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