[PATCH] D41695: [Metadata] Extend 'count' field of DISubrange to take a metadata node

[Adrian Prantl via llvm-commits]

> On Jan 17, 2018, at 10:20 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Jan 17, 2018 at 10:12 AM Adrian Prantl via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> aprantl added a comment.
> 
> It looks like this changes the layout of DISubrange from
> 
> Flags | Count |
> 
> 
> 
> ================
> Comment at: include/llvm/IR/DebugInfoMetadata.h:372
> +
> +  ConstantInt *getCount() const {
> +    if (auto *MD = dyn_cast<ConstantAsMetadata>(getRawCountNode()))
> ----------------
> Perhaps:
> `Optional<ConstantInt *> getConstantCount()` ?
> 
> FWIW (& opinions vary, this isn't something that's clearly one way or the other I think) - I generally use an existing empty state if there is one, like nullptr. Adding an extra empty state seems likely to be confusing - if I see Optional<T*> then I assume there's None, null, and the non-null values - and have to think about what the difference might be between None and null.

That's a good point — do you think Optional<ConstantInt &> would be better, or is this just being silly? :-)
I just would like to make it very obvious when we have to expect a nullptr from an API, since LLVM tends to also vend lots of guaranteed nonnull pointers (which I feel should really be references), too.

-- adrian

>  
> 
> 
> ================
> Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1445
>    Record.push_back(N->isDistinct());
> -  Record.push_back(N->getCount());
> +  Record.push_back(-1); // this field has become obsolete
> +  Record.push_back(VE.getMetadataOrNullID(N->getRawCountNode()));
> ----------------
> We typically version bitcode by adding a bit to the flags, like this:
> ` Record.push_back(N->isDistinct() | 1<<1);`
> This way we don't have to waste the space for the obsoleted field.
> 
> 
> https://reviews.llvm.org/D41695 <https://reviews.llvm.org/D41695>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180117/b8c1f506/attachment.html>