[PATCH] D157210: [symbolizer] Change reaction on invalid input

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:195
     if (!BinaryName.empty() || !BuildID.empty())
-      // Input file has already been specified on the command line.
-      return false;
+      return makeStringError("input file is already specified");
     StringRef Name = getSpaceDelimitedWord(InputString);
----------------



================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:299
+          if (Interactive)
+            printUnknownLineInfo(ModuleName, Printer);
+        },
----------------
Does this really need to be conditional on interactive mode? It seems an unnecessary complication to me.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:198
     if (Name.empty())
-      // Wrong name for module file.
-      return false;
+      return makeStringError("input file name is incorrect");
     if (HasBuildIDPrefix) {
----------------
sepavloff wrote:
> MaskRay wrote:
> > `cannot be empty` seems better than `is incorrect`
> It is not for missing argument but for the case of unterminated strings:
> ```
> $ cat sym.inp
> FILE:"addr.exe 0x400540
> $ ./llvm-symbolizer <sym.inp
> $ ./llvm-symbolizer: error: 'FILE:"addr.exe 0x400540': input file name is incorrect
> ```
"is incorrect" doesn't seem right to me either. It doesn't tell the user how it is incorrect.

Also, I couldn't see a test case for this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157210/new/

https://reviews.llvm.org/D157210