[PATCH] D35619: [ELF] - Introduce multiclass Eq helper for Options.td

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Driver.cpp:808
     switch (Arg->getOption().getID()) {
-    case OPT_l:
+    case OPT_alias_library:
+    case OPT_library:
----------------
ruiu wrote:
> Rebase and remove this line.
Done.


================
Comment at: ELF/Options.td:10
 
+multiclass JSEq<string name, string help> {
+  def "" : S<name>, HelpText<help>;
----------------
ruiu wrote:
> Is JSEq short for Joined-Separate-EQual? That doesn't make much sense. I'd name this just `Eq`.
Done.


================
Comment at: ELF/Options.td:11
+multiclass JSEq<string name, string help> {
+  def "" : S<name>, HelpText<help>;
+  def _eq : J<name#"=">, Alias<!cast<S>(NAME)>;
----------------
ruiu wrote:
> Remove space before `:`.
> 
> Use the extended version as we don't need to save characters here.
> 
>   def "": Separate<["--", "-"], name>;
>   def _eq: Joined<["--", "-"], name # "=">, Alias<!cast<Separate>(name)>;
Done.


================
Comment at: ELF/Options.td:12
+  def "" : S<name>, HelpText<help>;
+  def _eq : J<name#"=">, Alias<!cast<S>(NAME)>;
+}
----------------
ruiu wrote:
> NAME -> name
'name' will not work here.
Because `name` is a variable passed, which is option name used 
for rendering, for example `library-path`. 
But `NAME` would be `library_path`, because `NAME` is the special entry,
it is a name of def record itself. 

See:
"Each def record has a special entry called “NAME”. This is the name of the record (“ADD32rr” above). In the general case def names can be formed from various kinds of string processing expressions and NAME resolves to the final value obtained after resolving all of those expressions. The user may refer to NAME anywhere she desires to use the ultimate name of the def. NAME should not be defined anywhere else in user code to avoid conflicts."
http://llvm.org/docs/TableGen/index.html


================
Comment at: ELF/Options.td:30
 
-def compress_debug_sections : J<"compress-debug-sections=">,
-  HelpText<"Compress DWARF debug sections">;
+defm compress_debug_sections : JSEq<"compress-debug-sections", "Compress DWARF debug sections">;
 
----------------
ruiu wrote:
> I think it is better to remove `help` from `JSEq` and keep HelpText for each option.
I would not do that because of next reasons:

  - It breaks our help output. Because libOption implementation renders all options which has help text. If I do that change,
     we will end up with duplicated options with the same help text.
  - It is inconsistent - we do not apply help text for other aliases. So we will have situations where we have `option with text and alias with text` vs `option with help text and alias without`.
  - It does not make sence to have help text set for alias. I mean practical sence, this information will be always probably be unused.





https://reviews.llvm.org/D35619