[PATCH] D54879: Introduce `LocalAddressSpaceView::LoadWritable(...)` and make the `Load(...)` method return a const pointer.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_local_address_space_view.h:43
+  template <typename T>
+  static const T *Load(T *target_address, uptr num_elements = 1) {
+    // The target address space is the local address space so
----------------
vitalybuka wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > const T *target_addres?
> > > and then you maybe can remove Writable from the another name
> > @vitalybuka I considered this but this ends up putting the burden of giving `Load(...)` a const pointer on all callers.
> > 
> > This isn't good because it involves having to write a lot of `reinterpret_cast<const Something*>(...)` at the call sites. Given that getting a `const` pointer for enumeration of the allocator is the common case it makes more sense to force the caller to write the "longer" thing when we call site needs a non-const pointer (the uncommon case). So in that case the call-site uses `LoadWritable(...)` rather than `Load(...)`.  Does this make sense?
> I am not sure what is going to be implementation of these.
> Is there going to be any disadvantages of non-const vs const implementations?
Could you please respond to one above?


================
Comment at: lib/sanitizer_common/sanitizer_local_address_space_view.h:43
+  template <typename T>
+  static const T *Load(T *target_address, uptr num_elements = 1) {
+    // The target address space is the local address space so
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > delcypher wrote:
> > > vitalybuka wrote:
> > > > const T *target_addres?
> > > > and then you maybe can remove Writable from the another name
> > > @vitalybuka I considered this but this ends up putting the burden of giving `Load(...)` a const pointer on all callers.
> > > 
> > > This isn't good because it involves having to write a lot of `reinterpret_cast<const Something*>(...)` at the call sites. Given that getting a `const` pointer for enumeration of the allocator is the common case it makes more sense to force the caller to write the "longer" thing when we call site needs a non-const pointer (the uncommon case). So in that case the call-site uses `LoadWritable(...)` rather than `Load(...)`.  Does this make sense?
> > I am not sure what is going to be implementation of these.
> > Is there going to be any disadvantages of non-const vs const implementations?
> Could you please respond to one above?
input arg should be const?
```
static const T *Load(const T *target_address, uptr num_elements = 1)
```


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D54879