[PATCH] D142587: [clang-tidy] Improved too-small-loop-variable with bit-field support

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the fix! I have some suggestions for improved readability.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:97
 
 /// Returns the magnitude bits of an integer type.
+static std::pair<unsigned, unsigned>
----------------
Having an `std::pair` is a bit hard to read, as it's not clear what each element of the pair represents. Could you replace it with something like this, for improved readability and maintainability? Then you can also skip the `utility` header.

```
struct MagnitudeBits
{
  unsigned Width;
  bool IsBitField;
};
```




================
Comment at: clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:186
+  auto Msg =
+      diag(LoopVar->getBeginLoc(), "loop variable has narrower type '%0%1%2' "
+                                   "than iteration's upper bound '%3%4%5'");
----------------
This is a bit confusing, could you keep it with `%0` and `%1` (which clearly represent "loop variable" and "upper bound"), and simply create a `std::string` with the appropriate content? Something like:

```
std::string type = LoopVarType.getAsString();
if (LoopVarMagnitudeBits.second) {
  type += ":" + std::to_string(LoopVarMagntiudeBits.second);
}
Msg << type;

```




================
Comment at: clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:197-202
+  if (UpperBoundMagnitudeBits.second) {
+    Msg << ":" << UpperBoundMagnitudeBits.second;
+  } else {
+    Msg << ""
+        << "";
+  }
----------------
Please create a helper function to remove duplication.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142587/new/

https://reviews.llvm.org/D142587