sammccall created this revision.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
This should probably be untangled into 3 separate patches, but
looking for feedback first on whether we want to do this at all.
a) store include-insertion information for main-file symbols that are
not eligible for code completion indexing.
b) consider an #ifndef guard that's dangling at the end of the preamble
to be a well-formed header guard (otherwise we never will)
c) load HeaderFileInfo for the preamble main-file even if the file-size
mismatches
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D78038
Files:
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang/lib/Lex/Lexer.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp
llvm/include/llvm/Support/OnDiskHashTable.h
Index: llvm/include/llvm/Support/OnDiskHashTable.h
===================================================================
--- llvm/include/llvm/Support/OnDiskHashTable.h
+++ llvm/include/llvm/Support/OnDiskHashTable.h
@@ -320,8 +320,8 @@
class iterator {
internal_key_type Key;
- const unsigned char *const Data;
- const offset_type Len;
+ const unsigned char *Data;
+ offset_type Len;
Info *InfoObj;
public:
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1648,7 +1648,6 @@
endian::Writer LE(Out, little);
uint64_t Start = Out.tell(); (void)Start;
-
unsigned char Flags = (Data.HFI.isImport << 5)
| (Data.HFI.isPragmaOnce << 4)
| (Data.HFI.DirInfo << 1)
@@ -1768,6 +1767,10 @@
SmallVector<const FileEntry *, 16> FilesByUID;
HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+ const auto &SM = Context->getSourceManager();
+ unsigned MainFileUID = -1;
+ if (const auto *Entry = SM.getFileEntryForID(SM.getMainFileID()))
+ MainFileUID = Entry->getUID();
if (FilesByUID.size() > HS.header_file_size())
FilesByUID.resize(HS.header_file_size());
@@ -1806,6 +1809,13 @@
};
Generator.insert(Key, Data, GeneratorTrait);
++NumHeaderSearchEntries;
+ // We may reuse a preamble even if the rest of the file is different, so
+ // allow looking up info for the main file with a zero size.
+ if (File->getUID() == MainFileUID) {
+ Key.Size = 0;
+ Generator.insert(Key, Data, GeneratorTrait);
+ ++NumHeaderSearchEntries;
+ }
}
// Create the on-disk hash table in a buffer.
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6147,9 +6147,19 @@
= static_cast<HeaderFileInfoLookupTable *>(M.HeaderFileInfoTable);
if (!Table)
return false;
-
// Look in the on-disk hash table for an entry for this file name.
HeaderFileInfoLookupTable::iterator Pos = Table->find(FE);
+ // Preambles may be reused with different main-file content.
+ // A second entry with size zero is stored for the main-file, try that.
+ // To avoid doing this on every miss, require the bare filename to match.
+ if (Pos == Table->end() && M.Kind == clang::serialization::MK_Preamble &&
+ llvm::sys::path::filename(FE->getName()) ==
+ llvm::sys::path::filename(M.OriginalSourceFileName)) {
+ auto InternalKey = Table->getInfoObj().GetInternalKey(FE);
+ InternalKey.Size = 0;
+ Pos = Table->find_hashed(InternalKey,
+ Table->getInfoObj().ComputeHash(InternalKey));
+ }
if (Pos == Table->end())
return false;
Index: clang/lib/Lex/Lexer.cpp
===================================================================
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -2743,6 +2743,11 @@
if (PP->isRecordingPreamble() && PP->isInPrimaryFile()) {
PP->setRecordedPreambleConditionalStack(ConditionalStack);
+ // If the preamble cuts off the end of a header guard, consider it guarded.
+ // The guard is valid for the preamble content itself, and for tools the
+ // most useful answer is "yes, this file has a header guard".
+ if (!ConditionalStack.empty())
+ MIOpt.ExitTopLevelConditional();
ConditionalStack.clear();
}
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1283,6 +1283,47 @@
EXPECT_THAT(Symbols, Each(IncludeHeader()));
}
+TEST_F(SymbolCollectorTest, HeaderGuardDetectedPragmaInPreamble) {
+ // TestTU builds with a preamble.
+ auto TU = TestTU::withCode(R"cpp(
+ #pragma once
+
+ // Symbols are seen before the header guard is complete.
+ #define MACRO
+ int decl();
+ #define MACRO2
+ )cpp");
+ TU.HeaderFilename = "Foo.h";
+ auto Symbols = TU.headerSymbols();
+ EXPECT_THAT(Symbols, Not(Contains(QName("HEADER_GUARD_"))));
+ EXPECT_THAT(Symbols, Contains(QName("MACRO")));
+ EXPECT_THAT(Symbols, Contains(QName("MACRO2")));
+ EXPECT_THAT(Symbols, Contains(QName("decl")));
+ EXPECT_THAT(Symbols, Each(IncludeHeader()));
+}
+
+TEST_F(SymbolCollectorTest, HeaderGuardDetectedIfdefInPreamble) {
+ // TestTU builds with a preamble.
+ auto TU = TestTU::withCode(R"cpp(
+ #ifndef HEADER_GUARD_
+ #define HEADER_GUARD_
+
+ // Symbols are seen before the header guard is complete.
+ #define MACRO
+ int decl();
+ #define MACRO2
+
+ #endif // Header guard is recognized here.
+ )cpp");
+ TU.HeaderFilename = "Foo.h";
+ auto Symbols = TU.headerSymbols();
+ EXPECT_THAT(Symbols, Not(Contains(QName("HEADER_GUARD_"))));
+ EXPECT_THAT(Symbols, Contains(QName("MACRO")));
+ EXPECT_THAT(Symbols, Contains(QName("MACRO2")));
+ EXPECT_THAT(Symbols, Contains(QName("decl")));
+ EXPECT_THAT(Symbols, Each(IncludeHeader()));
+}
+
TEST_F(SymbolCollectorTest, NonModularHeader) {
auto TU = TestTU::withHeaderCode("int x();");
EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -654,28 +654,25 @@
*ASTCtx, *PP, CodeCompletionContext::CCC_Symbol, *CompletionAllocator,
*CompletionTUInfo,
/*IncludeBriefComments*/ false);
- std::string Documentation =
- formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
- /*CommentsFromHeaders=*/true));
- if (!(S.Flags & Symbol::IndexedForCodeCompletion)) {
- if (Opts.StoreAllDocumentation)
- S.Documentation = Documentation;
- Symbols.insert(S);
- return Symbols.find(S.ID);
+ std::string Documentation;
+ if (Opts.StoreAllDocumentation ||
+ S.Flags & Symbol::IndexedForCodeCompletion) {
+ Documentation =
+ formatDocumentation(*CCS, getDocComment(Ctx, SymbolCompletion,
+ /*CommentsFromHeaders=*/true));
+ S.Documentation = Documentation;
}
- S.Documentation = Documentation;
std::string Signature;
std::string SnippetSuffix;
- getSignature(*CCS, &Signature, &SnippetSuffix);
- S.Signature = Signature;
- S.CompletionSnippetSuffix = SnippetSuffix;
- std::string ReturnType = getReturnType(*CCS);
- S.ReturnType = ReturnType;
-
- llvm::Optional<OpaqueType> TypeStorage;
if (S.Flags & Symbol::IndexedForCodeCompletion) {
- TypeStorage = OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion);
- if (TypeStorage)
+ getSignature(*CCS, &Signature, &SnippetSuffix);
+ S.Signature = Signature;
+ S.CompletionSnippetSuffix = SnippetSuffix;
+ std::string ReturnType = getReturnType(*CCS);
+ S.ReturnType = ReturnType;
+
+ if (auto TypeStorage =
+ OpaqueType::fromCompletionResult(*ASTCtx, SymbolCompletion))
S.Type = TypeStorage->raw();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
