[PATCH] D57738: [llvm-objcopy] Add --redefine-syms

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:268
+  BufOrErr.get()->getBuffer().split(Lines, '\n');
+  for (unsigned LineNo = 0; LineNo < Lines.size(); ++LineNo) {
+    auto TrimmedLine = Lines[LineNo].split('#').first.trim();
----------------
Nit: use size_t instead of unsigned to match the return type of .size(). Also, Lines.size() should be calculated out-of-line, I think, based on the coding standards (although it only talks about end() calls explicitly, so I could be wrong on this).


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:269
+  for (unsigned LineNo = 0; LineNo < Lines.size(); ++LineNo) {
+    auto TrimmedLine = Lines[LineNo].split('#').first.trim();
+    if (TrimmedLine.empty())
----------------
I don't think this should be auto. The type isn't obvious.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:273
+
+    auto Pair = Saver.save(TrimmedLine).split(' ');
+    StringRef NewName = Pair.second.trim();
----------------
Ditto.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:276-277
+    if (NewName.empty())
+      error(Filename + ":" + std::to_string(LineNo + 1) +
+              ": missing new symbol name");
+    SymbolsToRename.insert({Pair.first, NewName});
----------------
Rather than call `error` here, this should probably return an `llvm::Error` which is handled in the calling code.


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:83
+         "--redefine-sym=<old>=<new> is set for each one. <filename> "
+         "contains two symbol per line separated with whitespace and may "
+         "contain comments beginning with '#'. Leading and trailing "
----------------
jhenderson wrote:
> Nit: symbol -> symbols
This still needs doing.


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

https://reviews.llvm.org/D57738