Change the ownership of AidlDefinedType objects
Previously, AidlDefinedType objects were directly owned by the
AidlTypenames object, although an AidlDefinedType is conceptually a part
of the AidlDocument object wheere the AidlDefinedType is found. At the
time, AidlDocument merely have references to AidlDefinedTypes that it
(conceptually) owns.
This change fixes the wrong ownership relationship. Now, the ownership
hierarchy is: AIdlTypenames -> AidlDocument -> AidlDefinedType. The
ownership is enforced by keeping AidlDocument and AidlDefinedType using
std::unique_ptr, and making them non-copyable and non-movable.
This change also alters the way AidlDefinedType objects are returned
from the load_and_validate_aidl function. Previously, we simply
returned the references (pointers) that were stored in the AidlDocument
via an out param. Now, the client of the function can get the objects
via typenames.MainDocument().DefinedTypes().
Bug: 160367901
Test: aidl_unittests
Change-Id: If3fcd2acaccce3a484e6329f8fa68bb959b743be
diff --git a/aidl.cpp b/aidl.cpp
index dc3dc3e..944032f 100644
--- a/aidl.cpp
+++ b/aidl.cpp
@@ -411,7 +411,6 @@
AidlError load_and_validate_aidl(const std::string& input_file_name, const Options& options,
const IoDelegate& io_delegate, AidlTypenames* typenames,
- vector<AidlDefinedType*>* defined_types,
vector<string>* imported_files) {
AidlError err = AidlError::OK;
@@ -425,7 +424,7 @@
return AidlError::PARSE_ERROR;
}
int num_interfaces_or_structured_parcelables = 0;
- for (AidlDefinedType* type : main_parser->Document().DefinedTypes()) {
+ for (const auto& type : main_parser->ParsedDocument().DefinedTypes()) {
if (type->AsInterface() != nullptr || type->AsStructuredParcelable() != nullptr) {
num_interfaces_or_structured_parcelables++;
if (num_interfaces_or_structured_parcelables > 1) {
@@ -451,7 +450,7 @@
options.InputFiles()};
vector<string> type_from_import_statements;
- for (const auto& import : main_parser->Document().Imports()) {
+ for (const auto& import : main_parser->ParsedDocument().Imports()) {
if (!AidlTypenames::IsBuiltinTypename(import->GetNeededClass())) {
type_from_import_statements.emplace_back(import->GetNeededClass());
}
@@ -568,9 +567,9 @@
// serious failures.
bool contains_unstructured_parcelable = false;
- const auto& types = main_parser->Document().DefinedTypes();
+ const auto& types = main_parser->ParsedDocument().DefinedTypes();
const int num_defined_types = types.size();
- for (const auto defined_type : types) {
+ for (const auto& defined_type : types) {
CHECK(defined_type != nullptr);
// Language specific validation
@@ -738,10 +737,6 @@
return err;
}
- if (defined_types != nullptr) {
- *defined_types = main_parser->Document().DefinedTypes();
- }
-
if (imported_files != nullptr) {
*imported_files = import_paths;
}
@@ -761,17 +756,16 @@
for (const string& input_file : options.InputFiles()) {
AidlTypenames typenames;
- vector<AidlDefinedType*> defined_types;
vector<string> imported_files;
- AidlError aidl_err = internals::load_and_validate_aidl(
- input_file, options, io_delegate, &typenames, &defined_types, &imported_files);
+ AidlError aidl_err = internals::load_and_validate_aidl(input_file, options, io_delegate,
+ &typenames, &imported_files);
bool allowError = aidl_err == AidlError::FOUND_PARCELABLE && !options.FailOnParcelable();
if (aidl_err != AidlError::OK && !allowError) {
return 1;
}
- for (const auto defined_type : defined_types) {
+ for (const auto& defined_type : typenames.MainDocument().DefinedTypes()) {
CHECK(defined_type != nullptr);
string output_file_name = options.OutputFile();
@@ -800,8 +794,8 @@
// Legacy behavior. For parcelable declarations in Java, don't generate output file.
success = true;
} else {
- success =
- java::generate_java(output_file_name, defined_type, typenames, io_delegate, options);
+ success = java::generate_java(output_file_name, defined_type.get(), typenames,
+ io_delegate, options);
}
} else {
LOG(FATAL) << "Should not reach here" << endl;
@@ -819,16 +813,15 @@
android::aidl::mappings::SignatureMap all_mappings;
for (const string& input_file : options.InputFiles()) {
AidlTypenames typenames;
- vector<AidlDefinedType*> defined_types;
vector<string> imported_files;
- AidlError aidl_err = internals::load_and_validate_aidl(
- input_file, options, io_delegate, &typenames, &defined_types, &imported_files);
+ AidlError aidl_err = internals::load_and_validate_aidl(input_file, options, io_delegate,
+ &typenames, &imported_files);
if (aidl_err != AidlError::OK) {
return false;
}
- for (const auto defined_type : defined_types) {
- auto mappings = mappings::generate_mappings(defined_type, typenames);
+ for (const auto& defined_type : typenames.MainDocument().DefinedTypes()) {
+ auto mappings = mappings::generate_mappings(defined_type.get(), typenames);
all_mappings.insert(mappings.begin(), mappings.end());
}
}
@@ -849,7 +842,7 @@
std::unique_ptr<Parser> p = Parser::Parse(file, io_delegate, typenames);
if (p == nullptr) return false;
- for (const auto& defined_type : p->Document().DefinedTypes()) {
+ for (const auto& defined_type : p->ParsedDocument().DefinedTypes()) {
if (!writer->Write("%s %s;\n", defined_type->GetPreprocessDeclarationName().c_str(),
defined_type->GetCanonicalName().c_str())) {
return false;
@@ -870,10 +863,9 @@
bool dump_api(const Options& options, const IoDelegate& io_delegate) {
for (const auto& file : options.InputFiles()) {
AidlTypenames typenames;
- vector<AidlDefinedType*> defined_types;
- if (internals::load_and_validate_aidl(file, options, io_delegate, &typenames, &defined_types,
- nullptr) == AidlError::OK) {
- for (const auto type : defined_types) {
+ if (internals::load_and_validate_aidl(file, options, io_delegate, &typenames, nullptr) ==
+ AidlError::OK) {
+ for (const auto& type : typenames.MainDocument().DefinedTypes()) {
unique_ptr<CodeWriter> writer =
io_delegate.GetCodeWriter(GetApiDumpPathFor(*type, options));
if (!type->GetPackage().empty()) {
diff --git a/aidl.h b/aidl.h
index d1e2eb9..21598d5 100644
--- a/aidl.h
+++ b/aidl.h
@@ -80,7 +80,6 @@
AidlError load_and_validate_aidl(const std::string& input_file_name, const Options& options,
const IoDelegate& io_delegate, AidlTypenames* typenames,
- vector<AidlDefinedType*>* defined_types,
vector<string>* imported_files);
bool parse_preprocessed_file(const IoDelegate& io_delegate, const std::string& filename,
diff --git a/aidl_checkapi.cpp b/aidl_checkapi.cpp
index 4897cbb..55a1137 100644
--- a/aidl_checkapi.cpp
+++ b/aidl_checkapi.cpp
@@ -308,13 +308,16 @@
for (const auto& file : old_files) {
if (!android::base::EndsWith(file, ".aidl")) continue;
- vector<AidlDefinedType*> types;
- if (internals::load_and_validate_aidl(file, options, io_delegate, &old_tns, &types,
+ if (internals::load_and_validate_aidl(file, options, io_delegate, &old_tns,
nullptr /* imported_files */) != AidlError::OK) {
AIDL_ERROR(file) << "Failed to read.";
return false;
}
- old_types.insert(old_types.end(), types.begin(), types.end());
+ }
+ for (const auto& d : old_tns.AllDocuments()) {
+ for (const auto& t : d->DefinedTypes()) {
+ old_types.push_back(t.get());
+ }
}
AidlTypenames new_tns;
@@ -328,13 +331,16 @@
for (const auto& file : new_files) {
if (!android::base::EndsWith(file, ".aidl")) continue;
- vector<AidlDefinedType*> types;
- if (internals::load_and_validate_aidl(file, options, io_delegate, &new_tns, &types,
+ if (internals::load_and_validate_aidl(file, options, io_delegate, &new_tns,
nullptr /* imported_files */) != AidlError::OK) {
AIDL_ERROR(file) << "Failed to read.";
return false;
}
- new_types.insert(new_types.end(), types.begin(), types.end());
+ }
+ for (const auto& d : new_tns.AllDocuments()) {
+ for (const auto& t : d->DefinedTypes()) {
+ new_types.push_back(t.get());
+ }
}
map<string, AidlDefinedType*> new_map;
diff --git a/aidl_language.h b/aidl_language.h
index 41c915a..86bd928 100644
--- a/aidl_language.h
+++ b/aidl_language.h
@@ -739,6 +739,8 @@
const std::vector<std::string> split_package_;
DISALLOW_COPY_AND_ASSIGN(AidlDefinedType);
+ AidlDefinedType(AidlDefinedType&&) = delete;
+ AidlDefinedType& operator=(AidlDefinedType&&) = delete;
};
class AidlParcelable : public AidlDefinedType, public AidlParameterizable<std::string> {
@@ -899,12 +901,21 @@
class AidlDocument : public AidlNode {
public:
AidlDocument(const AidlLocation& location, std::vector<std::unique_ptr<AidlImport>>& imports,
- std::vector<AidlDefinedType*>& defined_types)
- : AidlNode(location), imports_(std::move(imports)), defined_types_(defined_types) {}
+ std::vector<std::unique_ptr<AidlDefinedType>>&& defined_types)
+ : AidlNode(location),
+ imports_(std::move(imports)),
+ defined_types_(std::move(defined_types)) {}
const std::vector<std::unique_ptr<AidlImport>>& Imports() const { return imports_; }
- const std::vector<AidlDefinedType*>& DefinedTypes() const { return defined_types_; }
+ const std::vector<std::unique_ptr<AidlDefinedType>>& DefinedTypes() const {
+ return defined_types_;
+ }
+ AidlDocument(const AidlDocument&) = delete;
+ AidlDocument(AidlDocument&&) = delete;
+ AidlDocument& operator=(const AidlDocument&) = delete;
+ AidlDocument& operator=(AidlDocument&&) = delete;
+ ~AidlDocument() = default;
private:
const std::vector<std::unique_ptr<AidlImport>> imports_;
- const std::vector<AidlDefinedType*> defined_types_;
+ const std::vector<std::unique_ptr<AidlDefinedType>> defined_types_;
};
diff --git a/aidl_language_y.yy b/aidl_language_y.yy
index f0b174a..c1bf77f 100644
--- a/aidl_language_y.yy
+++ b/aidl_language_y.yy
@@ -96,21 +96,11 @@
std::vector<std::string>* type_params;
std::vector<std::unique_ptr<AidlImport>>* imports;
AidlImport* import;
- std::vector<AidlDefinedType*>* declarations;
+ std::vector<std::unique_ptr<AidlDefinedType>>* declarations;
}
%destructor { } <character>
%destructor { } <direction>
-// TODO(b/160367901) remove this.
-%destructor {
- // decl is std::vector<AidlDefinedType*>. When deleting it,
- // we should first delete AidlDefinedType objects in it.
- // Otherwise, there would be memory leaks.
- for (auto* t: *($$)) {
- delete(t);
- }
- delete ($$);
-} decls
%destructor { delete ($$); } <*>
%token<token> ANNOTATION "annotation"
@@ -196,7 +186,7 @@
document
: package imports decls
- { ps->SetDocument(std::make_unique<AidlDocument>(loc(@1), *$2, *$3));
+ { ps->SetDocument(std::make_unique<AidlDocument>(loc(@1), *$2, std::move(*$3)));
delete $2;
delete $3;
}
@@ -254,7 +244,7 @@
decls
: decl
- { $$ = new std::vector<AidlDefinedType*>();
+ { $$ = new std::vector<std::unique_ptr<AidlDefinedType>>();
if ($1 != nullptr) {
$$->emplace_back($1);
}
diff --git a/aidl_typenames.cpp b/aidl_typenames.cpp
index c5674fb..08a4aff 100644
--- a/aidl_typenames.cpp
+++ b/aidl_typenames.cpp
@@ -115,18 +115,27 @@
return in_ignore_import || defined_type_not_from_preprocessed;
}
-bool AidlTypenames::AddDefinedType(unique_ptr<AidlDefinedType> type) {
- const string name = type->GetCanonicalName();
- if (defined_types_.find(name) != defined_types_.end()) {
- return false;
+bool AidlTypenames::AddDocument(std::unique_ptr<AidlDocument> doc) {
+ for (const auto& type : doc->DefinedTypes()) {
+ if (defined_types_.find(type->GetCanonicalName()) != defined_types_.end()) {
+ return false;
+ }
+ if (!HasValidNameComponents(*type)) {
+ return false;
+ }
}
- if (!HasValidNameComponents(*type)) {
- return false;
+ documents_.push_back(std::move(doc));
+ for (const auto& type : documents_.back()->DefinedTypes()) {
+ defined_types_.emplace(type->GetCanonicalName(), type.get());
}
- defined_types_.emplace(name, std::move(type));
return true;
}
+const AidlDocument& AidlTypenames::MainDocument() const {
+ CHECK(documents_.size() != 0) << "Main document doesn't exist";
+ return *(documents_[0]);
+}
+
bool AidlTypenames::AddPreprocessedType(unique_ptr<AidlDefinedType> type) {
const string name = type->GetCanonicalName();
if (preprocessed_types_.find(name) != preprocessed_types_.end()) {
@@ -157,7 +166,7 @@
// Do the exact match first.
auto found_def = defined_types_.find(type_name);
if (found_def != defined_types_.end()) {
- return DefinedImplResult(found_def->second.get(), false);
+ return DefinedImplResult(found_def->second, false);
}
auto found_prep = preprocessed_types_.find(type_name);
@@ -169,7 +178,7 @@
// types from the preprocessed file.
for (auto it = defined_types_.begin(); it != defined_types_.end(); it++) {
if (it->second->GetName() == type_name) {
- return DefinedImplResult(it->second.get(), false);
+ return DefinedImplResult(it->second, false);
}
}
@@ -240,6 +249,7 @@
void AidlTypenames::Reset() {
defined_types_.clear();
preprocessed_types_.clear();
+ documents_.clear();
}
} // namespace aidl
diff --git a/aidl_typenames.h b/aidl_typenames.h
index 3b01ada..18d9e77 100644
--- a/aidl_typenames.h
+++ b/aidl_typenames.h
@@ -34,6 +34,7 @@
class AidlEnumDeclaration;
class AidlInterface;
class AidlTypeSpecifier;
+class AidlDocument;
namespace android {
namespace aidl {
@@ -53,7 +54,9 @@
public:
AidlTypenames() = default;
void Reset();
- bool AddDefinedType(unique_ptr<AidlDefinedType> type);
+ bool AddDocument(std::unique_ptr<AidlDocument> doc);
+ const std::vector<std::unique_ptr<AidlDocument>>& AllDocuments() const { return documents_; }
+ const AidlDocument& MainDocument() const;
bool AddPreprocessedType(unique_ptr<AidlDefinedType> type);
static bool IsBuiltinTypename(const string& type_name);
static bool IsPrimitiveTypename(const string& type_name);
@@ -83,8 +86,9 @@
const bool from_preprocessed;
};
DefinedImplResult TryGetDefinedTypeImpl(const string& type_name) const;
- map<string, unique_ptr<AidlDefinedType>> defined_types_;
+ map<string, const AidlDefinedType*> defined_types_;
map<string, unique_ptr<AidlDefinedType>> preprocessed_types_;
+ std::vector<std::unique_ptr<AidlDocument>> documents_;
};
} // namespace aidl
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp
index 291fdc8..1388594 100644
--- a/aidl_unittest.cpp
+++ b/aidl_unittest.cpp
@@ -169,11 +169,10 @@
}
args.emplace_back(path);
Options options = Options::From(args);
- vector<AidlDefinedType*> defined_types;
vector<string> imported_files;
ImportResolver import_resolver{io_delegate_, path, import_paths_, {}};
AidlError actual_error = ::android::aidl::internals::load_and_validate_aidl(
- path, options, io_delegate_, &typenames_, &defined_types, &imported_files);
+ path, options, io_delegate_, &typenames_, &imported_files);
if (error != nullptr) {
*error = actual_error;
@@ -183,9 +182,10 @@
return nullptr;
}
+ const auto& defined_types = typenames_.MainDocument().DefinedTypes();
EXPECT_EQ(1ul, defined_types.size());
- return defined_types.front();
+ return defined_types.front().get();
}
Options::Language GetLanguage() { return GetParam(); }
diff --git a/generate_cpp_unittest.cpp b/generate_cpp_unittest.cpp
index b7df190..118f545 100644
--- a/generate_cpp_unittest.cpp
+++ b/generate_cpp_unittest.cpp
@@ -1521,40 +1521,38 @@
AidlInterface* ParseSingleInterface() {
io_delegate_.SetFileContents(options_.InputFiles().at(0), file_contents_);
- vector<AidlDefinedType*> defined_types;
vector<string> imported_files;
ImportResolver import_resolver{io_delegate_, options_.InputFiles().at(0), {"."}, {}};
AidlError err = ::android::aidl::internals::load_and_validate_aidl(
- options_.InputFiles().front(), options_, io_delegate_, &typenames_, &defined_types,
- &imported_files);
+ options_.InputFiles().front(), options_, io_delegate_, &typenames_, &imported_files);
if (err != AidlError::OK) {
return nullptr;
}
+ const auto& defined_types = typenames_.MainDocument().DefinedTypes();
EXPECT_EQ(1ul, defined_types.size());
- EXPECT_NE(nullptr, defined_types.front()->AsInterface());
+ EXPECT_NE(nullptr, defined_types.front().get()->AsInterface());
- return defined_types.front()->AsInterface();
+ return defined_types.front().get()->AsInterface();
}
AidlEnumDeclaration* ParseSingleEnumDeclaration() {
io_delegate_.SetFileContents(options_.InputFiles().at(0), file_contents_);
- vector<AidlDefinedType*> defined_types;
vector<string> imported_files;
AidlError err = ::android::aidl::internals::load_and_validate_aidl(
- options_.InputFiles().front(), options_, io_delegate_, &typenames_, &defined_types,
- &imported_files);
+ options_.InputFiles().front(), options_, io_delegate_, &typenames_, &imported_files);
if (err != AidlError::OK) {
return nullptr;
}
+ const auto& defined_types = typenames_.MainDocument().DefinedTypes();
EXPECT_EQ(1ul, defined_types.size());
- EXPECT_NE(nullptr, defined_types.front()->AsEnumDeclaration());
+ EXPECT_NE(nullptr, defined_types.front().get()->AsEnumDeclaration());
- return defined_types.front()->AsEnumDeclaration();
+ return defined_types.front().get()->AsEnumDeclaration();
}
void Compare(Document* doc, const char* expected) {
diff --git a/parser.cpp b/parser.cpp
index 7efa04c..1b3edcc 100644
--- a/parser.cpp
+++ b/parser.cpp
@@ -41,7 +41,9 @@
std::unique_ptr<Parser> parser(new Parser(filename, *raw_buffer, typenames));
- if (yy::parser(parser.get()).parse() != 0 || parser->HasError()) return nullptr;
+ if (yy::parser(parser.get()).parse() != 0 || parser->HasError()) {
+ return nullptr;
+ }
return parser;
}
diff --git a/parser.h b/parser.h
index 33a7d2a..826e80e 100644
--- a/parser.h
+++ b/parser.h
@@ -20,6 +20,7 @@
#include "aidl_typenames.h"
#include "io_delegate.h"
#include "options.h"
+#include <android-base/logging.h>
#include <memory>
#include <string>
@@ -42,7 +43,7 @@
AidlTypenames& typenames);
void AddError() { error_++; }
- bool HasError() { return error_ != 0; }
+ bool HasError() const { return error_ != 0; }
const std::string& FileName() const { return filename_; }
void* Scanner() const { return scanner_; }
@@ -58,19 +59,19 @@
bool Resolve();
void SetDocument(std::unique_ptr<AidlDocument>&& document) {
- // AidlDocument does not have the ownership to AidlDefinedTypes.
- // AidlTypenames has the ownership.
- // TODO(jiyong): fix this by making AidlTypenames to own
- // AidlDocuments and AidlDocument to own AidlDefinedTypes.
- for (auto* t : document->DefinedTypes()) {
- if (!typenames_.AddDefinedType(std::unique_ptr<AidlDefinedType>(t))) {
- AddError();
- }
+ // The parsed document is owned by typenames_. This parser object only has
+ // a reference to it.
+ document_ = document.get();
+ if (!typenames_.AddDocument(std::move(document))) {
+ document_ = nullptr;
+ AddError();
}
- document_ = std::move(document);
}
- const AidlDocument& Document() const { return *document_; }
+ const AidlDocument& ParsedDocument() const {
+ CHECK(!HasError());
+ return *document_;
+ }
private:
explicit Parser(const std::string& filename, std::string& raw_buffer,
@@ -85,5 +86,5 @@
int error_ = 0;
vector<AidlTypeSpecifier*> unresolved_typespecs_;
- std::unique_ptr<AidlDocument> document_;
+ const AidlDocument* document_;
};