[PATCH] D35333: Create empty shell of llvm-mt.
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 17 12:19:46 PDT 2017
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);
----------------
ruiu wrote:
> 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.
Please fix.
================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:71
+
+int main(int argc_, const char *argv_[]) {
+ sys::PrintStackTraceOnErrorSignal(argv_[0]);
----------------
ruiu wrote:
> 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`.
Please fix.
================
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";
----------------
ruiu wrote:
> Error messages should start with lowercase letter.
Shouldn't end with a full stop.
https://reviews.llvm.org/D35333
More information about the llvm-commits
mailing list