[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:56:07 PST 2018


*nod* I'd be OK with that. (personally I'd figure there are few enough
callers of this API that it probably won't be misused, but don't mind
adding the extra clarity in the name, for sure)

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

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


More information about the llvm-commits mailing list