[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71973#1817941 <https://reviews.llvm.org/D71973#1817941>, @gbencze wrote:

> Another option that came to my mind is using a BitVector to (recursively) flag bits that are occupied by the fields. This solution would be slightly slower and uses more memory but is probably a lot easier to understand, maintain and more robust than the currently proposed implementation.  This would also catch a few additional cases as it could also look inside unions.
>
> I stared to experiment with an implementation of this here <https://github.com/gaborbencze/llvm-project/commit/689bba792b5699444674a1cadc0e235b668b2970#diff-b476912022a94429d868e13a139cf406>.
>
> Which direction do you think would be a better solution?


This is a neat approach, but I think I still prefer asking the RecordLayout for this information if possible.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:82-83
+
+static bool hasPadding(const ASTContext &Ctx, const RecordDecl *RD,
+                       uint64_t ComparedBits) {
+  assert(RD && RD->isCompleteDefinition() &&
----------------
gbencze wrote:
> aaron.ballman wrote:
> > I am surprised that we have to do this much manual work, I would have expected this to be something that `RecordLayout` would handle for us. I am a bit worried about this manual work being split in a few places because we're likely to get it wrong in at least one of them. For instance, this doesn't seem to be taking into account things like `[[no_unique_address]]` or alignment when considering the layout of fields.
> > 
> > I sort of wonder if we should try using the `RecordLayout` class to do this work instead, even if that requires larger changes.
> Thanks, I didn't realize the [[no_unique_address]] attribute existed but I fixed it (in this check for now) and also added some tests to cover it as well as alignas. 
> 
> If you think this information should be provided by RecordLayout I'm willing to work on that as well (though I guess those changes should be in a different patch). Do you have any ideas as to how it should expose the necessary information? 
I do think this should be provided by `RecordLayout` (and yes, please as a separate patch -- feel free to add myself and Richard Smith as reviewers for it). Intuitively, I would expect to be able to query for whether a record has any padding within it whatsoever. For your needs, I would imagine an interface like `bool hasPaddingWithin(CharUnits InitialOffset, CharUnits FinalOffset)` would also help -- though I suspect we may not want to use `CharUnits` because of bit-fields that may have padding due to 0-sized bit-fields.


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

https://reviews.llvm.org/D71973