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&id=89983#toc
Repository:
rL LLVM
https://revie
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
include-f
bkramer accepted this revision.
bkramer added a comment.
lg
Comment at: include-fixer/SymbolIndexManager.cpp:156
+ for (const auto &SymAndSig : MatchedSymbols)
+Res.push_back(SymAndSig.Symbol);
+ return Res;
std::move
Comment at: includ
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
include-fixe
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`
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
include-fi
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 McCall wrote:
>
>>
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:
> unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp:40
> + voi
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,
+ SymbolInf
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)
-
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
include-fixer/Sy
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.
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 m
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
include-fixer/InMemorySy
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
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
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
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 type
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
include-fixer/SymbolIn
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
include-fixer/InMem
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
- FindAllSymbols/FindAllMacros now report in batche
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 efficiently,
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?
==
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 th
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
include-fixer/find-all-symbols/FindAllS
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 = &SymbolReporter::reportSym
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
include-fixer/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 symbo
28 matches
Mail list logo
