ast-exporter: pass compilation database directory to Clang - #1963
ast-exporter: pass compilation database directory to Clang#1963ShiroKSH wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
CommonOptionsParser treats -p as a build directory and searches that directory for the compilation database. The AST exporter passed the JSON file itself, so newer Clang versions attempted to treat the file as a directory and continued without the database flags.
https://github.com/llvm/llvm-project/commits/main/clang/lib/Tooling/CommonOptionsParser.cpp has not changed its functionality since 2022. I also don't see any recent changes to https://github.com/llvm/llvm-project/commits/main/clang/lib/Tooling/CompilationDatabase.cpp that would make this a clang 22 specific issue.
I do agree that we ought to pass the parent directory of the compilation database, so that part looks good to me. However, I think we ought to clarify whether this is a general correctness fix or what versions of clang need this.
nitpick: C2Rust validation means passing CI so we do not need a validation section. Generally, PRs bodies are shorter and not split into sections. I'd encourage you to take a look at other PRs and make yours follow project conventions.
a33d5e1 to
2b80bee
Compare
|
Thanks — I shortened the PR body to match the project convention and clarified that this is a general correctness fix; #1809 only exposed the existing mismatch on Clang 22. I also rebased the branch onto the current |
102b4f5 to
b017f2a
Compare
Fixes #1809.
CommonOptionsParserexpects-pto name the build directory, but the AST exporter passedcompile_commands.jsonitself. Pass its parent directory instead, using.for a bare relative path, and cover both forms.This is a general correctness fix; #1809 exposed the existing mismatch on Clang 22.