[PATCH] D26100: Bitcode: Maintain block info block abbreviations in the BitstreamCursor.

[Duncan P. N. Exon Smith via llvm-commits]

Okay, I'm less opposed to this now that I understand it.  Not entirely convinced though.

IIUC, BitstreamReader previously served as a shared cache for the BitstreamCursors.  You're moving the cache into each BitstreamCursor instance.  This will cause increased work in use cases that have multiple BitstreamCursors, but somehow it enables multiple-block-info-per-block.

I still think it merits a discussion on llvm-dev.  I'm not (yet) convinced that (1) multiple-block-info-per-block is incompatible with a shared cache and/or (2) multiple-block-info-per-block is valuable enough to be worth losing the shared cache.  I haven't thought deeply, so maybe you'll convince me easily enough; but the design decision seems worth discussing with a wider audience, since the bitstream reader has pretty broad usage (e.g., PCHs, serialized diagnostics, etc.).

> On 2016-Oct-28, at 16:51, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Heh, bad memory; thanks.  Let me have another look.
> 
>> On 2016-Oct-28, at 16:50, Mehdi Amini <mehdi.amini at apple.com> wrote:
>> 
>> You extracted it in “SimpleBitstreamCursor”. The BitstreamCursor inherit from it and is documented as:
>> 
>> `Unlike iterators, BitstreamCursors are heavy-weight objects that should not be passed by value.`
>> 
>> 
>>> On Oct 28, 2016, at 4:49 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> 
>>> IIRC, I've added code that takes advantage of BitstreamCursor being a lightweight object.  This seemed like a nice advantage.
>>> 
>>> It seems worth discussing this design change on llvm-dev (or, did I miss one there that you can point me at?).
>>> 
>>>> On 2016-Oct-28, at 16:38, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>>>> 
>>>> mehdi_amini added a comment.
>>>> 
>>>> I think this is basically like "merging" responsibility from the bitstream reader and the cursor into the cursor. The cursor isn't just a "position" in the stream with the "bitstreamreader" in charge of keeping the "context".
>>>> If this is correct, I'm not against it but at least you should reflect it in the class documentation.
>>>> 
>>>> Also added Duncan for an extra opinion here.
>>>> 
>>>> 
>>>> https://reviews.llvm.org/D26100
>>>> 
>>>> 
>>>> 
>>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits