Generate AST when importing cpp files #4790
Conversation
Ignore the AST, for now. Report cpp compilation errors and warnings. Part of carbon-language#4666
toolchain/check/check_unit.cpp
Outdated
| llvm::raw_ostream* vlog_stream) | ||
| : unit_and_imports_(unit_and_imports), | ||
| total_ir_count_(total_ir_count), | ||
| fs_(fs), |
There was a problem hiding this comment.
| fs_(fs), | |
| fs_(std::move(fs)), |
(avoid refcount churn)
|
|
||
| auto file = fs_->openFileForRead(cpp_file_path); | ||
| if (!file) { | ||
| CARBON_DIAGNOSTIC(CppInteropFileNotFound, Error, |
There was a problem hiding this comment.
| CARBON_DIAGNOSTIC(CppInteropFileNotFound, Error, | |
| CARBON_DIAGNOSTIC(CppInteropCantReadFile, Error, |
The text and the name say different things, they could be more similar?
| if (num_errors > 0) { | ||
| // TODO: Remove the warnings part when there are no warnings. | ||
| CARBON_DIAGNOSTIC( | ||
| CppInteropCodeErrors, Error, |
There was a problem hiding this comment.
| CppInteropCodeErrors, Error, | |
| CppInteropParseError, Error, |
| context.emitter().Emit(loc, CppInteropCodeErrors, num_errors, num_warnings, | ||
| file_path.str(), diagnostics_str); | ||
| } else if (num_warnings > 0) { | ||
| CARBON_DIAGNOSTIC(CppInteropCodeWarnings, Warning, |
There was a problem hiding this comment.
| CARBON_DIAGNOSTIC(CppInteropCodeWarnings, Warning, | |
| CARBON_DIAGNOSTIC(CppInteropParseWarning, Warning, |
|
|
||
| library "[[@TEST_NAME]]"; | ||
|
|
||
| // CHECK:STDERR: fail_cpp_file_not_found.carbon:[[@LINE+3]]:1: error: file 'not_found.h' couldn't be opened for reading: No such file or directory [CppInteropFileNotFound] |
There was a problem hiding this comment.
I think the "No such file or directory" reason could move to a note here
| llvm::ArrayRef(DiagnosticKinds), | ||
| llvm::ArrayRef(UntestedDiagnosticKinds)); | ||
| Testing::TestKindCoverage( | ||
| absl::GetFlag(FLAGS_testdata_manifest), R"( \[([A-Z]\w*)\]$)", |
There was a problem hiding this comment.
This regex will match most anything now, is it still effectively testing something?
There was a problem hiding this comment.
This is great!
I'm looking at this, and thought of a lateral issue... We're fuzz testing, and as soon as the fuzzer discovers it can write C++ imports we'll end up fuzz testing Clang, and Clang is not crash-resilient. The driver already has a DriverEnv::fuzzing flag, can you have it lead to an error when set and importing from Cpp?
| bool is_cpp = | ||
| import_package_name == CppPackageName && !import_library_name.empty(); | ||
|
|
||
| if (is_cpp) { | ||
| unit_info.cpp_imports.push_back(import); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Would it make sense to move this above is_explicit_main since is_explicit_main isn't used in this path? Also, does is_cpp need the local or is it only used here? (note for contrast, is_explicit_main is referenced in a few spots)
| bool is_cpp = | ||
| import_package_name == CppPackageName && !import_library_name.empty(); |
There was a problem hiding this comment.
Can you add some tests for things like:
import package Cpp;-- should probably be complaining thatCppneeds a library, might complain that there's noapifile right now.import package Cpp library "";- "file not found" seems fineimport package Cpp library "\"foo.h\"";- ???
| ImportCppFile(context_, import.node_id, cpp_file_path, | ||
| buffer.get()->getBuffer()); |
There was a problem hiding this comment.
Versus loading each C++ file individually, had you considered generating a file like:
std::string content;
for (const auto& import : unit_and_imports_->cpp_imports) {
llvm::StringRef cpp_file_path =
unit_and_imports_->unit->value_stores->string_literal_values().Get(
import.library_id);
content += llvm::formatv("#include \"{0}\"\n", cpp_file_path);
}
ImportCppFile(context_, import.node_id, carbon_file_name + '.interop.cpp", content);
One particular consequence of that kind of approach is that if you have something like C++ headers that interact (e.g. one #define's a value, the next has a #ifdef on it) they'd work just like they would in C++. Or if there are overloaded functions, it lets C++ decide how to handle them. Thoughts?
(note, my example uses a simple #include, but down the road we'll want to think about how to work with C++ modules too)
There was a problem hiding this comment.
That's a good idea. It'll also be faster as it'll have to build/walk an AST once instead of N times, and only has to go through transitive includes once.
Ignore the AST, for now.
Report cpp compilation errors and warnings.
Part of #4666