[PATCH] MIR Parsing: Introduce a MI Lexing class.

[Alex L]

2015-06-19 16:40 GMT-07:00 Sean Silva <chisophugis at gmail.com>:

> Overall, this LGTM. A couple small suggestions in case performance is ever
> a problem here.
>
>
> REPOSITORY
>   rL LLVM
>
> ================
> Comment at: lib/CodeGen/MIRParser/MILexer.h:58-60
> @@ +57,5 @@
> +/// the remaining source string.
> +StringRef lexMIToken(
> +    StringRef Source, MIToken &Token,
> +    std::function<void(StringRef::iterator, const Twine &)>
> ErrorCallback);
> +
> ----------------
> We probably don't want to be passing a std::function here by value.
> Probably just use a reference to std::function or llvm::function_ref
> http://llvm.org/docs/ProgrammersManual.html#the-function-ref-class-template
>
>
Thanks, llvm::function_ref is perfect here.


>
> ================
> Comment at: lib/CodeGen/MIRParser/MIParser.cpp:74-76
> @@ +73,5 @@
> +void MIParser::lex() {
> +  CurrentSource = lexMIToken(
> +      CurrentSource, Token,
> +      [this](StringRef::iterator Loc, const Twine &Msg) { error(Loc,
> Msg); });
> +}
> ----------------
> Could you doublecheck that the compiler is able to optimize the
> materialization of the std::function here? If it isn't doing a good job,
> then it might be better to just construct the std::function once in the
> MIParser constructor and store it as a member.
>

The llvm::function_ref materialization is optimized out very effectively by
the compiler during the optimized build.


>
> http://reviews.llvm.org/D10521
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150619/393dfcf8/attachment.html>