aaron.ballman added subscribers: hubert.reinterpretcast, rsmith.
aaron.ballman added inline comments.
================
Comment at: clang/lib/AST/Interp/Descriptor.cpp:42
const Descriptor *D) {
+ new (Ptr) InitMapPtr(std::nullopt);
+
----------------
This worries me a little bit for a few reasons, but it might be okay:
* What validates that the bytes pointed to by `Ptr` are aligned properly for an
`InitMapPtr` object? Perhaps we need an `alignas` in the function signature to
ensure correct alignment of those bytes?
* `InitMapPtr` is `std::optional<std::pair<bool, std::shared_ptr<InitMap>>>`
and I *think* using placement new will ensure we have correct objects in all
the expected places, but I'm not 100% sure because we're calling the `nullopt`
constructor here.
* I *think* it is correct that you are not assigning the result of the
placement `new` expression into anything; and I think we need this placement
`new` because of the `reinterpret_cast` happening in `dtorArrayTy()`. But it is
a bit strange to see the placement `new` hanging off on its own like this.
Might be worth some comments explaining.
CC @hubert.reinterpretcast @rsmith in case my assessment is incorrect.
================
Comment at: clang/lib/AST/Interp/Descriptor.h:203
+public:
/// Initializes the map with no fields set.
----------------
This interface is now more confusing as to how to use because it now has a
public non-default constructor *and* an `initialize` method. Perhaps we should
rename `initialize` to `initializeMapElement` so it's clear that the
`initialize` method isn't for the map itself?
================
Comment at: clang/lib/AST/Interp/Descriptor.h:219
+ static constexpr size_t numFields(unsigned N) {
+ return ((N + PER_FIELD - 1) / PER_FIELD);
+ }
----------------
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154581/new/
https://reviews.llvm.org/D154581
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
