<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 13, 2014 at 4:39 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><br>
> On 2014 Nov 12, at 16:55, Keno Fischer <<a href="mailto:kfischer@college.harvard.edu" target="_blank">kfischer@college.harvard.edu</a>> wrote:<br>
><br>
> Hi dexonsmith,<br>
><br>
> In some instances (such as in BitCodeWriter - see <a href="http://reviews.llvm.org/D6184" target="_blank">http://reviews.llvm.org/D6184</a>),<br>
> it is useful to get an unsiged version of the data pointer instead. This adds an<br>
> appropriate API to be used in BitCodeWriter in a follow up commit.<br>
><br>
> <a href="http://reviews.llvm.org/D6241" target="_blank">http://reviews.llvm.org/D6241</a><br>
><br>
> Files:<br>
> include/llvm/ADT/StringRef.h<br>
> include/llvm/IR/Metadata.h<br>
</div></div>> <D6241.16125.patch><br>
<br>
This LGTM. I have minor nitpicks below that you can take or leave.<br>
<br>
@David, does this API make sense to you given Keno's use case [1]?<br></blockquote><div><br>While it matches that use case quite well (needing iterator begin/end) there are many other cases where we play the same game - getting uint8_t rather than just unsigned char:<br><br><div>lib/Support/DataExtractor.cpp: return getU<uint8_t>(offset_ptr, this, IsLittleEndian, Data.data());</div><div>lib/Support/MD5.cpp: const uint8_t *Ptr = Data.data();</div><div>lib/Support/MD5.cpp: ArrayRef<uint8_t> SVal((const uint8_t *)Str.data(), Str.size());</div><div>lib/IR/Constants.cpp: const uint8_t *Data = reinterpret_cast<const uint8_t *>(Str.data());</div><div>lib/ProfileData/CoverageMappingReader.cpp: Result = decodeULEB128(reinterpret_cast<const uint8_t *>(Data.data()), &N);</div><div>lib/ProfileData/CoverageMappingReader.cpp: decodeULEB128(reinterpret_cast<const uint8_t *>(Data.data()), &N);</div><div>lib/ProfileData/CoverageMappingReader.cpp: decodeULEB128(reinterpret_cast<const uint8_t *>(Data.data()), &N);</div><div>lib/ExecutionEngine/RuntimeDyld/RuntimeDyldChecker.cpp: reinterpret_cast<const uint8_t *>(SectionMem.data()),</div><div>lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp: uint8_t *ArgDataPtr = ArgData.data();</div><div>lib/DebugInfo/DWARFFormValue.cpp: Value.data = reinterpret_cast<const uint8_t *>(str.data());</div><div>include/llvm/Support/FileOutputBuffer.h: return (uint8_t*)Region->data();</div><div>include/llvm/Support/FileOutputBuffer.h: return (uint8_t*)Region->data() + Region->size();</div><div>include/llvm/Object/ELF.h: return reinterpret_cast<const uint8_t *>(Buf.data());</div><div>include/llvm/MC/YAML.h: : Data(reinterpret_cast<const uint8_t *>(Data.data()), Data.size()),<br><div>tools/llvm-objdump/llvm-objdump.cpp: ArrayRef<uint8_t> Bytes(reinterpret_cast<const uint8_t *>(BytesStr.data()),</div><div>tools/llvm-objdump/MachODump.cpp: ArrayRef<uint8_t> Bytes(reinterpret_cast<const uint8_t *>(BytesStr.data()),</div><div>tools/llvm-mc/Disassembler.cpp: ArrayRef<uint8_t> Data(Bytes.first.data(), Bytes.first.size());</div><div>tools/llvm-readobj/StreamWriter.h: auto V = makeArrayRef(reinterpret_cast<const uint8_t*>(Value.data()),</div><div>tools/llvm-readobj/StreamWriter.h: auto V = makeArrayRef(reinterpret_cast<const uint8_t*>(Value.data()),</div><div>tools/llvm-readobj/StreamWriter.h: auto V = makeArrayRef(reinterpret_cast<const uint8_t*>(Value.data()),</div><div>tools/llvm-readobj/StreamWriter.h: auto V = makeArrayRef(reinterpret_cast<const uint8_t*>(Value.data()),</div><br>In several of these cases we're interested in getting an ArrayRef<uint8_t> from the StringRef - so maybe that's a better primitive operation (it's annoying when interoperating with containers, I'll grant you - since you need to call it twice, or stash a local variable - but one day... we'll have range algorithms for this sort of thing - in the mean time we could have utility functions like those in this patch, but I think the primary operation should probably be "give me a raw (uint8_t) byte array for this StringRef")<br><br>Also see Aaron's recent -Wcast-qual fixing patches that touched several cases like this: <br><a href="http://llvm.org/viewvc/llvm-project?view=revision&revision=221782">http://llvm.org/viewvc/llvm-project?view=revision&revision=221782</a><br><a href="http://llvm.org/viewvc/llvm-project?view=revision&revision=221781">http://llvm.org/viewvc/llvm-project?view=revision&revision=221781</a><br></div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
[1]: <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20141103/243658.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20141103/243658.html</a><br>
<br>
> Index: include/llvm/ADT/StringRef.h<br>
> ===================================================================<br>
> --- include/llvm/ADT/StringRef.h<br>
> +++ include/llvm/ADT/StringRef.h<br>
> @@ -40,6 +40,7 @@<br>
> class StringRef {<br>
> public:<br>
> typedef const char *iterator;<br>
> + typedef unsigned const char *uiterator;<br>
<br>
This spelling is strange to me; I'd expect:<br>
<br>
typedef const unsigned char *uiterator;<br>
<br>
> typedef const char *const_iterator;<br>
> static const size_t npos = ~size_t(0);<br>
> typedef size_t size_type;<br>
> @@ -88,8 +89,12 @@<br>
> /// @{<br>
><br>
> iterator begin() const { return Data; }<br>
> + uiterator ubegin() const { return reinterpret_cast<uiterator>(Data); }<br>
<br>
I have a slight preference for:<br>
<br>
uiterator ubegin() const { return reinterpret_cast<uiterator>(begin()); }<br>
<br>
But mainly just to match my change to `uend()` below.<br>
<br>
><br>
> iterator end() const { return Data + Length; }<br>
> + uiterator uend() const {<br>
> + return reinterpret_cast<uiterator>(Data) + Length;<br>
> + }<br>
<br>
I think this should be:<br>
<br>
uiterator uend() const { return reinterpret_cast<uiterator>(end()); }<br>
<br>
><br>
> /// @}<br>
> /// @name String Operations<br>
> Index: include/llvm/IR/Metadata.h<br>
> ===================================================================<br>
> --- include/llvm/IR/Metadata.h<br>
> +++ include/llvm/IR/Metadata.h<br>
> @@ -55,12 +55,15 @@<br>
> unsigned getLength() const { return (unsigned)getName().size(); }<br>
><br>
> typedef StringRef::iterator iterator;<br>
> + typedef StringRef::uiterator uiterator;<br>
><br>
> /// \brief Pointer to the first byte of the string.<br>
> iterator begin() const { return getName().begin(); }<br>
> + uiterator ubegin() const { return getName().ubegin(); }<br>
><br>
> /// \brief Pointer to one byte past the end of the string.<br>
> iterator end() const { return getName().end(); }<br>
> + uiterator uend() const { return getName().uend(); }<br>
><br>
> /// \brief Methods for support type inquiry through isa, cast, and dyn_cast.<br>
> static bool classof(const Value *V) {<br>
><br>
<br>
</blockquote></div><br></div></div>