sammccall added a comment. This is excellent, a pretty nice model in the end! Good job puzzling it out, when we discussed the idea in the past I imagined this part would be a pile of hacks.
Thanks for the offline help understanding this, too... ================ Comment at: clang-tools-extra/clangd/ParsedAST.cpp:268 + PreamblePatch Patch = Preamble + ? PreamblePatch::create(Filename, Inputs, *Preamble) ---------------- As discussed offline, might be clearer not to have the patch exist unless the preamble does. ================ Comment at: clang-tools-extra/clangd/ParsedAST.h:51 static llvm::Optional<ParsedAST> - build(llvm::StringRef Version, std::unique_ptr<clang::CompilerInvocation> CI, + build(const ParseInputs &Inputs, + std::unique_ptr<clang::CompilerInvocation> CI, ---------------- ah, this is going to conflict with https://github.com/llvm/llvm-project/commit/6880c4dfa3981ec26d44b5f4844b641342a4e3e8 Sorry about that, but that change also ParseInputs available here so I think you can just drop your changes. ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:280 const PreambleData &Baseline) { + assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!"); // First scan the include directives in Baseline and Modified. These will be ---------------- BTW do we want to check isPreambleCompatible and bail out early if it is, or is that the caller's job? ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:332 + for (auto &Inc : *ModifiedIncludes) { + auto It = ExistingIncludes.find({Inc.Directive, Inc.Written}); + // Include already present in the baseline preamble. Set resolved path and ---------------- This explicitly looks at the existing includes based on spelling, and not whether the include was resolved during scanning or not. One implication is that if you insert an #include that was already transitively included (which IIUC will hit the preamble VFS cache and thus be resolved) then we're not going to take advantage of this to record its resolution now. Instead we're going to parse it along with the mainfile, and... well, I *hope* everything works out: - do we pay for the stat, or is the preamble's stat cache still in effect? - if the file is include-guarded, we're not going to read or parse it I guess? - if it's not include-guarded but #imported, then again we won't re-read or re-parse it because we use the same directive? - if the file isn't include-guarded, I guess we must read it (because preamble doesn't contain the actual buffer) and then parse it, but such is life. ================ Comment at: clang-tools-extra/clangd/Preamble.cpp:340 } // FIXME: Traverse once auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset); ---------------- nice to have a comment similar to the one on line 333 for this case ``` // Include is new in the modified preamble. // Inject it into the patch and use #line to set the presumed location to where it is spelled ``` ================ Comment at: clang-tools-extra/clangd/Preamble.h:112 + /// Adjusts \p BaselineIncludes to reflect state of \p Modified. This won't + /// add any new includes but rather drop the deleted ones. Note that it won't ---------------- This comment explains what but not why, and the why is pretty non-obvious here. Assuming we switch to returning a vector, I'd call it `preambleIncludes()` and say something like: ``` Returns #include directives from the \c Modified preamble that were resolved using the \c Baseline preamble. This covers the new locations of inclusions that were moved around, but not inclusions of new files. Those will be recorded when parsing the main file: the includes in the injected section will be resolved back to their spelled positions in the main file using the presumed-location mechanism. ``` ("injected section" isn't a term we've defined anywhere, it'd be good to give this idea some name in the class comment as it's the main mechanism behind preamble patching) ================ Comment at: clang-tools-extra/clangd/Preamble.h:115 + /// mutate include depths. + void patchPreambleIncludes(IncludeStructure &BaselineIncludes) const; + ---------------- kadircet wrote: > i am not happy about this interface. I was thinking about a > `std::vector<Inclusion> remainingBaselineIncludes() const` but didn't go for > it to keep the concept of `patching`. > > I suppose the former is more intuitive though, WDYT? Looking at the callsite, I think I like returning a vector better. buildAST is copyng a data structure specifically to be patched, and the patch is just overwriting it. The interface looks good but doesn't really reflect what's going on. To make the vector thing work cleanly, I think we should have it always overwrite. This implies it needs to have the preamble's includes, so the default constructor needs to go away. I'd suggest adding a static factory `PreamblePatch::unmodified(const PreambleData&)` and hiding all constructors. This means we can't have a PreamblePatch instance with no preamble, which means dealing with Optional in places but it also seems clearer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https://reviews.llvm.org/D77644 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
