Refactor the clang invocations to make them more uniform and robust:
* make sure that we correctly retain the exit code from clang and fail tests on
errors
* unify the clang invocations into one, so that we can share the command line
configuration
* fix the command line to correctly use the clang binary name as the first
entry and replace the faulty `--syntax-only` with `-fsyntax-only`
PiperOrigin-RevId: 402220726
diff --git a/rs_bindings_from_cc/ast_visitor_test.cc b/rs_bindings_from_cc/ast_visitor_test.cc
index 06a46d9..7adc000 100644
--- a/rs_bindings_from_cc/ast_visitor_test.cc
+++ b/rs_bindings_from_cc/ast_visitor_test.cc
@@ -26,6 +26,7 @@
using ::testing::Property;
using ::testing::SizeIs;
using ::testing::VariantWith;
+using ::testing::status::StatusIs;
// Matches an IR node that has the given identifier.
MATCHER_P(IdentifierIs, identifier, "") {
@@ -207,27 +208,28 @@
}
TEST(AstVisitorTest, Noop) {
- IR ir = IrFromCc({"// nothing interesting there."}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"// nothing interesting there."}));
+
EXPECT_THAT(ir.items, IsEmpty());
EXPECT_THAT(ir.used_headers,
ElementsAre(Property(&HeaderName::IncludePath,
"test/testing_header_0.h")));
}
-TEST(AstVisitorTest, IREmptyOnInvalidInput) {
- IR ir = IrFromCc({"int foo(); But this is not C++"}, {});
- EXPECT_THAT(ir.items, IsEmpty());
+TEST(AstVisitorTest, ErrorOnInvalidInput) {
+ ASSERT_THAT(IrFromCc({"int foo(); But this is not C++"}),
+ StatusIs(absl::StatusCode::kInvalidArgument));
}
TEST(AstVisitorTest, FuncWithVoidReturnType) {
- IR ir = IrFromCc({"void Foo();"}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"void Foo();"}));
EXPECT_THAT(ir.items, ElementsAre(VariantWith<Func>(
AllOf(IdentifierIs("Foo"), MangledNameIs("_Z3Foov"),
ReturnType(IsVoid()), ParamsAre()))));
}
TEST(AstVisitorTest, TwoFuncs) {
- IR ir = IrFromCc({"void Foo(); void Bar();"}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"void Foo(); void Bar();"}));
EXPECT_THAT(
ir.items,
ElementsAre(
@@ -238,32 +240,32 @@
}
TEST(AstVisitorTest, TwoFuncsFromTwoHeaders) {
- IR ir = IrFromCc({"void Foo();", "void Bar();"}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"void Foo();", "void Bar();"}));
EXPECT_THAT(ir.items, ElementsAre(VariantWith<Func>(IdentifierIs("Foo")),
VariantWith<Func>(IdentifierIs("Bar"))));
}
TEST(AstVisitorTest, NonInlineFunc) {
- IR ir = IrFromCc({"void Foo() {}"}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"void Foo() {}"}));
EXPECT_THAT(ir.items, ElementsAre(VariantWith<Func>(
AllOf(IdentifierIs("Foo"), Not(IsInline())))));
}
TEST(AstVisitorTest, InlineFunc) {
- IR ir = IrFromCc({"inline void Foo() {}"}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"inline void Foo() {}"}));
EXPECT_THAT(
ir.items,
ElementsAre(VariantWith<Func>(AllOf(IdentifierIs("Foo"), IsInline()))));
}
TEST(AstVisitorTest, FuncJustOnce) {
- IR ir = IrFromCc({"void Foo(); void Foo();"}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"void Foo(); void Foo();"}));
EXPECT_THAT(ir.items,
ElementsAre(VariantWith<Func>(AllOf(IdentifierIs("Foo")))));
}
TEST(AstVisitorTest, TestImportPointerFunc) {
- IR ir = IrFromCc({"int* Foo(int* a);"}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({"int* Foo(int* a);"}));
EXPECT_THAT(ir.items,
ElementsAre(VariantWith<Func>(AllOf(
@@ -271,8 +273,9 @@
}
TEST(AstVisitorTest, Struct) {
- IR ir = IrFromCc(
- {"struct SomeStruct { int first_field; int second_field; };"}, {});
+ ASSERT_OK_AND_ASSIGN(
+ IR ir,
+ IrFromCc({"struct SomeStruct { int first_field; int second_field; };"}));
EXPECT_THAT(ir.items,
ElementsAre(VariantWith<Record>(AllOf(
@@ -287,7 +290,7 @@
absl::string_view file =
"struct Implicit {};\n"
"struct Defaulted { Defaulted(const Defaulted&) = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(CopyConstructor(DefinitionIs(
@@ -299,7 +302,7 @@
// TODO(b/202113881): "struct MemberImplicit { Defined x; };\n"
// TODO(b/202113881): "struct MemberDefaulted { MemberDefaulted(const
// MemberDefaulted&) = default; Defined x; };\n"
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(1));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(CopyConstructor(DefinitionIs(
SpecialMemberFunc::Definition::kNontrivial)))));
@@ -310,7 +313,7 @@
"struct Deleted { Deleted(const Deleted&) = delete;};\n"
// TODO(b/202113881): "struct DeletedByMember { Deleted x; };\n"
"struct DeletedByCtorDef { DeletedByCtorDef(DeletedByCtorDef&&) {} };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(CopyConstructor(DefinitionIs(
@@ -322,7 +325,7 @@
"class Implicit {};\n"
"struct Defaulted { Defaulted(const Defaulted&) = default; };\n"
"class Section { public: Section(const Section&) = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(3));
EXPECT_THAT(ir.items,
@@ -333,7 +336,7 @@
absl::string_view file =
"class Defaulted { Defaulted(const Defaulted&) = default; };\n"
"struct Section { private: Section(const Section&) = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items,
@@ -344,7 +347,7 @@
absl::string_view file =
"struct Implicit {};\n"
"struct Defaulted { Defaulted(Defaulted&&) = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(MoveConstructor(DefinitionIs(
@@ -356,7 +359,7 @@
// TODO(b/202113881): "struct MemberImplicit { Defined x; };\n"
// TODO(b/202113881): "struct MemberDefaulted { MemberDefaulted(
// MemberDefaulted&&) = default; Defined x; };\n"
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(1));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(MoveConstructor(DefinitionIs(
SpecialMemberFunc::Definition::kNontrivial)))));
@@ -368,7 +371,7 @@
// TODO(b/202113881): "struct DeletedByMember { Deleted x; };\n"
"struct SuppressedByCtorDef {"
" SuppressedByCtorDef(const SuppressedByCtorDef&) {}};\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(MoveConstructor(DefinitionIs(
@@ -380,7 +383,7 @@
"class Implicit {};\n"
"struct Defaulted { Defaulted(Defaulted&&) = default; };\n"
"class Section { public: Section(Section&&) = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(3));
EXPECT_THAT(ir.items,
@@ -391,7 +394,7 @@
absl::string_view file =
"class Defaulted { Defaulted(Defaulted&&) = default; };\n"
"struct Section { private: Section(Section&&) = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items,
@@ -402,7 +405,7 @@
absl::string_view file =
"struct Implicit {};\n"
"struct Defaulted { ~Defaulted() = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(Destructor(DefinitionIs(
@@ -414,7 +417,7 @@
// TODO(b/202113881): "struct MemberImplicit { Defined x; };\n"
// TODO(b/202113881): "struct MemberDefaulted { ~MemberDefaulted() = default;
// Defined x; };\n"
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(1));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(Destructor(DefinitionIs(
SpecialMemberFunc::Definition::kNontrivial)))));
@@ -423,7 +426,7 @@
TEST(AstVisitorTest, DeletedDestructor) {
absl::string_view file = "struct Deleted { ~Deleted() = delete;};\n";
// TODO(b/202113881): "struct DeletedByMember { Deleted x; };\n"
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(1));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(Destructor(DefinitionIs(
@@ -435,7 +438,7 @@
"class Implicit {};\n"
"struct Defaulted { ~Defaulted() = default; };\n"
"class Section { public: ~Section() = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(3));
EXPECT_THAT(ir.items,
@@ -446,7 +449,7 @@
absl::string_view file =
"class Defaulted { ~Defaulted() = default; };\n"
"struct Section { private: ~Section() = default; };\n";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(2));
EXPECT_THAT(ir.items,
@@ -463,7 +466,7 @@
Nontrivial(const Nontrivial&) {}
};
)cc";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(3));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(IsTrivialAbi())));
@@ -475,14 +478,14 @@
Nontrivial(const Nontrivial&) {}
};
)cc";
- IR ir = IrFromCc({file}, {});
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({file}));
EXPECT_THAT(ir.items, SizeIs(1));
EXPECT_THAT(ir.items, Each(VariantWith<Record>(Not(IsTrivialAbi()))));
}
TEST(AstVisitorTest, MemberVariableAccessSpecifiers) {
- IR ir = IrFromCc({R"(
+ ASSERT_OK_AND_ASSIGN(IR ir, IrFromCc({R"(
struct SomeStruct {
int default_access_int;
public:
@@ -496,8 +499,7 @@
class SomeClass {
int default_access_int;
};
- )"},
- {});
+ )"}));
EXPECT_THAT(
ir.items,
diff --git a/rs_bindings_from_cc/ir_from_cc.cc b/rs_bindings_from_cc/ir_from_cc.cc
index b32e8e4..8ff68dd 100644
--- a/rs_bindings_from_cc/ir_from_cc.cc
+++ b/rs_bindings_from_cc/ir_from_cc.cc
@@ -12,11 +12,13 @@
namespace rs_bindings_from_cc {
static constexpr absl::string_view kVirtualInputPath =
- "ast_visitor_test_virtual_input.cc";
+ "ir_from_cc_virtual_input.cc";
-IR IrFromCc(absl::Span<const absl::string_view> header_files_contents,
- const std::vector<absl::string_view>& args) {
- std::vector<std::string> headers;
+absl::StatusOr<IR> IrFromCc(
+ absl::Span<const absl::string_view> header_files_contents,
+ absl::Span<const absl::string_view> header_names,
+ absl::Span<const absl::string_view> args) {
+ std::vector<std::string> headers{header_names.begin(), header_names.end()};
absl::flat_hash_map<std::string, std::string> file_contents;
std::string virtual_input_file_content;
@@ -25,27 +27,39 @@
std::string filename(
absl::Substitute("test/testing_header_$0.h", counter++));
file_contents.insert({filename, std::string(header_content)});
- absl::SubstituteAndAppend(&virtual_input_file_content, "#include \"$0\"\n",
- filename);
headers.push_back(std::move(filename));
}
+ for (const std::string& filename : headers) {
+ absl::SubstituteAndAppend(&virtual_input_file_content, "#include \"$0\"\n",
+ filename);
+ }
file_contents.insert(
{std::string(kVirtualInputPath), virtual_input_file_content});
- std::vector<std::string> args_as_strings(args.begin(), args.end());
- args_as_strings.push_back("--syntax-only");
- // Needed, so that we can copy over non-doc comments that are used as
- // documention.
- args_as_strings.push_back("-fparse-all-comments");
+ std::vector<std::string> args_as_strings{
+ // This includes the path of the binary that we're pretending to be.
+ // TODO(forster): Find out where to point this. Should we use the
+ // production crosstool for this? See
+ // http://google3/devtools/cymbal/common/clang_tool.cc;l=171;rcl=385188113
+ "clang",
+ // We only need the AST.
+ "-fsyntax-only",
+ // Parse non-doc comments that are used as documention.
+ "-fparse-all-comments"};
+ args_as_strings.insert(args_as_strings.end(), args.begin(), args.end());
args_as_strings.push_back(std::string(kVirtualInputPath));
- IR ir;
- devtools::cymbal::RunToolWithClangFlagsOnCode(
- args_as_strings, file_contents,
- std::make_unique<rs_bindings_from_cc::FrontendAction>(
- std::vector<absl::string_view>(headers.begin(), headers.end()), ir));
- return ir;
+ if (IR ir; devtools::cymbal::RunToolWithClangFlagsOnCode(
+ args_as_strings, file_contents,
+ std::make_unique<FrontendAction>(
+ std::vector<absl::string_view>(headers.begin(), headers.end()),
+ ir))) {
+ return ir;
+ } else {
+ return absl::Status(absl::StatusCode::kInvalidArgument,
+ "Could not compile header contents");
+ }
}
} // namespace rs_bindings_from_cc
diff --git a/rs_bindings_from_cc/ir_from_cc.h b/rs_bindings_from_cc/ir_from_cc.h
index 5e6c8c7..19834c6 100644
--- a/rs_bindings_from_cc/ir_from_cc.h
+++ b/rs_bindings_from_cc/ir_from_cc.h
@@ -8,13 +8,24 @@
#include <vector>
#include "rs_bindings_from_cc/ir.h"
+#include "third_party/absl/status/statusor.h"
#include "third_party/absl/strings/string_view.h"
#include "third_party/absl/types/span.h"
namespace rs_bindings_from_cc {
-IR IrFromCc(absl::Span<const absl::string_view> header_files_contents,
- const std::vector<absl::string_view>& args);
+/// Parses C++ source code into IR.
+///
+/// Parameters:
+/// * `header_file_contents`: textual C++ source code to be parsed directly
+/// * `header_names`: names of headers to include from the file system before
+/// the textual source
+/// * `args`: additional command line arguments for Clang
+///
+absl::StatusOr<IR> IrFromCc(
+ absl::Span<const absl::string_view> header_files_contents,
+ absl::Span<const absl::string_view> header_names = {},
+ absl::Span<const absl::string_view> args = {});
} // namespace rs_bindings_from_cc
diff --git a/rs_bindings_from_cc/json_from_cc.cc b/rs_bindings_from_cc/json_from_cc.cc
index 0c19eb9..37e4d1d 100644
--- a/rs_bindings_from_cc/json_from_cc.cc
+++ b/rs_bindings_from_cc/json_from_cc.cc
@@ -4,13 +4,19 @@
#include "rs_bindings_from_cc/ffi_types.h"
#include "rs_bindings_from_cc/ir_from_cc.h"
+#include "util/task/status.h"
namespace rs_bindings_from_cc {
// This is intended to be called from Rust.
extern "C" FfiU8SliceBox json_from_cc(FfiU8Slice cc_source) {
- IR ir = IrFromCc({StringViewFromFfiU8Slice(cc_source)}, {});
- std::string json = ir.ToJson().dump();
+ absl::StatusOr<IR> ir = IrFromCc({StringViewFromFfiU8Slice(cc_source)});
+ // TODO(forster): For now it is good enough to just exit: We are just using
+ // this from tests, which are ok to just fail. Clang has already printed error
+ // messages. If we start using this for production, then we should bridge the
+ // error code into Rust.
+ CHECK_OK(ir);
+ std::string json = ir->ToJson().dump();
return AllocFfiU8SliceBox(MakeFfiU8Slice(json));
}
diff --git a/rs_bindings_from_cc/rs_bindings_from_cc.cc b/rs_bindings_from_cc/rs_bindings_from_cc.cc
index e45dd2d..b5ce039 100644
--- a/rs_bindings_from_cc/rs_bindings_from_cc.cc
+++ b/rs_bindings_from_cc/rs_bindings_from_cc.cc
@@ -15,6 +15,7 @@
#include "devtools/cymbal/common/clang_tool.h"
#include "rs_bindings_from_cc/frontend_action.h"
#include "rs_bindings_from_cc/ir.h"
+#include "rs_bindings_from_cc/ir_from_cc.h"
#include "rs_bindings_from_cc/src_code_gen.h"
#include "file/base/filesystem.h"
#include "file/base/helpers.h"
@@ -38,9 +39,6 @@
"for, in a format suitable for usage in google3-relative quote "
"include (#include \"\").");
-constexpr absl::string_view kVirtualInputPath =
- "rs_bindings_from_cc_virtual_input.cc";
-
int main(int argc, char* argv[]) {
InitGoogle(argv[0], &argc, &argv, true);
@@ -54,34 +52,20 @@
auto ir_out = absl::GetFlag(FLAGS_ir_out); // Optional.
- std::vector<std::string> command_line(argv, argv + argc);
- // Needed, so that we can copy over non-doc comments that are used as
- // documention.
- command_line.push_back("-fparse-all-comments");
- command_line.push_back(std::string(kVirtualInputPath));
-
- std::string virtual_input_file_content;
- for (const std::string& header : public_headers) {
- absl::SubstituteAndAppend(&virtual_input_file_content, "#include \"$0\"\n",
- header);
- }
-
- absl::flat_hash_map<std::string, std::string> file_contents{
- {std::string(kVirtualInputPath), virtual_input_file_content}};
-
- rs_bindings_from_cc::IR ir;
- if (devtools::cymbal::RunToolWithClangFlagsOnCode(
- command_line, file_contents,
- std::make_unique<rs_bindings_from_cc::FrontendAction>(
+ if (absl::StatusOr<rs_bindings_from_cc::IR> ir =
+ rs_bindings_from_cc::IrFromCc(
+ {},
std::vector<absl::string_view>(public_headers.begin(),
public_headers.end()),
- ir))) {
+
+ std::vector<absl::string_view>(argv, argv + argc));
+ ir.ok()) {
if (!ir_out.empty()) {
- CHECK_OK(file::SetContents(ir_out, ir.ToJson().dump(/*indent=*/2),
+ CHECK_OK(file::SetContents(ir_out, ir->ToJson().dump(/*indent=*/2),
file::Defaults()));
}
rs_bindings_from_cc::Bindings bindings =
- rs_bindings_from_cc::GenerateBindings(ir);
+ rs_bindings_from_cc::GenerateBindings(*ir);
CHECK_OK(file::SetContents(rs_out, bindings.rs_api, file::Defaults()));
CHECK_OK(file::SetContents(cc_out, bindings.rs_api_impl, file::Defaults()));
return 0;
