https://github.com/HighCommander4 requested changes to this pull request.
I have some relatively minor comments inline, but there is one more significant issue worth discussing before we proceed: the patch does not correctly handle compilation databases where the `"file"` key is relative to `"directory"`. I wrote an additional testcase, `indexer-clangd-config-relative-path.test`, which demonstrates the issue (i.e. it fails with the current implementation), and added it to the PR. The problem is that the `File` parameter passed to the `ArgumentsAdjuster` comes from the `"file"` field of the compilation database entry. If this is a relative path, `make_absolute` is going to interpret it relative to the **clangd-indexer invocation's** current working directory, which may be different from what it's actually relative to (which is the `"directory"` field of the entry). Unfortunately, this subtle issue is not super easy to fix. The libTooling abstractions that the indexer implementation is relying on (`ToolExecutor` and such) are pretty limiting, and e.g. do not expose the information we need to compute the correct absolute path to the `ArgumentsAdjuster`. As a result, handling this properly would require some upstream changes in libTooling. So let me start by checking: do you care about relative `"file"` paths in the compilation database for your use case? If not, I'm happy to proceed with the current implementation approach, document this as a known limitation until we improve it, and mark the new testcase as known-to-fail. Otherwise, we'll need to delve into the libTooling changes needed to support relative paths properly. https://github.com/llvm/llvm-project/pull/214951 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
