wenlei accepted this revision. wenlei added a comment. This revision is now accepted and ready to land.
lgtm, thanks. ================ Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:52 +namespace { +using namespace llvm::ore; ---------------- 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). 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