[llvm-commits] [llvm] r168587 - in /llvm/trunk: include/llvm/MC/MCInstBuilder.h lib/Target/ARM/ARMAsmPrinter.cpp lib/Target/PowerPC/PPCAsmPrinter.cpp lib/Target/X86/X86MCInstLower.cpp

[Benjamin Kramer]

On 26.11.2012, at 14:43, Eli Bendersky <eliben at google.com> wrote:

> On Mon, Nov 26, 2012 at 5:34 AM, Benjamin Kramer
> <benny.kra at googlemail.com> wrote:
>> Author: d0k
>> Date: Mon Nov 26 07:34:22 2012
>> New Revision: 168587
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=168587&view=rev
>> Log:
>> Add MCInstBuilder, a utility class to simplify MCInst creation similar to MachineInstrBuilder.
>> 
>> Simplify some repetitive code with it. No functionality change.
>> 
> 
> This is great! Just a small comment below.
> 
>> +  /// \brief Emit the built instruction to an MCStreamer.
>> +  void emit(MCStreamer &OutStreamer) {
>> +    OutStreamer.EmitInstruction(Inst);
>> +  }
>> +};
>> +
> 
> I would really prefer to separate the "builder" from emission. This
> method doesn't save a lot of typing and doesn't logically belong in a
> builder class. One of the manifestations is inconsistency - it not
> always makes sense to emit the MCInst right after building, so in some
> cases it will be emitted with this method, and in others with just
> calling OutStreamer.EmitInstruction.

I thought about that, but I was unsure how to represent in the least ugly way. The options I see are

- Give the builders names, and then pass it to EmitInstruction, which brings back the problem of too many variables names TmpInst.
- OutStreamer.EmitInstruction(MCInstBuilder(...).addFoo().addBar());

Any preferences?

- Ben