[PATCH] D143509: Move the BySpelling map to IncludeStructure.
This revision was automatically updated to reflect the committed changes.
VitaNuo marked an inline comment as done.
Closed by commit rGe028c9742897: Move the BySpelling map to IncludeStructure.
(authored by VitaNuo).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -197,6 +197,36 @@
Distance(getID(BazHeader, Includes), 1u)));
}
+TEST_F(HeadersTest, CacheBySpellingIsBuiltForMainInclusions) {
+ std::string FooHeader = testPath("foo.h");
+ FS.Files[FooHeader] = R"cpp(
+ void foo();
+)cpp";
+ std::string BarHeader = testPath("bar.h");
+ FS.Files[BarHeader] = R"cpp(
+ void bar();
+)cpp";
+ std::string BazHeader = testPath("baz.h");
+ FS.Files[BazHeader] = R"cpp(
+ void baz();
+)cpp";
+ FS.Files[MainFile] = R"cpp(
+#include "foo.h"
+#include "bar.h"
+#include "baz.h"
+)cpp";
+ auto Includes = collectIncludes();
+ EXPECT_THAT(Includes.MainFileIncludes,
+ UnorderedElementsAre(written("\"foo.h\""), written("\"bar.h\""),
+ written("\"baz.h\"")));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"foo.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[0]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"bar.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[1]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"baz.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[2]));
+}
+
TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
// We use TestTU here, to ensure we use the preamble replay logic.
// We're testing that the logic doesn't crash, and doesn't result in duplicate
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,16 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +493,37 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case incl
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks (both for the patch and for bearing with me)! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/ https://reviews.llvm.org/D143509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo marked 2 inline comments as done. VitaNuo added inline comments. Comment at: clang-tools-extra/clangd/Headers.cpp:303 + llvm::SmallVector Includes; + auto It = MainFileIncludesBySpelling.find(Spelling); + if (It == MainFileIncludesBySpelling.end()) kadircet wrote: > VitaNuo wrote: > > kadircet wrote: > > > nit: > > > ``` > > > llvm::SmallVector Includes; > > > for (auto Idx : MainFileIncludesBySpelling.lookup(Spelling)) > > > Includes.push_back(&MainFileIncludes[Idx]); > > > return Includes; > > > ``` > > But `lookup` will return 0 if it doesn't find the spelling in the map > > (default value of `int`). And at the same time 0 is a valid index. Isn't > > that just wrong? > `MainFileIncludesBySpelling` is a map with values of type vector, not int. > hence it'll be an empty vector, not `0`. Oh right. Sorry I guess I got confused. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/ https://reviews.llvm.org/D143509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo updated this revision to Diff 499456.
VitaNuo marked an inline comment as done.
VitaNuo added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -197,6 +197,36 @@
Distance(getID(BazHeader, Includes), 1u)));
}
+TEST_F(HeadersTest, CacheBySpellingIsBuiltForMainInclusions) {
+ std::string FooHeader = testPath("foo.h");
+ FS.Files[FooHeader] = R"cpp(
+ void foo();
+)cpp";
+ std::string BarHeader = testPath("bar.h");
+ FS.Files[BarHeader] = R"cpp(
+ void bar();
+)cpp";
+ std::string BazHeader = testPath("baz.h");
+ FS.Files[BazHeader] = R"cpp(
+ void baz();
+)cpp";
+ FS.Files[MainFile] = R"cpp(
+#include "foo.h"
+#include "bar.h"
+#include "baz.h"
+)cpp";
+ auto Includes = collectIncludes();
+ EXPECT_THAT(Includes.MainFileIncludes,
+ UnorderedElementsAre(written("\"foo.h\""), written("\"bar.h\""),
+ written("\"baz.h\"")));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"foo.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[0]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"bar.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[1]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"baz.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[2]));
+}
+
TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
// We use TestTU here, to ensure we use the preamble replay logic.
// We're testing that the logic doesn't crash, and doesn't result in duplicate
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,16 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +493,37 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case include_cleaner::Header::Verbatim:
- for (auto HeaderID : BySpelling.l
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
kadircet added inline comments. Comment at: clang-tools-extra/clangd/Headers.cpp:303 + llvm::SmallVector Includes; + auto It = MainFileIncludesBySpelling.find(Spelling); + if (It == MainFileIncludesBySpelling.end()) VitaNuo wrote: > kadircet wrote: > > nit: > > ``` > > llvm::SmallVector Includes; > > for (auto Idx : MainFileIncludesBySpelling.lookup(Spelling)) > > Includes.push_back(&MainFileIncludes[Idx]); > > return Includes; > > ``` > But `lookup` will return 0 if it doesn't find the spelling in the map > (default value of `int`). And at the same time 0 is a valid index. Isn't that > just wrong? `MainFileIncludesBySpelling` is a map with values of type vector, not int. hence it'll be an empty vector, not `0`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/ https://reviews.llvm.org/D143509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo added a comment.
Thanks for the comments!
Comment at: clang-tools-extra/clangd/Headers.cpp:303
+ llvm::SmallVector Includes;
+ auto It = MainFileIncludesBySpelling.find(Spelling);
+ if (It == MainFileIncludesBySpelling.end())
kadircet wrote:
> nit:
> ```
> llvm::SmallVector Includes;
> for (auto Idx : MainFileIncludesBySpelling.lookup(Spelling))
> Includes.push_back(&MainFileIncludes[Idx]);
> return Includes;
> ```
But `lookup` will return 0 if it doesn't find the spelling in the map (default
value of `int`). And at the same time 0 is a valid index. Isn't that just wrong?
Comment at: clang-tools-extra/clangd/Headers.h:167
+ // Spelling should include brackets or quotes, e.g. .
+ llvm::SmallVector
+ mainFileIncludesWithSpelling(llvm::StringRef Spelling) const {
kadircet wrote:
> kadircet wrote:
> > we're still returning just the `HeaderID`, the suggestion was to return
> > `llvm::SmallVector` so that applications can work with other
> > information available in the `Inclusion`. we also won't have any
> > requirements around include being resolved that way. the application should
> > figure out what to do if `HeaderID` is missing.
> >
> > also can you move this function body to source file instead?
> almost, but not right. we're returning copies of `Inclusion`s here, not
> pointers to `Inclusion`s inside the main file. the suggested signature was
> `llvm::SmallVector` any reason for returning by value?
Sure, thanks. No particular reason.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo updated this revision to Diff 499436.
VitaNuo marked 2 inline comments as done.
VitaNuo added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -197,6 +197,36 @@
Distance(getID(BazHeader, Includes), 1u)));
}
+TEST_F(HeadersTest, CacheBySpellingIsBuiltForMainInclusions) {
+ std::string FooHeader = testPath("foo.h");
+ FS.Files[FooHeader] = R"cpp(
+ void foo();
+)cpp";
+ std::string BarHeader = testPath("bar.h");
+ FS.Files[BarHeader] = R"cpp(
+ void bar();
+)cpp";
+ std::string BazHeader = testPath("baz.h");
+ FS.Files[BazHeader] = R"cpp(
+ void baz();
+)cpp";
+ FS.Files[MainFile] = R"cpp(
+#include "foo.h"
+#include "bar.h"
+#include "baz.h"
+)cpp";
+ auto Includes = collectIncludes();
+ EXPECT_THAT(Includes.MainFileIncludes,
+ UnorderedElementsAre(written("\"foo.h\""), written("\"bar.h\""),
+ written("\"baz.h\"")));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"foo.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[0]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"bar.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[1]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"baz.h\""),
+ UnorderedElementsAre(&Includes.MainFileIncludes[2]));
+}
+
TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
// We use TestTU here, to ensure we use the preamble replay logic.
// We're testing that the logic doesn't crash, and doesn't result in duplicate
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,16 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +493,37 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case include_cleaner::Header::Verbatim:
- for (auto HeaderID : BySpelling.l
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Headers.cpp:303
+ llvm::SmallVector Includes;
+ auto It = MainFileIncludesBySpelling.find(Spelling);
+ if (It == MainFileIncludesBySpelling.end())
nit:
```
llvm::SmallVector Includes;
for (auto Idx : MainFileIncludesBySpelling.lookup(Spelling))
Includes.push_back(&MainFileIncludes[Idx]);
return Includes;
```
Comment at: clang-tools-extra/clangd/Headers.h:167
+ // Spelling should include brackets or quotes, e.g. .
+ llvm::SmallVector
+ mainFileIncludesWithSpelling(llvm::StringRef Spelling) const {
kadircet wrote:
> we're still returning just the `HeaderID`, the suggestion was to return
> `llvm::SmallVector` so that applications can work with other
> information available in the `Inclusion`. we also won't have any requirements
> around include being resolved that way. the application should figure out
> what to do if `HeaderID` is missing.
>
> also can you move this function body to source file instead?
almost, but not right. we're returning copies of `Inclusion`s here, not
pointers to `Inclusion`s inside the main file. the suggested signature was
`llvm::SmallVector` any reason for returning by value?
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:516
+ Includes.mainFileIncludesWithSpelling(H.verbatim())) {
+ std::optional HeaderOpt = Inc.HeaderID;
+ if (HeaderOpt.has_value()) {
nit:
```
if(!Inc.HeaderID.has_value())
continue;
Used.insert(static_cast(*Inc.HeaderID));
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo updated this revision to Diff 498428.
VitaNuo added a comment.
Move to source file.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -197,6 +197,36 @@
Distance(getID(BazHeader, Includes), 1u)));
}
+TEST_F(HeadersTest, CacheBySpellingIsBuiltForMainInclusions) {
+ std::string FooHeader = testPath("foo.h");
+ FS.Files[FooHeader] = R"cpp(
+ void foo();
+)cpp";
+ std::string BarHeader = testPath("bar.h");
+ FS.Files[BarHeader] = R"cpp(
+ void bar();
+)cpp";
+ std::string BazHeader = testPath("baz.h");
+ FS.Files[BazHeader] = R"cpp(
+ void baz();
+)cpp";
+ FS.Files[MainFile] = R"cpp(
+#include "foo.h"
+#include "bar.h"
+#include "baz.h"
+)cpp";
+ auto Includes = collectIncludes();
+ EXPECT_THAT(Includes.MainFileIncludes,
+ UnorderedElementsAre(written("\"foo.h\""), written("\"bar.h\""),
+ written("\"baz.h\"")));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"foo.h\""),
+ UnorderedElementsAre(Includes.MainFileIncludes[0]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"bar.h\""),
+ UnorderedElementsAre(Includes.MainFileIncludes[1]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"baz.h\""),
+ UnorderedElementsAre(Includes.MainFileIncludes[2]));
+}
+
TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
// We use TestTU here, to ensure we use the preamble replay logic.
// We're testing that the logic doesn't crash, and doesn't result in duplicate
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,16 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +493,38 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case include_cleaner::Header::Verbatim:
- for (auto HeaderID : BySpelling.lookup(H.verbatim()))
- Used.insert(
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo marked an inline comment as done. VitaNuo added a comment. Thanks for the comments! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/ https://reviews.llvm.org/D143509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo updated this revision to Diff 498419.
VitaNuo added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -197,6 +197,36 @@
Distance(getID(BazHeader, Includes), 1u)));
}
+TEST_F(HeadersTest, CacheBySpellingIsBuiltForMainInclusions) {
+ std::string FooHeader = testPath("foo.h");
+ FS.Files[FooHeader] = R"cpp(
+ void foo();
+)cpp";
+ std::string BarHeader = testPath("bar.h");
+ FS.Files[BarHeader] = R"cpp(
+ void bar();
+)cpp";
+ std::string BazHeader = testPath("baz.h");
+ FS.Files[BazHeader] = R"cpp(
+ void baz();
+)cpp";
+ FS.Files[MainFile] = R"cpp(
+#include "foo.h"
+#include "bar.h"
+#include "baz.h"
+)cpp";
+ auto Includes = collectIncludes();
+ EXPECT_THAT(Includes.MainFileIncludes,
+ UnorderedElementsAre(written("\"foo.h\""), written("\"bar.h\""),
+ written("\"baz.h\"")));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"foo.h\""),
+ UnorderedElementsAre(Includes.MainFileIncludes[0]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"bar.h\""),
+ UnorderedElementsAre(Includes.MainFileIncludes[1]));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"baz.h\""),
+ UnorderedElementsAre(Includes.MainFileIncludes[2]));
+}
+
TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
// We use TestTU here, to ensure we use the preamble replay logic.
// We're testing that the logic doesn't crash, and doesn't result in duplicate
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,16 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +493,38 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case include_cleaner::Header::Verbatim:
- for (auto HeaderID : BySpelling.lookup(H.verbatim()))
- Used.insert(Head
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Headers.h:167
+ // Spelling should include brackets or quotes, e.g. .
+ llvm::SmallVector
+ mainFileIncludesWithSpelling(llvm::StringRef Spelling) const {
we're still returning just the `HeaderID`, the suggestion was to return
`llvm::SmallVector` so that applications can work with other
information available in the `Inclusion`. we also won't have any requirements
around include being resolved that way. the application should figure out what
to do if `HeaderID` is missing.
also can you move this function body to source file instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo added a comment. Thanks for the review! Comment at: clang-tools-extra/clangd/Headers.cpp:76 +Out->MainFileIncludesBySpelling.try_emplace(Inc.Written) +.first->second.push_back(static_cast(*Inc.HeaderID)); } kadircet wrote: > right now we're only storing "resolved" includes from the main file and not > all, this is creating a discrepancy between the view one gets through > `MainFileIncludes` and through this map. > in addition to that only storing `HeaderID` gets the job done for > IncludeCleaner, but won't really help anyone that wants to match main file > includes apart from that (there's no easy way to go from a `HeaderID` to an > `Inclusion`). > > so instead of storing the `HeaderID` in the map values, we can actually store > indexes into `MainFileIncludes`. later on during the lookup, we can build a > `SmallVector` that contains pointers to the relevant includes. > WDYT? Ok sure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/ https://reviews.llvm.org/D143509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo updated this revision to Diff 497602.
VitaNuo marked an inline comment as done.
VitaNuo added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -197,6 +197,39 @@
Distance(getID(BazHeader, Includes), 1u)));
}
+TEST_F(HeadersTest, CacheBySpellingIsBuiltForMainInclusions) {
+ std::string FooHeader = testPath("foo.h");
+ FS.Files[FooHeader] = R"cpp(
+ void foo();
+)cpp";
+ std::string BarHeader = testPath("bar.h");
+ FS.Files[BarHeader] = R"cpp(
+ void bar();
+)cpp";
+ std::string BazHeader = testPath("baz.h");
+ FS.Files[BazHeader] = R"cpp(
+ void baz();
+)cpp";
+ FS.Files[MainFile] = R"cpp(
+#include "foo.h"
+#include "bar.h"
+#include "baz.h"
+)cpp";
+ auto Includes = collectIncludes();
+ EXPECT_THAT(Includes.MainFileIncludes,
+ UnorderedElementsAre(written("\"foo.h\""), written("\"bar.h\""),
+ written("\"baz.h\"")));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"foo.h\""),
+ UnorderedElementsAre(static_cast(
+ *(Includes.MainFileIncludes[0]).HeaderID)));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"bar.h\""),
+ UnorderedElementsAre(static_cast(
+ *(Includes.MainFileIncludes[1]).HeaderID)));
+ EXPECT_THAT(Includes.mainFileIncludesWithSpelling("\"baz.h\""),
+ UnorderedElementsAre(static_cast(
+ *(Includes.MainFileIncludes[2]).HeaderID)));
+}
+
TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
// We use TestTU here, to ensure we use the preamble replay logic.
// We're testing that the logic doesn't crash, and doesn't result in duplicate
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,16 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +493,32 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
kadircet added a comment. oh forgot to mention, could you also please add some tests into llvm-project/clang-tools-extra/clangd/unittests/HeadersTests.cpp ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/ https://reviews.llvm.org/D143509 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Headers.cpp:76
+Out->MainFileIncludesBySpelling.try_emplace(Inc.Written)
+.first->second.push_back(static_cast(*Inc.HeaderID));
}
right now we're only storing "resolved" includes from the main file and not
all, this is creating a discrepancy between the view one gets through
`MainFileIncludes` and through this map.
in addition to that only storing `HeaderID` gets the job done for
IncludeCleaner, but won't really help anyone that wants to match main file
includes apart from that (there's no easy way to go from a `HeaderID` to an
`Inclusion`).
so instead of storing the `HeaderID` in the map values, we can actually store
indexes into `MainFileIncludes`. later on during the lookup, we can build a
`SmallVector` that contains pointers to the relevant includes.
WDYT?
Comment at: clang-tools-extra/clangd/Headers.h:166
+ llvm::SmallVector
+ lookupHeaderIDsBySpelling(llvm::StringRef Spelling) const {
+return MainFileIncludesBySpelling.lookup(Spelling);
what about renaming `lookupHeaderIDsBySpelling` to
`mainFileIncludesWithSpelling`. with a comment explaining `Returns includes
inside the main file with the given Spelling. Spelling should include brackets
or quotes, e.g. `.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo added a comment.
Thanks for review!
Comment at: clang-tools-extra/clangd/Headers.h:164
+ llvm::StringMap>
+ buildMainFileIncludesBySpelling() const {
+llvm::StringMap> BySpelling;
kadircet wrote:
> instead of building this on-demand, what about building it as we're
> collecting directives around
> https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L52
> ?
>
> afterwards we can just have a lookup function exposed here, that returns an
> `ArrayRef` ?
That's a great idea. I was thinking about this, but didn't know where to put
it, so I thought if it was possible, you'd point out in review ;-)
Thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo updated this revision to Diff 496463.
VitaNuo added a comment.
Expose API for lookup instead of retrieving the whole map.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,16 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +493,32 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case include_cleaner::Header::Verbatim:
- for (auto HeaderID : BySpelling.lookup(H.verbatim()))
- Used.insert(HeaderID);
- break;
- }
- }
- });
- return getUnused(AST, Used, /*ReferencedPublicHeaders*/{});
+ }
+ llvm::DenseSet Used;
+ include_cleaner::walkUsed(
+ AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
+ AST.getPragmaIncludes(), SM,
+ [&](const include_cleaner::SymbolReference &Ref,
+ llvm::ArrayRef Providers) {
+for (const auto &H : Providers) {
+ switch (H.kind()) {
+ case include_cleaner::Header::Physical:
+if (auto HeaderID = Includes.getID(H.physical()))
+ Used.insert(*HeaderID);
+break;
+ case include_cleaner::Header::Standard:
+for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
+ Used.insert(HeaderID);
+break;
+ case include_cleaner::Header::Verbatim:
+for (auto HeaderID :
+ Includes.lookupHeaderIDsBySpelling(H.verbatim()))
+ Used.insert(HeaderID);
+break;
+ }
+}
+ });
+ return getUnused(AST, Used, /*ReferencedPublicHeaders*/ {});
}
std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST,
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -23,6 +23,8 @@
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
#in
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo updated this revision to Diff 496460.
VitaNuo added a comment.
Address review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,18 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+ llvm::StringMap> BySpelling =
+ Includes.getMainFileIncludesBySpelling();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +495,31 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case include_cleaner::Header::Verbatim:
- for (auto HeaderID : BySpelling.lookup(H.verbatim()))
- Used.insert(HeaderID);
- break;
- }
- }
- });
- return getUnused(AST, Used, /*ReferencedPublicHeaders*/{});
+ }
+ llvm::DenseSet Used;
+ include_cleaner::walkUsed(
+ AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
+ AST.getPragmaIncludes(), SM,
+ [&](const include_cleaner::SymbolReference &Ref,
+ llvm::ArrayRef Providers) {
+for (const auto &H : Providers) {
+ switch (H.kind()) {
+ case include_cleaner::Header::Physical:
+if (auto HeaderID = Includes.getID(H.physical()))
+ Used.insert(*HeaderID);
+break;
+ case include_cleaner::Header::Standard:
+for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
+ Used.insert(HeaderID);
+break;
+ case include_cleaner::Header::Verbatim:
+for (auto HeaderID : BySpelling.lookup(H.verbatim()))
+ Used.insert(HeaderID);
+break;
+ }
+}
+ });
+ return getUnused(AST, Used, /*ReferencedPublicHeaders*/ {});
}
std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST,
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -23,6 +23,8 @@
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Stri
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
kadircet added inline comments.
Comment at: clang-tools-extra/clangd/Headers.h:164
+ llvm::StringMap>
+ buildMainFileIncludesBySpelling() const {
+llvm::StringMap> BySpelling;
instead of building this on-demand, what about building it as we're collecting
directives around
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L52
?
afterwards we can just have a lookup function exposed here, that returns an
`ArrayRef` ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143509/new/
https://reviews.llvm.org/D143509
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D143509: Move the BySpelling map to IncludeStructure.
VitaNuo created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
VitaNuo requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D143509
Files:
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/IncludeCleaner.cpp
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -85,7 +85,7 @@
// Using templateName case is handled by the override TraverseTemplateName.
if (TST->getTemplateName().getKind() == TemplateName::UsingTemplate)
return true;
-add(TST->getAsCXXRecordDecl()); // Specialization
+add(TST->getAsCXXRecordDecl()); // Specialization
return true;
}
@@ -474,22 +474,18 @@
translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM);
return getUnused(AST, ReferencedHeaders, ReferencedFiles.SpelledUmbrellas);
}
-std::vector computeUnusedIncludesExperimental(ParsedAST &AST) {
- const auto &SM = AST.getSourceManager();
- const auto &Includes = AST.getIncludeStructure();
- // FIXME: this map should probably be in IncludeStructure.
- llvm::StringMap> BySpelling;
- for (const auto &Inc : Includes.MainFileIncludes) {
-if (Inc.HeaderID)
- BySpelling.try_emplace(Inc.Written)
- .first->second.push_back(
- static_cast(*Inc.HeaderID));
- }
- // FIXME: !!this is a hacky way to collect macro references.
- std::vector Macros;
-auto& PP = AST.getPreprocessor();
- for (const syntax::Token &Tok :
-AST.getTokens().spelledTokens(SM.getMainFileID())) {
+std::vector
+computeUnusedIncludesExperimental(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
+ const auto &Includes = AST.getIncludeStructure();
+ llvm::StringMap> BySpelling =
+ Includes.buildMainFileIncludesBySpelling();
+
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector Macros;
+ auto &PP = AST.getPreprocessor();
+ for (const syntax::Token &Tok :
+ AST.getTokens().spelledTokens(SM.getMainFileID())) {
auto Macro = locateMacroAt(Tok, PP);
if (!Macro)
continue;
@@ -499,31 +495,31 @@
include_cleaner::Macro{/*Name=*/PP.getIdentifierInfo(Tok.text(SM)),
DefLoc},
include_cleaner::RefType::Explicit});
- }
- llvm::DenseSet Used;
- include_cleaner::walkUsed(
- AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes(), SM,
- [&](const include_cleaner::SymbolReference &Ref,
- llvm::ArrayRef Providers) {
- for (const auto &H : Providers) {
- switch (H.kind()) {
- case include_cleaner::Header::Physical:
- if (auto HeaderID = Includes.getID(H.physical()))
- Used.insert(*HeaderID);
- break;
- case include_cleaner::Header::Standard:
- for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
- Used.insert(HeaderID);
- break;
- case include_cleaner::Header::Verbatim:
- for (auto HeaderID : BySpelling.lookup(H.verbatim()))
- Used.insert(HeaderID);
- break;
- }
- }
- });
- return getUnused(AST, Used, /*ReferencedPublicHeaders*/{});
+ }
+ llvm::DenseSet Used;
+ include_cleaner::walkUsed(
+ AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
+ AST.getPragmaIncludes(), SM,
+ [&](const include_cleaner::SymbolReference &Ref,
+ llvm::ArrayRef Providers) {
+for (const auto &H : Providers) {
+ switch (H.kind()) {
+ case include_cleaner::Header::Physical:
+if (auto HeaderID = Includes.getID(H.physical()))
+ Used.insert(*HeaderID);
+break;
+ case include_cleaner::Header::Standard:
+for (auto HeaderID : Includes.StdlibHeaders.lookup(H.standard()))
+ Used.insert(HeaderID);
+break;
+ case include_cleaner::Header::Verbatim:
+for (auto HeaderID : BySpelling.lookup(H.verbatim()))
+ Used.insert(HeaderID);
+break;
+ }
+}
+ });
+ return getUnused(AST, Used, /*ReferencedPublicHeaders*/ {});
}
std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST,
Index: clang-tools-extra/clangd/Headers.h
===
--- clang-tools-extra/clangd/Headers.h
+++ clang-tools-extra/clangd/Headers.h
@@ -155,13 +155,23 @@
return !NonSelfContained.contains(ID);
}
- bool hasIWYUExport(HeaderID ID) const {
-return HasIWYUExport.contains(ID);
- }
+ bool hasIWYUExport(HeaderID ID) const { return HasIWYUExport.contain
