That's two votes for "this is too surprising" - changed. https://imgflip.com/i/1k93rm https://68.media.tumblr.com/8db2fe0a6f84ff128157a2b615f519bf/tumblr_inline_nenq4hMoQA1sb080b.gif
On Fri, Feb 24, 2017 at 9:54 AM, Manuel Klimek <kli...@google.com> wrote: > On Thu, Feb 23, 2017 at 10:40 PM Sam McCall <sammcc...@google.com> wrote: > >> >> >> On Feb 23, 2017 8:48 PM, "Haojian Wu via Phabricator" < >> revi...@reviews.llvm.org> wrote: >> >> hokein added inline comments. >> >> >> ================ >> Comment at: unittests/include-fixer/find-all-symbols/ >> FindAllSymbolsTests.cpp:40 >> + void reportSymbols(llvm::StringRef FileName, >> + SymbolInfo::SignalMap NewSymbols) override { >> + for (const auto &Entry : NewSymbols) >> ---------------- >> A new catch: `NewSymbols` should be passed by reference, otherwise a copy >> will be generated. >> >> I did actually intend by-value here, FindAllSymbols no longer needs the >> map once it's reported, so it is moved rather than copied (see the end of >> FindAllSymbols.cpp) >> >> If this is too surprising, I can change it. >> > > I'd say that that couples the call site and the function too much. > > >> >> >> >> >> >> https://reviews.llvm.org/D30210 >> >> >> >> >>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits