[PATCH] D78050: Introduce `__sanitizer::ArrayRef<T>` type.

Mon Apr 13 21:42:49 PDT 2020

delcypher marked an inline comment as done.
delcypher added a comment.

In D78050#1979455 <https://reviews.llvm.org/D78050#1979455>, @dblaikie wrote:

> While I'm in favor of this type looking more like llvm's MutableArrayRef (ie: allowing mutability or immutability to be encoded in the type parameter (const T versus non-const T)) rather than needing the two types LLVM has (ArrayRef + MutableArrayRef). I wonder if having this type called "ArrayRef" when it is subtly different from llvm's ArrayRef might be confusing?

Thank you for raising this. I probably should have said "loosely inspired" by ArrayRef because I wasn't aware of MutableArrayRef and I hadn't noticed that LLVM ADT's ArrayRef was immutable. Given that LLVM's ADT ArrayRef is immutable my choice of naming would certainly be confusing. For my use case I actually only need the underlying data to be mutable so I don't need an immutable version.

> Wonder if it needs another/better name - though the standard doesn't offer us a great alternative, it seems the standardized equivalent is std::span, which might be sufficiently different not to be a good name to use either? Not sure.

My early draft name was actually `BufferRef<T>`. Does that seem any better?

================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_array_ref.h:87
+    CHECK_LE(n, size());
+    T *slice_begin = const_cast<T *>(begin()) + n;
+    uptr slice_size = size() - n;
----------------
dblaikie wrote:
> Might just be better/easier to use "data_" directly, rather than const_casting+begin?
> 
> Or use "data()" instead, like llvm::ArrayRef does?
Good idea.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78050