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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Driver.cpp:591
 
   Config->OFormatBinary = isOutputFormatBinary(Args);
   Config->SectionStartMap = getSectionStartMap(Args);
----------------
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.


================
Comment at: ELF/Options.td:152
+def no_dynamic_linker: F<"no-dynamic-linker">;
+
 def no_export_dynamic: F<"no-export-dynamic">;
----------------
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.


https://reviews.llvm.org/D30258