[PATCH] D154581: [clang][Interp] Track existing InitMaps in InterpState

[Aaron Ballman via Phabricator via cfe-commits]

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