dblaikie added inline comments.
================ Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:52 +namespace { +using namespace llvm::ore; ---------------- mtrofin wrote: > wenlei wrote: > > mtrofin wrote: > > > wenlei wrote: > > > > curious why do we need anonymous namespace here? > > > iiuc it's preferred we place file-local types inside an anonymous > > > namespace. > > > > > > Looking now at the [[ > > > https://llvm.org/docs/CodingStandards.html#anonymous-namespaces | style > > > guideline ]], it touts their benefits but also says I should have only > > > placed de decl there and the impl of those members out... but the members > > > are quite trivial. Happy to move them out though. > > Thanks for the pointer. I don't have a strong opinion but slightly leaning > > towards moving out of anonymous namespace be consistent with how other > > InlineAdvice is organized (DefaultInlineAdvice, MLInlineAdvice not in > > anonymous namespace). > Ah, those are public (i.e. in a .h file) Generally if a type is declared/defined inside a .cpp file it should be in an anonymous namespace so it can't collide with other implementation type names in other .cpp files. (& .cpp-local functions should be static or in an anonymous namespace for the same reason) Looks like the other two (`DefaultInlineAdvice` and `MLInlineAdvice`) are defined in headers, so they must not be in anonymous namespaces. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110891/new/ https://reviews.llvm.org/D110891 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits