[PATCH] D12737: Pass an optional "code" to EmitRecordWithAbbrevImpl()

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

> On 2015-Sep-09, at 14:28, Teresa Johnson <tejohnson at google.com> wrote:
> 
> On Wed, Sep 9, 2015 at 2:22 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>> 
>>> On 2015-Sep-09, at 13:01, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>>> 
>>> joker.eph created this revision.
>>> joker.eph added a reviewer: dexonsmith.
>>> joker.eph added a subscriber: llvm-commits.
>>> 
>>> This avoids having EmitRecord() shifts the Vals array to insert the
>>> code at the front.
>> 
>> This LGTM with a few minor changes below.
> 
> I already committed a variant of this (r247186).
> 

Too many cooks in the kitchen.

>> 
>>> 
>>> http://reviews.llvm.org/D12737
>>> 
>>> Files:
>>> include/llvm/Bitcode/BitstreamWriter.h
>>> 
>>> Index: include/llvm/Bitcode/BitstreamWriter.h
>>> ===================================================================
>>> --- include/llvm/Bitcode/BitstreamWriter.h
>>> +++ include/llvm/Bitcode/BitstreamWriter.h
>>> @@ -287,9 +287,12 @@
>>>  /// emission code.  If BlobData is non-null, then it specifies an array of
>>>  /// data that should be emitted as part of the Blob or Array operand that is
>>>  /// known to exist at the end of the record.
>>> +  /// An optional Code can be passed separately as a convenience to avoid
>>> +  /// having EmitRecord() performing a push_front on Vals.
>>>  template <typename uintty>
>>>  void EmitRecordWithAbbrevImpl(unsigned Abbrev, const ArrayRef<uintty> &Vals,
>> 
>> We usually pass ArrayRefs by-value; no need for `const` and `&`.
>> 
>>> -                                StringRef Blob) {
>>> +                                StringRef Blob,
>>> +                                Optional<uintty> Code = Optional<uintty>()) {
>> 
>> This can be `Optional<uintty> Code = None`.
> 
> I can make that change to my commit, since I am passing in None for
> the other callsites explictly right now.

I like the way you've done it; no harm in being explicit.

> 
>> 
>>>    const char *BlobData = Blob.data();
>>>    unsigned BlobLen = (unsigned) Blob.size();
>>>    unsigned AbbrevNo = Abbrev-bitc::FIRST_APPLICATION_ABBREV;
>>> @@ -299,8 +302,24 @@
>>>    EmitCode(Abbrev);
>>> 
>>>    unsigned RecordIdx = 0;
>>> -    for (unsigned i = 0, e = static_cast<unsigned>(Abbv->getNumOperandInfos());
>>> -         i != e; ++i) {
>>> +    unsigned i = 0, e = static_cast<unsigned>(Abbv->getNumOperandInfos());
>>> +
>>> +    // If a "Code" is passed in separately, emit it first.
>>> +    if (Code) {
>>> +      assert(e && "Expected non-empty abbreviation");
>>> +      const BitCodeAbbrevOp &Op = Abbv->getOperandInfo(i++);
>>> +      assert(!Op.getEncoding() == BitCodeAbbrevOp::Array &&
>>> +             !Op.getEncoding() == BitCodeAbbrevOp::Blob &&
>>> +             "Expected literal or scalar code");
> 
> This assert did not quite work and my commit fixes it (the "! ... =="
> changed to "!=" and also I nested this within the else below since
> getEncoding does not like to be called when isLiteral is true).
> 
>>> +
>>> +      if (Op.isLiteral())
>>> +        EmitAbbreviatedLiteral(Op, Code.getValue());
>>> +      else
>>> +        EmitAbbreviatedField(Op, Code.getValue());
>>> +    }
>>> +
>>> +    // Main loop, emitting all value in Vals.
>>> +    for (; i != e; ++i) {
>>>      const BitCodeAbbrevOp &Op = Abbv->getOperandInfo(i);
>>>      if (Op.isLiteral()) {
>>>        assert(RecordIdx < Vals.size() && "Invalid abbrev/record");
>>> @@ -396,10 +415,8 @@
>>>      return;
>>>    }
>>> 
>>> -    // Insert the code into Vals to treat it uniformly.
>>> -    Vals.insert(Vals.begin(), Code);
>>> -
>>> -    EmitRecordWithAbbrev(Abbrev, makeArrayRef(Vals));
>>> +    EmitRecordWithAbbrevImpl(Abbrev, makeArrayRef(Vals), StringRef(),
>> 
>> I like the paint colour `""` better than `StringRef()`.
>> 
>>> +                             Optional<uintty>(Code));
>> 
>> I think `Code` should implicitly convert to `Optional<uintty>`; no need
>> to call the constructor explicitly.
> 
> My committed patch doesn't use the cast. Note I had to use
> Optional<unsigned> instead of Optional<uintty> in
> EmitRecordWithAbbrevImpl (regardless of whether a cast is used here,
> it didn't compile correctly with uintty).
> 
>> 
>>>  }
>>> 
>>>  /// EmitRecordWithAbbrev - Emit a record with the specified abbreviation.
>>> 
>>> 
>>> <D12737.34361.patch>
>> 
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413