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

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

> 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.

> 
> 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`.

>     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");
> +
> +      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.

>   }
> 
>   /// EmitRecordWithAbbrev - Emit a record with the specified abbreviation.
> 
> 
> <D12737.34361.patch>