[PATCH] D53683: [ELF] Add --{, no-}call-graph-profile (enabled by default)

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese added inline comments.


================
Comment at: ELF/Driver.cpp:789
   Config->EmitRelocs = Args.hasArg(OPT_emit_relocs);
+  Config->EnableCallGraphProfile =
+      Args.hasFlag(OPT_call_graph_profile, OPT_no_call_graph_profile, true);
----------------
ruiu wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > ruiu wrote:
> > > > nit: you probably should spend a bit more time to read existing code around the location where you add new code to make your new code consistent with existing one. All the other `Config` members have the exact same name with a corresponding command line flag. For example, `Config->EhFrameHdr` corresponds to `--eh-frame-hdr` and `Config->EmitRelocs` corresponds to `--emit-relocs`. Please make your new code consistent with existing code.
> > > I know, but the variable `CallGraphProfile` is already used as a map, what should I rename it to?
> > 
> > ```
> > bool CallGraphProfile;
> > llvm::MapVector<std::pair<const InputSectionBase *, const InputSectionBase *>,
> >                   uint64_t>
> >     CallGraphInfo;
> > ```
> > 
> > Let me know if you have other preference
> Yeah you should, because you essentially should keep code beautiful/readable for those who read this code for the first time. If in doubt, forget about history of code and see new code with fresh eye. I'm sure you'll find this code inconsistent and odd.
This is a much less descriptive name.  I would call the option `{-no}-call-graph-profile-sort` as you are disabling the sorting based on the profile, not the profile itself.  Then the option name becomes `CallGraphProfileSort`.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53683