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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


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


================
Comment at: ELF/Driver.cpp:888
+    if (!isValidCIdentifier(Expr))
+      error("--defsym=: alias expected, but got " + Expr);
+    Ret.push_back({Name, Expr});
----------------
ruiu wrote:
> Remove `=`.
> 
> Since we expect a symbol name, the error should be something like error("-defsym: symbol name expected, but got " + To);
Done.


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


================
Comment at: ELF/SymbolTable.cpp:176
+  if (!B) {
+    error("undefined symbol '" + Name + "' used in --defsym= expression");
+    return;
----------------
ruiu wrote:
> `=` 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);
Done.


https://reviews.llvm.org/D32171