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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
