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

This revision was automatically updated to reflect the committed changes. Closed by commit rL296446: [include-fixer] Add usage count to find-all-symbols. (authored by sammccall). Changed prior to commit: https://reviews.llvm.org/D30210?vs=89982=89983#toc Repository: rL LLVM

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

sammccall updated this revision to Diff 89982. sammccall marked an inline comment as done. sammccall added a comment. Review comments https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp include-fixer/InMemorySymbolIndex.h include-fixer/IncludeFixer.cpp

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

bkramer accepted this revision. bkramer added a comment. lg Comment at: include-fixer/SymbolIndexManager.cpp:156 + for (const auto : MatchedSymbols) +Res.push_back(SymAndSig.Symbol); + return Res; std::move Comment at:

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

sammccall updated this revision to Diff 89636. sammccall added a comment. Remove redundant std::move https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp include-fixer/InMemorySymbolIndex.h include-fixer/IncludeFixer.cpp include-fixer/SymbolIndex.h

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

hokein added a comment. Thanks, still LGTM with one nit. Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:262 + if (Filename != "") { +Reporter->reportSymbols(Filename, std::move(FileSymbols)); +FileSymbols = {}; We don't need `std::move`

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

sammccall updated this revision to Diff 89632. sammccall added a comment. Report symbols by reference. https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp include-fixer/InMemorySymbolIndex.h include-fixer/IncludeFixer.cpp include-fixer/SymbolIndex.h

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

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 wrote: > On Thu, Feb 23, 2017 at 10:40 PM Sam

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

On Thu, Feb 23, 2017 at 10:40 PM Sam McCall wrote: > > > On Feb 23, 2017 8:48 PM, "Haojian Wu via Phabricator" < > revi...@reviews.llvm.org> wrote: > > hokein added inline comments. > > > > Comment at: >

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

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, +

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

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 : NewSymbols) A

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

sammccall updated this revision to Diff 89510. sammccall added a comment. Update comment - oops! https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp include-fixer/InMemorySymbolIndex.h include-fixer/IncludeFixer.cpp include-fixer/SymbolIndex.h

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

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Thanks! Looks good from my side. I'd wait to see whether @bkramer has more comments before commit it. Comment at: include-fixer/find-all-symbols/SymbolInfo.h:50 + // used.

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

sammccall added a comment. Thanks for bearing with me here :) Comment at: include-fixer/InMemorySymbolIndex.h:27 - std::vector + std::vector search(llvm::StringRef Identifier) override; hokein wrote: > sammccall wrote: > > hokein wrote: > > > There are

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

sammccall updated this revision to Diff 89506. sammccall marked 3 inline comments as done. sammccall added a comment. Address review comments; remove redundant namespace qualifiers; format. https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp

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

hokein added inline comments. Comment at: include-fixer/InMemorySymbolIndex.h:27 - std::vector + std::vector search(llvm::StringRef Identifier) override; sammccall wrote: > hokein wrote: > > There are many places using > > `std::vector`. Maybe we can use

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

sammccall added inline comments. Comment at: include-fixer/InMemorySymbolIndex.h:27 - std::vector + std::vector search(llvm::StringRef Identifier) override; hokein wrote: > There are many places using > `std::vector`. Maybe we can use a > type alias for

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

sammccall updated this revision to Diff 89484. sammccall marked 4 inline comments as done. sammccall added a comment. Address review comments. https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp include-fixer/InMemorySymbolIndex.h include-fixer/IncludeFixer.cpp

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

hokein added inline comments. Comment at: include-fixer/InMemorySymbolIndex.h:27 - std::vector + std::vector search(llvm::StringRef Identifier) override; There are many places using `std::vector`. Maybe we can use a type alias for it, so that we can

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

sammccall updated this revision to Diff 89366. sammccall added a comment. git-clang-format https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp include-fixer/InMemorySymbolIndex.h include-fixer/IncludeFixer.cpp include-fixer/SymbolIndex.h

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

sammccall updated this revision to Diff 89365. sammccall added a comment. Switch from SymbolInfo::WithSignals as a typedef for std::pair, to a struct SymbolAndSignals, to have clearer semantics. https://reviews.llvm.org/D30210 Files: include-fixer/InMemorySymbolIndex.cpp

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

sammccall updated this revision to Diff 89360. sammccall added a comment. Changes based on offline discussion: - mutable SignalInfo data split into SignalInfo::Signals - yaml serialized data is SignalInfo::WithSignals, which is currently a pair -

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

sammccall planned changes to this revision. sammccall added a comment. OK, some changes to come here: - klimek thinks using a set as a pseudo-map with `mutable` is evil, which is a fair point - the current `SymbolReporter` interface doesn't provide enough information to deduplicate

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

klimek added inline comments. Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:251 + } else { +assert(false && "Must match a NamedDecl!"); + } You can use llvm_unreachable instead. Or do you actually want to fall through in release mode?

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

sammccall added a comment. Thanks for the review, still learning the style :) Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:250 + } else { +assert(!"Must match a NamedDecl!"); + } hokein wrote: > Is the preceding `!` intended? This does

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

sammccall updated this revision to Diff 89337. sammccall marked 7 inline comments as done. sammccall added a comment. Address review comments. https://reviews.llvm.org/D30210 Files: include-fixer/IncludeFixer.cpp include-fixer/SymbolIndexManager.cpp

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

hokein added a comment. Thanks for the contributions! Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:244 - const auto *ND = Result.Nodes.getNodeAs("decl"); - assert(ND && "Matched declaration must be a NamedDecl!"); + auto report = ::reportSymbol; + const

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

sammccall updated this revision to Diff 89236. sammccall added a comment. Remove obsolete comment. https://reviews.llvm.org/D30210 Files: include-fixer/IncludeFixer.cpp include-fixer/SymbolIndexManager.cpp include-fixer/find-all-symbols/FindAllSymbols.cpp

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

sammccall created this revision. Add usage count to find-all-symbols. FindAllSymbols now finds (most!) main-file usages of the discovered symbols. The per-TU map output has NumUses=0 or 1 (only one use per file is counted). The reducer aggregates these to find the number of files that use a