[PATCH] D41695: [Metadata] Extend 'count' field of DISubrange to take a metadata node
Adrian Prantl via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 17 10:54:54 PST 2018
> 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>
More information about the llvm-commits
mailing list