[PATCH] D94486: [LTO] Expose opt() in LTOBackend (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:486
 
+bool lto::opt(const Config &Conf, TargetMachine *TM, unsigned Task, Module &Mod,
+              bool IsThinLTO, ModuleSummaryIndex *ExportSummary,
----------------
dexonsmith wrote:
> fhahn wrote:
> > steven_wu wrote:
> > > Any reason why the function need to be moved instead of just renamed? I will reduce the diff if possible.
> > Unfortunately it is originally defined in a anonymous namespace, so I think it has to be moved out of that to be defined in `lto::`.
> [drive-by comments]
> 
> I think just adding the `lto::` should do the right thing. If not, you can temporarily close the anonymous namespace and reopen it.
> 
> Another option is an NFC prep commit that limits the anonymous namespace to `.cpp`-declared structures, and adds a `static` keyword on each function that's no longer "anonymous". That'd match the more typical LLVM style... reducing this patch to `s/static bool opt/bool lto::opt/`.
> Another option is an NFC prep commit that limits the anonymous namespace to .cpp-declared structures, and adds a static keyword on each function that's no longer "anonymous". That'd match the more typical LLVM style... reducing this patch to s/static bool opt/bool lto::opt/.

Ah yes, LLVM style suggests to only use anonymous namespaces for class declarations! I went with this suggestion and removed the anonymous namespace in f638c2eb4ee6d0a0bd0e80cd305ad93e382db8f5, because it just contained function definitions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94486/new/

https://reviews.llvm.org/D94486