[PATCH] D98582: [llvm-objcopy][NFC] remove split dwo file creation from executeObjcopyOnBinary.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:329
 
-  using ProcessRawFn = Error (*)(CopyConfig &, MemoryBuffer &, raw_ostream &);
-  ProcessRawFn ProcessRaw;
-  switch (Config.InputFormat) {
-  case FileFormat::Binary:
-    ProcessRaw = executeObjcopyOnRawBinary;
-    break;
-  case FileFormat::IHex:
-    ProcessRaw = executeObjcopyOnIHex;
-    break;
-  default:
-    ProcessRaw = nullptr;
-  }
+  std::function<Error(raw_ostream & OutFile)> ObjcopyFunc = nullptr;
+
----------------
You don't need the `= nullptr` here, I believe.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:347-351
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+        MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
+    if (!BufOrErr)
+      return createFileError(Config.InputFilename, BufOrErr.getError());
+    MemoryBufferHolder = std::move(*BufOrErr);
----------------
This code duplication is a real step backwards from what was there before. I think it would be far better to share the code for ihex and binary, with only a very localised if, probably inside the lambda, to choose the right function to run. Possible example:

```
if (Config.InputFormat == FileFormat::Binary || Config.InputFormat == FileFormat::IHex) {
    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
        MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
    if (!BufOrErr)
      return createFileError(Config.InputFilename, BufOrErr.getError());
    MemoryBufferHolder = std::move(*BufOrErr);
    ObjcopyFunc = [&](raw_ostream &OutFile) -> Error {
      switch(Config.InputFormat) {
      case FileFormat::Binary:
        return executeObjcopyOnRawBinary(Config, *MemoryBufferHolder, OutFile);
      case FileFormat::IHex:
        return executeObjcopyOnIHex(Config, *MemoryBufferHolder, OutFile);
    };
} else {
  ...
}
```


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:380-381
+              writeToFile(Config.OutputFilename, ObjcopyFunc,
+                          Config.InputFilename != "-" &&
+                              Config.InputFilename == Config.OutputFilename,
+                          Stat.getUser(), Stat.getGroup()))
----------------
I think this should be pulled out into a helper variable, since it's repeated in each instance.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:385
+    } else {
+      Config.ExtractDWO = true;
+      Config.StripDWO = false;
----------------
A pair of short comments at each of this and the following stage would be helpful to show which part is doing what.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98582