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