sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG, Just readability stuff.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:39
SrcMgr::CharacteristicKind FileKind) override {
- if (isInsideMainFile(HashLoc, SM)) {
+ auto AddMainFileInc = [&](SourceLocation HashLoc) {
Out->MainFileIncludes.emplace_back();
----------------
nit: Inc -> Include
================
Comment at: clang-tools-extra/clangd/Headers.cpp:39
SrcMgr::CharacteristicKind FileKind) override {
- if (isInsideMainFile(HashLoc, SM)) {
+ auto AddMainFileInc = [&](SourceLocation HashLoc) {
Out->MainFileIncludes.emplace_back();
----------------
sammccall wrote:
> nit: Inc -> Include
this structure is not terrible but the reverse-chronological-order hurts
readability a bit.
You could try:
```
// If an include is part of the preamble patch, translate #line directives.
if (BuiltinFileInStack) {
auto PreLoc = ...;
if (...) {
HashLoc = ...; // now we'll hit the case below
}
}
// Record main-file inclusions (including those mapped from the preamble patch).
if (isInsideMainFile(HashLoc, SM)) {
// add the include
}
```
================
Comment at: clang-tools-extra/clangd/Headers.cpp:55
+ if (BuiltinFileInStack) {
+ // Directives included through builtin file can be mapped back to main
+ // file via line directives.
----------------
can -> might, line -> #line, ... as part of a preamble patch
(just to hint a little more clearly who created the mapping)
================
Comment at: clang-tools-extra/clangd/Headers.cpp:57
+ // file via line directives.
+ auto PreLoc = SM.getPresumedLoc(HashLoc);
+ if (auto FE = SM.getFileManager().getFile(PreLoc.getFilename())) {
----------------
PreLoc -> Presumed?
(these abbreviations aren't common)
================
Comment at: clang-tools-extra/clangd/Headers.cpp:58
+ auto PreLoc = SM.getPresumedLoc(HashLoc);
+ if (auto FE = SM.getFileManager().getFile(PreLoc.getFilename())) {
+ if (SM.getFileEntryForID(MainFID) == *FE) {
----------------
For clarity I think we could do this only with an invalid file ID and pull out
a function, ending up with:
```
if (Presumed.getFileID().isInvalid() && isMainFile(Presumed.getFilename()))
AddMainFileInc(HashLoc);
```
================
Comment at: clang-tools-extra/clangd/Headers.cpp:65
+ }
+ } else if (isInsideMainFile(HashLoc, SM)) {
+ AddMainFileInc(HashLoc);
----------------
nit: I'd find it slightly clearer to handle this case first in the if/else, as
it kind of "motivates" the more complicated one.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:69
+
if (File) {
auto *IncludingFileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc));
----------------
maybe delimit this with a comment like
// Record include graph (not just for main-file includes)
================
Comment at: clang-tools-extra/clangd/Headers.cpp:107
+ // Indicates whether <built-in> file is part of include stack.
+ bool BuiltinFileInStack = false;
+
----------------
Using the name `stack` for a boolean was pretty confusing to me. I think
`InBuiltinFile` plus the comment would be enough, otherwise maybe
`UnderBuiltinFile` or so?
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:309
+TEST_F(HeadersTest, PresumedLocations) {
+ std::string HeaderFile = testPath("implicit_include.h");
+
----------------
I don't think testPath is needed here: it should be on the include path, and
FS.Files adds the qualifiers where needed
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:314
+ HeaderContents += R"cpp(
+#line 1
+#include <a.h>)cpp";
----------------
Can we use more aribtrary placeholder here, like 42? :)
We could easily hit 1 by mistake.
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:324
+ // Now include through built-in file.
+ CDB.ExtraClangFlags = {"-include" + HeaderFile};
+ EXPECT_THAT(collectIncludes().MainFileIncludes,
----------------
changing plus to comma here seems clearer
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78740/new/
https://reviews.llvm.org/D78740
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
