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

[Nico Weber via Phabricator via llvm-commits]

thakis marked 2 inline comments as done.
thakis added a comment.

Thanks! Please let me know what to do about the two comments below :)



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


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


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

https://reviews.llvm.org/D61049