llvm dev - Nov 2016 - RFC [Bitcode]: Moving block info block state

Hi all,

This is about https://reviews.llvm.org/D26100

That change moves the block info block state from BitstreamReader to
BitstreamCursor in order to accommodate multiple block info blocks (the
idea is that the cursor would store the block info block state for
whichever block info block is active for that cursor).

Duncan objected to it on the grounds that we should aim for a design that
would continue to allow a shared block info block state. I'd agree with him
that such a design would be possible while still allowing multiple block
info blocks; the design I have in mind would decouple the block info state
from not only the cursor but the reader. The interface would look like this:

class BitstreamBlockInfo {
  // block info block state, i.e. everything matching *BlockInfo* in the
current BitstreamReader interface
};

class BitstreamCursor : SimpleBitstreamCursor {
  // existing BitstreamCursor interface here

  /// Read a block info block into a BitstreamBlockInfo object, and return
it.
  BitstreamBlockInfo ReadBlockInfo();

  /// Set the block info to be used by this BitstreamCursor to interpret
abbreviated records.
  void setBlockInfo(BitstreamBlockInfo *Info);
};

What I like about this design is that it is more explicit than either the
status quo or D26100, but I'd like feedback on it first.

Thanks,
-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161028/4cc9c1a1/attachment.html>

> On Oct 28, 2016, at 6:00 PM, Peter Collingbourne <peter at pcc.me.uk>
wrote:
> 
> Hi all,
> 
> This is about https://reviews.llvm.org/D26100
<https://reviews.llvm.org/D26100>
> 
> That change moves the block info block state from BitstreamReader to
BitstreamCursor in order to accommodate multiple block info blocks (the idea is
that the cursor would store the block info block state for whichever block info
block is active for that cursor).
> 
> Duncan objected to it on the grounds that we should aim for a design that
would continue to allow a shared block info block state. I'd agree with him
that such a design would be possible while still allowing multiple block info
blocks; the design I have in mind would decouple the block info state from not
only the cursor but the reader. The interface would look like this:
> 
> class BitstreamBlockInfo {
>   // block info block state, i.e. everything matching *BlockInfo* in the
current BitstreamReader interface
> };
> 
> class BitstreamCursor : SimpleBitstreamCursor {
>   // existing BitstreamCursor interface here
> 
>   /// Read a block info block into a BitstreamBlockInfo object, and return
it.
>   BitstreamBlockInfo ReadBlockInfo();
> 
>   /// Set the block info to be used by this BitstreamCursor to interpret
abbreviated records.
>   void setBlockInfo(BitstreamBlockInfo *Info);
> };
> 
> What I like about this design is that it is more explicit than either the
status quo or D26100, but I'd like feedback on it first.
That’s fine with me.

— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161031/904ca5b4/attachment.html>

> On 2016-Oct-31, at 20:30, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 
>> 
>> On Oct 28, 2016, at 6:00 PM, Peter Collingbourne <peter at
pcc.me.uk> wrote:
>> 
>> Hi all,
>> 
>> This is about https://reviews.llvm.org/D26100
>> 
>> That change moves the block info block state from BitstreamReader to
BitstreamCursor in order to accommodate multiple block info blocks (the idea is
that the cursor would store the block info block state for whichever block info
block is active for that cursor).
>> 
>> Duncan objected to it on the grounds that we should aim for a design
that would continue to allow a shared block info block state. I'd agree with
him that such a design would be possible while still allowing multiple block
info blocks; the design I have in mind would decouple the block info state from
not only the cursor but the reader. The interface would look like this:
>> 
>> class BitstreamBlockInfo {
>>   // block info block state, i.e. everything matching *BlockInfo* in
the current BitstreamReader interface
>> };
>> 
>> class BitstreamCursor : SimpleBitstreamCursor {
>>   // existing BitstreamCursor interface here
>> 
>>   /// Read a block info block into a BitstreamBlockInfo object, and
return it.
>>   BitstreamBlockInfo ReadBlockInfo();
>> 
>>   /// Set the block info to be used by this BitstreamCursor to
interpret abbreviated records.
>>   void setBlockInfo(BitstreamBlockInfo *Info);
>> };
>> 
>> What I like about this design is that it is more explicit than either
the status quo or D26100, but I'd like feedback on it first.
> 
> That’s fine with me.
Might be worth waiting until after the dev meeting to be sure everyone has had a
chance to catch up to this, but this is fine with me too.