[PATCH] D35333: Create empty shell of llvm-mt.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:66
+LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg) {
+  errs() << "llvm-mt error: " << Msg;
+  exit(1);
----------------
ecbeckmann wrote:
> ruiu wrote:
> > ruiu wrote:
> > > It is convenient to write out "\n" here, instead of embedding "\n" as part of Msg.
> > It's not done.
> hmmm in most recent revision it should be done
Error messages shouldn't end with a full stop. See https://clang.llvm.org/diagnostics.html for example.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:71
+
+int main(int argc_, const char *argv_[]) {
+  sys::PrintStackTraceOnErrorSignal(argv_[0]);
----------------
ecbeckmann wrote:
> ruiu wrote:
> > ruiu wrote:
> > > Please follow the LLVM naming convention.
> > `argc` and `argv` still don't follow the LLVM naming convention.
> on most recent revision this should be fine
Is it? `argc` and `argv` should be `Argc` and `Argv`.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:48
+               HELPTEXT, METAVAR, VALUES)                                      \
+{                                                                            \
+      PREFIX,      NAME,      HELPTEXT,                                        \
----------------
Align the trailing `\`.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:90
+    if (Arg->getOption().matches(OPT_unsupported)) {
+      outs() << "Ignoring '" << Arg->getOption().getName()
+             << "' option: not supported.\n";
----------------
Error messages should start with lowercase letter.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:103
+  if (InputFiles.size() == 0) {
+    reportError("No input file specified");
+  }
----------------
Ditto


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:113
+  } else {
+    reportError("No output file specified");
+  }
----------------
Ditto


https://reviews.llvm.org/D35333