-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Properly set up C++ include paths and similar environment settings when parsing imported C++. #5767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Properly set up C++ include paths and similar environment settings when parsing imported C++. #5767
Conversation
The tooling interface doesn't work well here -- it expects frontend arguments, not driver arguments, and is a bit too high-level. Create an ASTUnit directly instead, and pass driver arguments to its construction.
Plumb through the clang path rather than specifying a resource directory manually. Add line marker directives to point diagnostics from clang back at the corresponding Carbon import.
Unsure if you were aware of #5543 which is also touching how we invoke Clang. How would you like to proceed in terms of merging the two approaches? |
references to different files. Base the line delta on the file mentioned in that particular line number reference, not the file number associated with the check line.
I think they're mostly independent, though they do overlap in that they both contain the change to switch to passing driver arguments and mapping them to frontend arguments. That's a necessary part of this change, but seems to be more of an incidental fix in #5543, so I suggested reverting that part of that PR, but I'm also happy with that landing as-is and rebasing this on top (or the opposite way around). |
Sounds good, let's work on landing this and then #5543 can rebase on top of this one I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass comments...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split out #5783 with a mostly-separable piece of this (though I've not removed the changes from this PR yet). Let me know if there are any other parts you'd like split out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor nits, LG with suggested edits.
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
3776e46
Stop using the clang tooling library to build an ASTUnit; that library is set up to process clang frontend arguments, assuming that something has already built frontend arguments from the compiler arguments. It is also too encapsulated and doesn't let us inspect and modify the compiler invocation before it's executed.
Instead, build the AST unit directly in two phases:
clang::createInvocation
. Internally, this uses the clang driver to build a frontend invocation, including building system-specific include paths as needed.I've factored this so that we can split out the
createInvocation
step, with the intention that we may want to move it out of check and into the carbon driver with the rest of the driver-level argument handling, and we may want to customize some of the clang options before we invoke the clang frontend with that set of options.In order to make the invocation reusable, it no longer depends on the name of the carbon file importing the C++ code. In place of synthesizing a header file name as
<foo.carbon>.generated.cpp_imports.h
, we now insert line marker directives into the generated header so that errors in that header cause Clang to point a diagnostic back at the Carbon source file itself. This results in a minor improvement in the diagnostic output: we no longer refer to a nonexistent generated file. But the snippet still contains text that doesn't match the source code, so it remains imperfect.