[PATCH] D32171: [ELF] - Implemented --defsym option #2

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.
This revision now requires changes to proceed.

Generally looking good, a few nits.

Please remove "#2" from the patch title. It's not relevant for people who will be reading this patch after you submit this, as they don't know and don't care about how many you have tried before.



================
Comment at: ELF/Driver.cpp:886
+    StringRef Expr;
+    std::tie(Name, Expr) = StringRef(Arg->getValue()).split('=');
+    if (!isValidCIdentifier(Expr))
----------------
Expr is not an expr, but a symbol name, so Expr is not a good name. I'd name them From and To.


================
Comment at: ELF/Driver.cpp:888
+    if (!isValidCIdentifier(Expr))
+      error("--defsym=: alias expected, but got " + Expr);
+    Ret.push_back({Name, Expr});
----------------
Remove `=`.

Since we expect a symbol name, the error should be something like error("-defsym: symbol name expected, but got " + To);


================
Comment at: ELF/Options.td:28
 
+def defsym: J<"defsym=">, HelpText<"Create a global symbol alias">;
+
----------------
"Define a symbol alias"


================
Comment at: ELF/SymbolTable.cpp:176
+  if (!B) {
+    error("undefined symbol '" + Name + "' used in --defsym= expression");
+    return;
----------------
`=` is not part of the option name but a separator.

Also, we are not handling an expression, so don't use the word "expression".

Error message should be error("-defsym: undefined symbol: " + Name);


https://reviews.llvm.org/D32171