[PATCH] D58234: [llvm-objcopy] Add --add-symbol

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:237
+  if (Bind)
+    error("conflicting values for binding: " + getBindAsString(*Bind) +
+          " and " + getBindAsString(Value));
----------------
See my comment below about returning `Error` rather than calling `error`.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:295
+  if (Flags[0].getAsInteger(0, SI.Value))
+    error("bad symbol value: '" + Flags[0] + "'");
+
----------------
I think we're generally aiming to move away from `error`/`reportError` in favour of passing `Error`/`Expected` back up the stack to be handled at the top level. Please do that instead here and elsewhere where you've added new error messages. You might need to rebase on top of D58316.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:320
+                       [&Type] { setTypeOnce(Type, ELF::STT_GNU_IFUNC); })
+            .CaseLower("debug", [] {})
+            .CasesLower("constructor", "warning", "indirect", "synthetic",
----------------
Could this be folded in with the other ones below?


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

https://reviews.llvm.org/D58234