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

Sean Silva chisophugis at gmail.com
Fri Jun 19 16:40:51 PDT 2015


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


================
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.

http://reviews.llvm.org/D10521

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list