[PATCH] D30258: [ELF] - Implemented --no-dynamic-linker option

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Driver.cpp:591
 
   Config->OFormatBinary = isOutputFormatBinary(Args);
   Config->SectionStartMap = getSectionStartMap(Args);
----------------
grimar wrote:
> ruiu wrote:
> > As you can see the code is roughly grouped in this function. Please keep it consistent. Probably you need to move the line here.
> Ok, I did not move it there intentionally because we have
> ```
> Config->Discard = getDiscardOption(Args);
> ```
> at its place which confused me, probably I had to.
> 
> But aside of this I find this grouping is confusing. It mixes real and "calculated" options.
> I mean:
> * OFormatBinary is ok, just corresponds to OPT_oformat.
> * SectionStartMap is different, there is no such option, it uses OPT_section_start/OPT_Ttext/OPT_Tdata/OPT_Tbss
> * SortSection and Target2 are ok, because corresponds to OPT_sort_section/OPT_target2
> * UnresolvedSymbols depends on OPT_noinhibit_exec/OPT_error_undef/OPT_warn_undef/OPT_no_undefined/OPT_unresolved_symbols/OPT_z
> 
> General grouping is also not ideal IMO.
> 
> What I would suggest is:
> * Do not split Args.hasArg/getString/getInteger groups and sort options alphabetically. That probably much more useful.
> I think code readers usually search option implementation by name and not by its type which is impossible to remember.
> * Move OFormatBinary, SortSection, Target2 back to that list, because them are regular options connected with single command line parameter which equals to their variable name.
> * Keep only SectionStartMap and UnresolvedSymbols outside in separate group.
Maybe. Let me think about that. Some options depend on other options, but from this code it is not very easy to see.


================
Comment at: ELF/Options.td:152
+def no_dynamic_linker: F<"no-dynamic-linker">;
+
 def no_export_dynamic: F<"no-export-dynamic">;
----------------
grimar wrote:
> grimar wrote:
> > ruiu wrote:
> > > Remove this blank line to keep the style consistent.
> > Blank line is intentional. Doesn't absence of blank line here
> > 
> > ```
> > def no_export_dynamic: F<"no-export-dynamic">;
> > def no_fatal_warnings: F<"no-fatal-warnings">;
> > ```
> > 
> > is a violation of style ? I think for regular options (not aliases and not ignored)
> > we always used blank line to separate them.
> Looks like these two options have no empty separating line because does not
> have HelpText ? That is inconsistent styling then.
> At least because LTO options has HelpText but also have no blank lines.
> 
> I think it worth to add help text for all options that are active (not ignored).
> For ignored options probably makes sence to put "Option is ignored" or something 
> as help text too.
> 
> I added HelpText for no_dynamic_linker before commiting.
This change was inconsistent in this local context. The entire context might be inconsistent, but it is not good to make more it more inconsistent by making an exception inside an exception (if it is exceptional).


Repository:
  rL LLVM

https://reviews.llvm.org/D30258