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

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 17 10:50:01 PST 2018


On Wed, Jan 17, 2018 at 10:44 AM Adrian Prantl <aprantl at apple.com> wrote:

>
> 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> 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. :/

- 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180117/5ca9ffbc/attachment.html>


More information about the llvm-commits mailing list