[PATCH] D61049: Let llvm-cvtres (and lld-link) report duplicate resources

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/test/tools/llvm-cvtres/duplicate.test:11
+RUN: cp %S/Inputs/id.res %t.dir/id2.res
+RUN: not llvm-cvtres /machine:X86 %t.dir/id1.res %t.dir/id2.res 2>&1 | \
+RUN:     FileCheck -check-prefix=ID %s
----------------
thakis wrote:
> mstorsjo wrote:
> > What output file does this write into? A temporary file that is removed afterwards - in what directory? (Would it be better to pass a named `-out:%t.obj` to make sure it doesn't do stray writes outside of the designated directory?)
> The default output name is the name of the first input file, with .res replaced with .obj, so that's in %t.dir already. Furthermore, the output file is only created once all input files have been parsed successfully, and parsing input files fails here. I can add an explicit `out:` flag if you think it makes the test easier to read, but it doesn't have an effect.
Ok, so it's not an issue. Go ahead whichever way you prefer then, it doesn't really hurt this way (and the risk of it becoming an issue later is also minimal), it just came to my mind while reviewing it.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:105
   if (InputArgs.hasArg(OPT_HELP)) {
-    T.PrintHelp(outs(), "llvm-cvtres [options] file...", "Resource Converter", false);
+    T.PrintHelp(outs(), "llvm-cvtres [options] file...", "Resource Converter");
     return 0;
----------------
thakis wrote:
> mstorsjo wrote:
> > What does this particular changed line do, with respect to the rest of the patch?
> It's unrelated to the patch. I had read this file while working on this patch, wondered what the `false` meant, opened the .h file that declares this function to add a `/*ArgName=*/` comment, and then realized that false is the default argument for this parameter already, so I figured I'd just remove it. I can land this in a separate commit if you want.
Yes, please do so, it's quite distracting/confusing here.


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

https://reviews.llvm.org/D61049