This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by PiotrZSL on Jan 25 2023, 3:05 PM.



Implemented support for bit-field members as a loop variable
or upper limit. Supporting also non bit-field integer members.

Fixes issues:

Diff Detail

Event Timeline

PiotrZSL created this revision.Jan 25 2023, 3:05 PM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL published this revision for review.Jan 25 2023, 3:20 PM

Ready for review.

Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 3:20 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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


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;

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;

Please create a helper function to remove duplication.

Eugene.Zelenko added a subscriber: Eugene.Zelenko.

@carlosgalvezp: Sorry, there are too much Clang specifics in this patch, so I could not be reviewer.

@carlosgalvezp: Sorry, there are too much Clang specifics in this patch, so I could not be reviewer.

No prob, thanks for letting us know :)

PiotrZSL updated this revision to Diff 500523.Feb 26 2023, 12:56 AM
PiotrZSL marked 2 inline comments as done.

Review comments fix

carlosgalvezp accepted this revision.Feb 26 2023, 3:44 AM

LGTM, thanks!

This revision is now accepted and ready to land.Feb 26 2023, 3:44 AM
Henric added a subscriber: Henric.Mar 13 2023, 2:01 AM

With this patch, the following code gives me a warning that I don't think is intentional.

typedef struct S {
  int x:4;
} S;

void foo(S s) {
  for (int i=10; i > s.x; --i) ;

loop_var.c:6:22: warning: loop variable has narrower type 'int:4' than iteration's upper bound 'int' [bugprone-too-small-loop-variable]
for (int i=10; i > s.x; --i) ;

Looks like the logic i reversed here, so the loop variable gets the bit field type.

Thank you for information, I will look into this....
Indeed test for this scenario is missing.