[PATCH] D30210: [include-fixer] Add usage count to find-all-symbols.

[Sam McCall via cfe-commits]

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 <klimek at google.com> wrote:

> On Thu, Feb 23, 2017 at 10:40 PM Sam McCall <sammccall at google.com> wrote:
>
>>
>>
>> On Feb 23, 2017 8:48 PM, "Haojian Wu via Phabricator" <
>> reviews at 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
>>
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170224/9a8ac237/attachment.html>