[PATCH] MachineFunction is exposed to X86AsmParser.

Rafael Espíndola rafael.espindola at gmail.com
Tue Apr 22 11:06:35 PDT 2014


The phabricator login doesn't seem to be working, so I will just put
the comments on the email.

setMCTargetOptions is now unused, please delete it.
onMCTargetOptionsSet is also unused, please delete it.

AsmPrinterInlineAsm.cpp:147 is now just a white space change now.


 /// Current MCTargetOptions.
 MCTargetOptions MCOptions;


Without a set method the comment is out of date.

Nit: maybe even better than a copy would be a const reference if that
is possible.

LGTM with that.


On 22 April 2014 13:10, Yuri Gorshenin <ygorshenin at chromium.org> wrote:
> PTAL
>
> ================
> Comment at: include/llvm/MC/MCTargetAsmParser.h:102
> @@ +101,3 @@
> +  /// Current MCTargetOptions.
> +  const MCTargetOptions *MCOptions;
> +
> ----------------
> Rafael Ávila de Espíndola wrote:
>> The class small and would be easy to copy. The existence of the onMCTargetOptionSet also suggests value semantics, don't you want to store just a "MCTargetOptions MCOptions"?
>>
> Done.
>
> ================
> Comment at: include/llvm/MC/MCTargetAsmParser.h:122
> @@ +121,3 @@
> +
> +  virtual void onMCTargetOptionsSet() {}
> +
> ----------------
> Rafael Ávila de Espíndola wrote:
>> This can be private. Even better, can't you also just remove setMCTargetOptions method completely and add an argument to createMCAsmParser?
>>
> Done, but it required changes for all target parsers.
>
> http://reviews.llvm.org/D3106
>
> REPLY HANDLER ACTIONS
>   Reply to comment, or !reject, !abandon, !reclaim, !resign, !rethink, !unsubscribe.
>
>




More information about the llvm-commits mailing list