[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