hokein added a comment. Sorry for the delay. I thought there was a dependent patch of this patch, but it seems resolved, right?
A rough round of review. > New version. I tried to share the code between this and SymbolCollector, but > didn't find a good clean way. Do you have concrete suggestions on how to do > this? Otherwise, would it be acceptable to merge it as-is? Yeah, I'm fine with the current change. I was not aware of the `getAbsoluteFilePath` has been pulled out to the `SourceCode.h`. I think the SymbolCollector could use this function as well (but that's out of scope of this patch). ================ Comment at: clangd/SourceCode.h:65 /// Get the absolute file path of a given file entry. -llvm::Optional<std::string> getAbsoluteFilePath(const FileEntry *F, - const SourceManager &SourceMgr); +llvm::Optional<std::string> getRealPath(const FileEntry *F, + const SourceManager &SourceMgr); ---------------- Why rename this function? Is it guaranteed that `RealPath` is always an absolute path? ================ Comment at: unittests/clangd/TestFS.cpp:52 CommandLine.insert(CommandLine.begin(), "clang"); - CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File); - return {tooling::CompileCommand(sys::path::parent_path(File), FileName, - std::move(CommandLine), "")}; + if (RelPathPrefix == StringRef()) { + // Use the absolute path in the compile command. ---------------- Can we use `RelPathPrefix.empty()` instead of comparing with `StringRef()`? ================ Comment at: unittests/clangd/XRefsTests.cpp:325 + + Annotations HeaderInPreambleAnnotations(R"cpp( +int [[bar_preamble]]; ---------------- It seems that there is no difference between `HeaderInPreambleAnnotations` and `HeaderNotInPreambleAnnotations` in the test. Both of them are treated equally. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48687 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits