This is an archive of the discontinued LLVM Phabricator instance.

[include-fixer] Add a find-all-symbols tool for include-fixer.
ClosedPublic

Authored by hokein on Apr 25 2016, 8:14 AM.

Details

Summary

The find-all-symbols tool generates a yaml symbol database for
include-fixer.

The symbol matcher is originally written by Xiaoyi Liu.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 54856.Apr 25 2016, 8:14 AM
hokein retitled this revision from to [include-fixer] Add a find-all-symbols tool for include-fixer..
hokein updated this object.
hokein set the repository for this revision to rL LLVM.
hokein added subscribers: ioeric, klimek, cfe-commits.
bkramer edited edge metadata.Apr 26 2016, 5:22 AM

First, please run this through clang-format with LLVM style. This aligns all the & and * to the name instead of the type. There are also some underscore_names left that need conversion to PascalCase.

include-fixer/find-all-symbols/FindAllSymbols.cpp
168 ↗(On Diff #54856)

Why?

include-fixer/find-all-symbols/FindAllSymbols.h
26 ↗(On Diff #54856)

StringRef for the argument. we also start methods with a small letter in LLVM (reportResult).

include-fixer/find-all-symbols/SymbolInfo.cpp
104 ↗(On Diff #54856)

This could be written as std::tie(Identifier, FilePath, LineNumber) < std::tie(Symbol.Identifier, Symbol.FilePath, Symbol.LineNumber)

111 ↗(On Diff #54856)

Please write directly to the stream instead of going through std::string.

hokein updated this revision to Diff 55003.Apr 26 2016, 7:46 AM
hokein marked 3 inline comments as done.
hokein edited edge metadata.

Address review comments.

hokein marked an inline comment as done.Apr 26 2016, 7:49 AM
bkramer accepted this revision.Apr 26 2016, 7:55 AM
bkramer edited edge metadata.

lg with some minor comments remaining.

include-fixer/find-all-symbols/FindAllSymbols.cpp
47 ↗(On Diff #55003)

Just use llvm::cast instead of llvm::dyn_cast, it includes the assert.

include-fixer/find-all-symbols/FindAllSymbols.h
24 ↗(On Diff #55003)

nit: use = default instead of {}

38 ↗(On Diff #55003)

nit: move const to the start of the line.

This revision is now accepted and ready to land.Apr 26 2016, 7:55 AM
hokein updated this revision to Diff 55172.Apr 27 2016, 2:17 AM
hokein marked 2 inline comments as done.
hokein edited edge metadata.

Update.

include-fixer/find-all-symbols/FindAllSymbols.h
38 ↗(On Diff #55003)

Moving const to the start of line will change semantic of the declaration. We need a constant pointer here, not a pointer to a constant data.

bkramer added inline comments.Apr 27 2016, 2:47 AM
include-fixer/find-all-symbols/FindAllSymbols.h
39 ↗(On Diff #55172)

Ah, sorry, my bad.

djasper added inline comments.Apr 27 2016, 4:47 AM
include-fixer/find-all-symbols/FindAllSymbols.cpp
22–24 ↗(On Diff #55172)

Can you remove the "abc = " part?

29 ↗(On Diff #55172)

Remove "clang::". Here and everywhere.

31 ↗(On Diff #55172)

I'd turn this into a for loop:

for (const auto *Context = ND->getDeclContext; Context; Context = Context->getParent()) {
  ...
33 ↗(On Diff #55172)

No braces. Here and at all the other single-statement ifs.

37 ↗(On Diff #55172)

nit: remove "llvm::" and "clang::"

40 ↗(On Diff #55172)

I'd just do:

Symbol->Contexts.emplace_back(
    SymbolInfo::Namespace,
    NSD->isAnonymousNamespace() ? "" : NSD->getName().str());
47 ↗(On Diff #55172)

Maybe assert that RD isn't nullptr?

76 ↗(On Diff #55172)

We traditionally call this registerMatchers ..

201 ↗(On Diff #55172)

Why is it useful to report with the name of the main file? I think some more comments on what this tool is supposed to do overall would help. Maybe at the top of this file or of FindAllSymbolsMain.

include-fixer/find-all-symbols/SymbolInfo.h
24 ↗(On Diff #55172)

Call this "Kind" so that we don't confuse it with the Type of variables and function parameters.

37 ↗(On Diff #55172)

I'd just call this name.

46 ↗(On Diff #55172)

Maybe just call this Context instead of ContextInfo?

52 ↗(On Diff #55172)

Can't hurt to explicitly state that unnamed namespaces are stored with the name "".

76 ↗(On Diff #55172)

Should we maybe just use inheritance here?

hokein updated this revision to Diff 55220.Apr 27 2016, 7:23 AM
hokein marked 13 inline comments as done.

Add Daniel's comments.

include-fixer/find-all-symbols/FindAllSymbols.cpp
22–24 ↗(On Diff #55172)

Remove all of these since they are not needed here.

47 ↗(On Diff #55172)

cast does it for us.

201 ↗(On Diff #55172)

Yeah, the find-all-symbols tool can accept multiple files at one time ("find-all-symbols <source0>, source1, ..."), we need the name for the main file here.

include-fixer/find-all-symbols/SymbolInfo.h
76 ↗(On Diff #55172)

We might not use inheritance here because LLVM yaml library doesn't support it.

djasper accepted this revision.Apr 27 2016, 7:26 AM
djasper edited edge metadata.

Looks good.

include-fixer/find-all-symbols/FindAllSymbols.cpp
39 ↗(On Diff #55220)

I'd still remove a "llvm::"..

hokein updated this revision to Diff 55223.Apr 27 2016, 7:26 AM
hokein edited edge metadata.

Fix a nit.

hokein updated this revision to Diff 55226.Apr 27 2016, 7:30 AM

Fix another nit.

This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
clang-tools-extra/trunk/unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
333

size_t clashes against built-in type. Tweaked in r267841.