union: default constructor inits with first member
CheckValid() checks if union is not-empty.
Bug: 150948558
Test: aidl_unittests / aidl_integration_test
Change-Id: I493b27c84977c3984d599ff7a7a32b0acdd5457f
diff --git a/aidl_language.cpp b/aidl_language.cpp
index 04a4493..3bf8c47 100644
--- a/aidl_language.cpp
+++ b/aidl_language.cpp
@@ -1136,6 +1136,52 @@
writer->Write("}\n");
}
+bool AidlUnionDecl::CheckValid(const AidlTypenames& typenames) const {
+ // visit parent
+ if (!AidlParcelable::CheckValid(typenames)) {
+ return false;
+ }
+ // visit members
+ for (const auto& v : GetFields()) {
+ if (!v->CheckValid(typenames)) {
+ return false;
+ }
+ }
+
+ // now, visit self!
+ bool success = true;
+
+ // TODO(b/170807936) do we need to allow ParcelableHolder in union?
+ for (const auto& v : GetFields()) {
+ if (v->GetType().GetName() == "ParcelableHolder") {
+ AIDL_ERROR(*v) << "A union can't have a member of ParcelableHolder '" << v->GetName() << "'";
+ success = false;
+ }
+ }
+
+ // first member should have default value (implicit or explicit)
+ if (GetFields().empty()) {
+ AIDL_ERROR(*this) << "The union '" << this->GetName() << "' has no fields.";
+ return false;
+ }
+
+ return success;
+}
+
+// TODO: we should treat every backend all the same in future.
+bool AidlUnionDecl::LanguageSpecificCheckValid(const AidlTypenames& typenames,
+ Options::Language lang) const {
+ if (!AidlParcelable::LanguageSpecificCheckValid(typenames, lang)) {
+ return false;
+ }
+ for (const auto& v : this->GetFields()) {
+ if (!v->GetType().LanguageSpecificCheckValid(typenames, lang)) {
+ return false;
+ }
+ }
+ return true;
+}
+
// TODO: we should treat every backend all the same in future.
bool AidlInterface::LanguageSpecificCheckValid(const AidlTypenames& typenames,
Options::Language lang) const {
diff --git a/aidl_language.h b/aidl_language.h
index 6a5932d..e8bf546 100644
--- a/aidl_language.h
+++ b/aidl_language.h
@@ -875,10 +875,9 @@
const std::vector<std::unique_ptr<AidlVariableDeclaration>>& GetFields() const {
return variables_;
}
- bool LanguageSpecificCheckValid(const AidlTypenames& /*typenames*/,
- Options::Language) const override {
- return true;
- }
+ bool CheckValid(const AidlTypenames& typenames) const override;
+ bool LanguageSpecificCheckValid(const AidlTypenames& typenames,
+ Options::Language lang) const override;
std::string GetPreprocessDeclarationName() const override { return "union"; }
void Dump(CodeWriter* writer) const override;
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp
index 0471570..8848c82 100644
--- a/aidl_unittest.cpp
+++ b/aidl_unittest.cpp
@@ -2692,6 +2692,23 @@
EXPECT_EQ(expected_stderr, GetCapturedStderr());
}
+TEST_P(AidlTest, UnionRejectsEmptyDecl) {
+ const string method = "package a; union Foo {}";
+ const string expected_stderr = "ERROR: a/Foo.aidl:1.17-21: The union 'Foo' has no fields.\n";
+ CaptureStderr();
+ EXPECT_EQ(nullptr, Parse("a/Foo.aidl", method, typenames_, GetLanguage()));
+ EXPECT_THAT(GetCapturedStderr(), testing::HasSubstr(expected_stderr));
+}
+
+TEST_P(AidlTest, UnionRejectsParcelableHolder) {
+ const string method = "package a; union Foo { ParcelableHolder x; }";
+ const string expected_stderr =
+ "ERROR: a/Foo.aidl:1.40-42: A union can't have a member of ParcelableHolder 'x'\n";
+ CaptureStderr();
+ EXPECT_EQ(nullptr, Parse("a/Foo.aidl", method, typenames_, GetLanguage()));
+ EXPECT_THAT(GetCapturedStderr(), testing::HasSubstr(expected_stderr));
+}
+
TEST_P(AidlTest, GenericStructuredParcelable) {
io_delegate_.SetFileContents("Foo.aidl", "parcelable Foo<T, U> { int a; int A; }");
Options options =
diff --git a/generate_cpp.cpp b/generate_cpp.cpp
index 6843e0b..447c6fc 100644
--- a/generate_cpp.cpp
+++ b/generate_cpp.cpp
@@ -1036,6 +1036,11 @@
NestInNamespaces(std::move(decls), interface.GetSplitPackage())}};
}
+string GetInitializer(const AidlTypenames& typenames, const AidlVariableDeclaration& variable) {
+ string cppType = CppNameOf(variable.GetType(), typenames);
+ return cppType + "(" + variable.ValueString(ConstantValueDecorator) + ")";
+}
+
template <typename ParcelableType>
struct ParcelableTraits {
static void AddIncludes(set<string>& includes);
@@ -1067,8 +1072,7 @@
std::string cppType = CppNameOf(variable->GetType(), typenames);
out << cppType.c_str() << " " << variable->GetName().c_str();
if (variable->GetDefaultValue()) {
- out << " = " << cppType.c_str() << "(" << variable->ValueString(ConstantValueDecorator)
- << ")";
+ out << " = " << GetInitializer(typenames, *variable);
} else if (variable->GetType().GetName() == "ParcelableHolder") {
if (decl.IsVintfStability()) {
out << " { ::android::Parcelable::Stability::STABILITY_VINTF }";
@@ -1161,26 +1165,34 @@
}
clazz.AddPublic(std::move(enum_tag));
- auto name = decl.GetName();
+ const auto& name = decl.GetName();
+
+ AIDL_FATAL_IF(decl.GetFields().empty(), decl) << "Union '" << name << "' is empty.";
+ const auto& first_field = decl.GetFields()[0];
+ const auto& first_name = first_field->GetName();
+ const auto& first_value = GetInitializer(typenames, *first_field);
+
// clang-format off
auto helper_methods = vector<string>{
// type classification
"template<typename _Tp>\n"
"static constexpr bool not_self = !std::is_same_v<std::remove_cv_t<std::remove_reference_t<_Tp>>, " + name + ">;\n\n",
- // ctors
- name + "() = default;\n",
+ // default ctor inits with the first member's default value
+ name + "() : _value(std::in_place_index<" + first_name + ">, " + first_value + ") { }\n",
+
+ // other ctors with default implementation
name + "(const " + name + "& other) = default;\n",
name + "(" + name + "&& other) = default;\n",
name + "& operator=(const " + name + "&) = default;\n",
name + "& operator=(" + name + "&&) = default;\n\n",
- // conversion from value
+ // conversion ctor from value
"template <typename T, std::enable_if_t<not_self<T>, int> = 0>\n"
"constexpr " + name + "(T&& arg)\n"
" : _value(std::forward<T>(arg)) {}\n",
- // to support in-place construction using in_place_index/in_place_type
+ // ctor to support in-place construction using in_place_index/in_place_type
"template <typename... T>\n"
"constexpr explicit " + name + "(T&&... args)\n"
" : _value(std::forward<T>(args)...) {}\n",
diff --git a/generate_java.cpp b/generate_java.cpp
index 17f6283..58c1870 100644
--- a/generate_java.cpp
+++ b/generate_java.cpp
@@ -482,11 +482,17 @@
out << "private Object " + value_name + ";\n";
out << "\n";
+ AIDL_FATAL_IF(decl->GetFields().empty(), *decl) << "Union '" << clazz << "' is empty.";
const auto& first_field = decl->GetFields()[0];
- // ctor()
+ const auto& first_type = JavaSignatureOf(first_field->GetType(), typenames);
+ const auto& first_value = first_field->ValueString(ConstantValueDecorator);
+
+ // default ctor() inits with first member's default value
out << "public " + clazz + "() {\n";
- out << " this(" + first_field->GetName() + ", " +
- first_field->ValueString(ConstantValueDecorator) + ");\n";
+ out.Indent();
+ out << first_type + " value = " << (first_value.empty() ? "null" : first_value) << ";\n";
+ out << "_set(" + first_field->GetName() + ", value);\n";
+ out.Dedent();
out << "}\n";
// ctor(Parcel)
out << "private " + clazz + "(android.os.Parcel _aidl_parcel) {\n";
diff --git a/tests/aidl_test_client_parcelables.cpp b/tests/aidl_test_client_parcelables.cpp
index 95768cb..f4ab2f6 100644
--- a/tests/aidl_test_client_parcelables.cpp
+++ b/tests/aidl_test_client_parcelables.cpp
@@ -152,6 +152,9 @@
}
TEST_F(AidlTest, UnionUsage) {
+ // default ctor inits with first member's default value
+ EXPECT_EQ(Union::make<Union::ns>(), Union());
+
// make<tag>(...) to create a value for a tag.
Union one_two_three = Union::make<Union::ns>({1, 2, 3});
diff --git a/tests/android/aidl/tests/Union.aidl b/tests/android/aidl/tests/Union.aidl
index 60a4726..f42ef16 100644
--- a/tests/android/aidl/tests/Union.aidl
+++ b/tests/android/aidl/tests/Union.aidl
@@ -18,9 +18,9 @@
import android.aidl.tests.ByteEnum;
union Union {
+ int[] ns;
int n;
int m;
- int[] ns;
@utf8InCpp String s;
@nullable IBinder ibinder;
@utf8InCpp List<String> ss;
diff --git a/tests/java/src/android/aidl/tests/UnionTests.java b/tests/java/src/android/aidl/tests/UnionTests.java
index 6b903b1..45f9686 100644
--- a/tests/java/src/android/aidl/tests/UnionTests.java
+++ b/tests/java/src/android/aidl/tests/UnionTests.java
@@ -17,6 +17,7 @@
package android.aidl.tests;
import static org.hamcrest.core.Is.is;
+import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import android.aidl.tests.Union;
@@ -31,9 +32,9 @@
public class UnionTests {
@Test
public void defaultConstructorInitsWithFirstField() {
- Union u = new Union();
- assertThat(u.getTag(), is(Union.n));
- assertThat(u.getN(), is(0));
+ Union u = new Union(); // `int[] ns`
+ assertThat(u.getTag(), is(Union.ns));
+ assertNull(u.getNs());
}
@Test