[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL358571: [clangd] Include insertion: require header guards,
drop other heuristics, treat… (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D60316?vs=195519=195531#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60316/new/
https://reviews.llvm.org/D60316
Files:
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
clang-tools-extra/trunk/unittests/clangd/TestTU.h
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -20,6 +20,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "gmock/gmock-more-matchers.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -33,7 +34,7 @@
using testing::_;
using testing::AllOf;
using testing::Contains;
-using testing::Eq;
+using testing::ElementsAre;
using testing::Field;
using testing::IsEmpty;
using testing::Not;
@@ -58,6 +59,7 @@
return StringRef(arg.CanonicalDeclaration.FileURI) == P;
}
MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
+MATCHER(IncludeHeader, "") { return !arg.IncludeHeaders.empty(); }
MATCHER_P(IncludeHeader, P, "") {
return (arg.IncludeHeaders.size() == 1) &&
(arg.IncludeHeaders.begin()->IncludeHeader == P);
@@ -249,6 +251,8 @@
TestFileURI = URI::create(TestFileName).toString();
}
+ // Note that unlike TestTU, no automatic header guard is added.
+ // HeaderCode should start with #pragma once to be treated as modular.
bool runSymbolCollector(llvm::StringRef HeaderCode, llvm::StringRef MainCode,
const std::vector = {}) {
llvm::IntrusiveRefCntPtr Files(
@@ -920,7 +924,7 @@
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
- runSymbolCollector("class Foo {};", /*Main=*/"");
+ runSymbolCollector("#pragma once\nclass Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols, UnorderedElementsAre(
AllOf(QName("Foo"), DeclURI(TestHeaderURI;
EXPECT_THAT(Symbols.begin()->IncludeHeaders,
@@ -1020,53 +1024,54 @@
TEST_F(SymbolCollectorTest, MainFileIsHeaderWhenSkipIncFile) {
CollectorOpts.CollectIncludePath = true;
- CanonicalIncludes Includes;
- CollectorOpts.Includes =
- TestFileName = testPath("main.h");
+ // To make this case as hard as possible, we won't tell clang main is a
+ // header. No extension, no -x c++-header.
+ TestFileName = testPath("no_ext_main");
TestFileURI = URI::create(TestFileName).toString();
auto IncFile = testPath("test.inc");
auto IncURI = URI::create(IncFile).toString();
InMemoryFileSystem->addFile(IncFile, 0,
llvm::MemoryBuffer::getMemBuffer("class X {};"));
- runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+ runSymbolCollector("", R"cpp(
+// Can't use #pragma once in a main file clang doesn't think is a header.
+#ifndef MAIN_H_
+#define MAIN_H_
+#include "test.inc"
+#endif
+ )cpp",
/*ExtraArgs=*/{"-I", testRoot()});
EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
IncludeHeader(TestFileURI;
}
-TEST_F(SymbolCollectorTest, MainFileIsHeaderWithoutExtensionWhenSkipIncFile) {
+TEST_F(SymbolCollectorTest, IncFileInNonHeader) {
CollectorOpts.CollectIncludePath = true;
- CanonicalIncludes Includes;
- CollectorOpts.Includes =
- TestFileName = testPath("no_ext_main");
+ TestFileName = testPath("main.cc");
TestFileURI = URI::create(TestFileName).toString();
auto IncFile = testPath("test.inc");
auto IncURI = URI::create(IncFile).toString();
InMemoryFileSystem->addFile(IncFile, 0,
llvm::MemoryBuffer::getMemBuffer("class X {};"));
- runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+ runSymbolCollector("", R"cpp(
+#include "test.inc"
+ )cpp",
/*ExtraArgs=*/{"-I", testRoot()});
EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
- IncludeHeader(TestFileURI;
+
[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.
sammccall updated this revision to Diff 195519.
sammccall marked 3 inline comments as done.
sammccall added a comment.
address review comments
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60316/new/
https://reviews.llvm.org/D60316
Files:
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/SymbolCollector.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -52,6 +52,9 @@
// Index to use when building AST.
const SymbolIndex *ExternalIndex = nullptr;
+ // Simulate a header guard of the header (using an #import directive).
+ bool ImplicitHeaderGuard = true;
+
ParsedAST build() const;
SymbolSlab headerSymbols() const;
std::unique_ptr index() const;
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -19,13 +19,19 @@
ParsedAST TestTU::build() const {
std::string FullFilename = testPath(Filename),
- FullHeaderName = testPath(HeaderFilename);
+ FullHeaderName = testPath(HeaderFilename),
+ ImportThunk = testPath("import_thunk.h");
std::vector Cmd = {"clang", FullFilename.c_str()};
+ // We want to implicitly include HeaderFilename without messing up offsets.
+ // -include achieves this, but sometimes we want #import (to simulate a header
+ // guard without messing up offsets). In this case, use an intermediate file.
+ std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
// FIXME: this shouldn't need to be conditional, but it breaks a
// GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
if (!HeaderCode.empty()) {
Cmd.push_back("-include");
-Cmd.push_back(FullHeaderName.c_str());
+Cmd.push_back(ImplicitHeaderGuard ? ImportThunk.c_str()
+ : FullHeaderName.c_str());
}
Cmd.insert(Cmd.end(), ExtraArgs.begin(), ExtraArgs.end());
ParseInputs Inputs;
@@ -33,7 +39,9 @@
Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
Inputs.CompileCommand.Directory = testRoot();
Inputs.Contents = Code;
- Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
+ Inputs.FS = buildTestFS({{FullFilename, Code},
+ {FullHeaderName, HeaderCode},
+ {ImportThunk, ThunkContents}});
Inputs.Opts = ParseOptions();
Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
Inputs.Index = ExternalIndex;
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -20,6 +20,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "gmock/gmock-more-matchers.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -33,7 +34,7 @@
using testing::_;
using testing::AllOf;
using testing::Contains;
-using testing::Eq;
+using testing::ElementsAre;
using testing::Field;
using testing::IsEmpty;
using testing::Not;
@@ -58,6 +59,7 @@
return StringRef(arg.CanonicalDeclaration.FileURI) == P;
}
MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
+MATCHER(IncludeHeader, "") { return !arg.IncludeHeaders.empty(); }
MATCHER_P(IncludeHeader, P, "") {
return (arg.IncludeHeaders.size() == 1) &&
(arg.IncludeHeaders.begin()->IncludeHeader == P);
@@ -249,6 +251,8 @@
TestFileURI = URI::create(TestFileName).toString();
}
+ // Note that unlike TestTU, no automatic header guard is added.
+ // HeaderCode should start with #pragma once to be treated as modular.
bool runSymbolCollector(llvm::StringRef HeaderCode, llvm::StringRef MainCode,
const std::vector = {}) {
llvm::IntrusiveRefCntPtr Files(
@@ -268,8 +272,8 @@
Args, Factory->create(), Files.get(),
std::make_shared());
-InMemoryFileSystem->addFile(TestHeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(
+TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
InMemoryFileSystem->addFile(TestFileName, 0,
llvm::MemoryBuffer::getMemBuffer(MainCode));
Invocation.run();
@@ -920,7 +924,7 @@
TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
CollectorOpts.CollectIncludePath = true;
- runSymbolCollector("class Foo {};", /*Main=*/"");
+ runSymbolCollector("#pragma once\nclass Foo {};",
[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.
ioeric requested changes to this revision. ioeric added a comment. This revision now requires changes to proceed. in case you missed this patch :) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60316/new/ https://reviews.llvm.org/D60316 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.
ioeric added inline comments.
Comment at: clangd/index/SymbolCollector.cpp:157
+ return Canonical.str();
+else if (Canonical != Filename)
+ return toURI(SM, Canonical, Opts);
nit: no need for `else`?
Comment at: unittests/clangd/FileIndexTests.cpp:214
TEST(FileIndexTest, HasSystemHeaderMappingsInPreamble) {
+ TestTU TU;
sammccall wrote:
> I suspect we can replace most of these tests with TestTU - here I just
> changed the ones where the include guard would otherwise be needed.
This looks good to me.
I think this test case tried to explicitly test that preamble callback has the
expected `CanonicalIncludes`. Using `TestTU` seems to achieve the same goal but
makes the intention a bit less clear though.
Comment at: unittests/clangd/TestTU.h:55
+ // Simulate a header guard of the header (using an #import directive).
+ bool ImplicitHeaderGuard = true;
is this used?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60316/new/
https://reviews.llvm.org/D60316
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.
sammccall marked an inline comment as done.
sammccall added inline comments.
Comment at: unittests/clangd/FileIndexTests.cpp:214
TEST(FileIndexTest, HasSystemHeaderMappingsInPreamble) {
+ TestTU TU;
I suspect we can replace most of these tests with TestTU - here I just changed
the ones where the include guard would otherwise be needed.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60316/new/
https://reviews.llvm.org/D60316
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
We do have some reports of include insertion behaving badly in some
codebases. Requiring header guards both makes sense in principle, and is
likely to disable this "nice-to-have" feature in codebases where headers don't
follow the expected pattern.
With this we can drop some other heuristics, such as looking at file
extensions to detect known non-headers - implementation files have no guards.
One wrinkle here is #import - objc headers may not have guards because
they're intended to be used via #import. If the header is the main file
or is #included, we won't collect locations - merge should take care of
this if we see the file #imported somewhere. Seems likely to be OK.
Headers which have a canonicalization (stdlib, IWYU) are exempt from this check.
*.inc files continue to be handled by looking up to the including file.
This patch also adds *.def here - tablegen wants this pattern too.
In terms of code structure, the division between SymbolCollector and
CanonicalIncludes has shifted: SymbolCollector is responsible for more.
This is because SymbolCollector has all the SourceManager/HeaderSearch access
needed for checking for guards, and we interleave these checks with the *.def
checks in a loop (potentially).
We could hand all the info into CanonicalIncludes and put the logic there
if that's preferable.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D60316
Files:
clangd/index/CanonicalIncludes.cpp
clangd/index/CanonicalIncludes.h
clangd/index/SymbolCollector.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -52,6 +52,9 @@
// Index to use when building AST.
const SymbolIndex *ExternalIndex = nullptr;
+ // Simulate a header guard of the header (using an #import directive).
+ bool ImplicitHeaderGuard = true;
+
ParsedAST build() const;
SymbolSlab headerSymbols() const;
std::unique_ptr index() const;
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -19,13 +19,19 @@
ParsedAST TestTU::build() const {
std::string FullFilename = testPath(Filename),
- FullHeaderName = testPath(HeaderFilename);
+ FullHeaderName = testPath(HeaderFilename),
+ ImportThunk = testPath("import_thunk.h");
std::vector Cmd = {"clang", FullFilename.c_str()};
+ // We want to implicitly include HeaderFilename without messing up offsets.
+ // -include achieves this, but sometimes we want #import (to simulate a header
+ // guard without messing up offsets). In this case, use an intermediate file.
+ std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
// FIXME: this shouldn't need to be conditional, but it breaks a
// GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
if (!HeaderCode.empty()) {
Cmd.push_back("-include");
-Cmd.push_back(FullHeaderName.c_str());
+Cmd.push_back(ImplicitHeaderGuard ? ImportThunk.c_str()
+ : FullHeaderName.c_str());
}
Cmd.insert(Cmd.end(), ExtraArgs.begin(), ExtraArgs.end());
ParseInputs Inputs;
@@ -33,7 +39,9 @@
Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
Inputs.CompileCommand.Directory = testRoot();
Inputs.Contents = Code;
- Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
+ Inputs.FS = buildTestFS({{FullFilename, Code},
+ {FullHeaderName, HeaderCode},
+ {ImportThunk, ThunkContents}});
Inputs.Opts = ParseOptions();
Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
Inputs.Index = ExternalIndex;
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -33,9 +33,7 @@
using testing::_;
using testing::AllOf;
using testing::Contains;
-using testing::Eq;
using testing::Field;
-using testing::IsEmpty;
using testing::Not;
using testing::Pair;
using testing::UnorderedElementsAre;
@@ -55,6 +53,7 @@
return StringRef(arg.CanonicalDeclaration.FileURI) == P;
}
MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
+MATCHER(NoIncludeHeader, "") { return arg.IncludeHeaders.empty(); }
MATCHER_P(IncludeHeader, P, "") {
return (arg.IncludeHeaders.size() == 1) &&
(arg.IncludeHeaders.begin()->IncludeHeader == P);
@@
