[PATCH] D63122: [llvm-strip] Error when using stdin twice

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:805
   } else {
-    for (const char *Filename : Positional) {
+    StringSet<> InputFiles;
+    for (StringRef Filename : Positional) {
----------------
abrachet wrote:
> I don't know why this is the case but the compiler was not content with just StringSet and asked if I meant StringRef. But adding the <> fixed this. 
What about using a `StringMap` to track the actual counts so you only report the first time a file is included multiple times?
```
StringMap<int> InputFileMap;
for (StringRef Filename : Positional)
  if (InputFileMap[Filename]++ == 1) {
    if (Filename == "-") return createStringError(...);
    reportWarning(...); // Only reported the first time we see a dupe file
  }
```


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:807
+    for (StringRef Filename : Positional) {
+      if (InputFiles.find(Filename) != InputFiles.end()) {
+        if (Filename == "-")
----------------
`llvm::is_contained(InputFiles, Filename)`


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:810
+          return createStringError(errc::invalid_argument, "cannot specify '-' as an input file more than once");
+        reportWarning("'" + Filename + "' was already specified");
+      }
----------------
jhenderson wrote:
> As you are planning on librarifying llvm-objcopy, be VERY wary about adding warnings directly in the code that might be eventually used in the library. If you haven't already, take a look at my lightning talk from last year's US Developers' meeting, which includes some tips and gotchas on this sort of thing:
> 
> https://www.youtube.com/watch?v=YSEY4pg1YB0
+1, if possible we should not be calling back into llvm-objcopy.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63122