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

[Adrian Prantl via llvm-commits]

> On Jan 17, 2018, at 10:50 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Jan 17, 2018 at 10:44 AM Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
> 
>> On Jan 17, 2018, at 10:20 AM, David Blaikie <dblaikie at gmail.com <mailto: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 do sort of like it, really - but I'm not sure it works. I forget if we made references work in llvm::Optional, and I don't think std::optional supports references anyway - so probably not a good path to go down.
>  
> 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.
> 
> Yep - that's the bit that makes it tricky (judgement call, how obvious is it from usage/naming/etc that this might be null, etc), for sure. :/

Okay, here is a pragmatic solution then:

  ConstantInt *getConstantCountOrNull()

-- adrian

> 
> - Dave
>  
> 
> -- 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/b0d2172b/attachment.html>