simark marked 3 inline comments as done. simark added a comment. In https://reviews.llvm.org/D48687#1193470, @hokein wrote:
> Sorry for the delay. I thought there was a dependent patch of this patch, but > it seems resolved, right? > > A rough round of review. Right, the patch this one depends on is in clang, and there does not seem to be regressions this time. >> 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). Thanks. ================ 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); ---------------- hokein wrote: > Why rename this function? > > Is it guaranteed that `RealPath` is always an absolute path? Before, it only made sure that the path was absolute, now it goes a step further and makes it a "real path", resolving symlinks and removing `.` and `..`. When we talk about a "real path", it refers to the unix realpath syscall: http://man7.org/linux/man-pages/man3/realpath.3.html So yes, the result is guaranteed to be absolute too. ================ 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. ---------------- hokein wrote: > Can we use `RelPathPrefix.empty()` instead of comparing with `StringRef()`? Done. ================ Comment at: unittests/clangd/XRefsTests.cpp:325 + + Annotations HeaderInPreambleAnnotations(R"cpp( +int [[bar_preamble]]; ---------------- hokein wrote: > It seems that there is no difference between `HeaderInPreambleAnnotations` > and `HeaderNotInPreambleAnnotations` in the test. Both of them are treated > equally. > The difference is that one is included in the main file as part of the preamble and the other is out of the preamble. Since they take different code paths in clang, I think it's good to test both. At some point, I had a similar bug that would only happen with a header included in the preamble. 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
