[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

[Eli Bendersky]

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.

Eli