Skip to content

Commit

Permalink
ICU-22261 Refactor MF2 attributes and options parsing code
Browse files Browse the repository at this point in the history 
Previously, there were separate overrides for the options and
attributes parsing methods in the parser that were used in different
context. (Options can appear in Operator and Markup, while attributes
can appear in Expression and Markup.)

This is a refactoring that eliminates this duplicated code.
To enable it, a builder is added for the internal OptionMap type.

Separately, this patch also explicitly deletes copy constructors
and copy assignment operators for all Builder classes; a bug in an
earlier version of this patch caused me to notice this hadn't been
done. Also explicitly deletes move constructors/assignment operators
with the exception of OptionMap::Builder (OptionMap is non-public,
so that shouldn't cause confusion).
  • Loading branch information
@catamorphism @echeran
catamorphism authored and echeran committed Apr 9, 2024
1 parent bae39ad commit 9e1c66d
Show file tree
Hide file tree
Showing 4 changed files with 208 additions and 310 deletions.
178 changes: 90 additions & 88 deletions icu4c/source/i18n/messageformat2_data_model.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ OptionMap::OptionMap(const UVector& opts, UErrorCode& status) {

len = opts.size();
Option* result = copyVectorToArray<Option>(opts, len);
if (result == nullptr) {
if (result == nullptr && len != 0) {
bogus = true;
status = U_MEMORY_ALLOCATION_ERROR;
return;
Expand Down Expand Up @@ -304,14 +304,64 @@ int32_t OptionMap::size() const {
return len;
}

Operator::Builder& Operator::Builder::setOptionMap(OptionMap&& m) {
optionMap = std::move(m);
delete options;
options = nullptr;
OptionMap::~OptionMap() {}

OptionMap OptionMap::Builder::build(UErrorCode& status) {
return OptionMap(*options, status);
}

OptionMap::Builder::Builder(UErrorCode& status) {
options = createStringUVector(status);
}

OptionMap::Builder::Builder(OptionMap::Builder&& other) {
checkDuplicates = other.checkDuplicates;
options = other.options;
other.options = nullptr;
}

OptionMap::Builder& OptionMap::Builder::operator=(OptionMap::Builder other) noexcept {
swap(*this, other);
return *this;
}

OptionMap::~OptionMap() {}
/* static */ OptionMap::Builder OptionMap::Builder::attributes(UErrorCode& status) {
Builder b(status);
// The same code is re-used for representing attributes and options.
// Duplicate attributes are allowed, while duplicate options are disallowed.
b.checkDuplicates = false;
return b;
}

static UBool hasOptionNamed(const UVector& v, const UnicodeString& s) {
for (int32_t i = 0; i < v.size(); i++) {
const Option* opt = static_cast<Option*>(v[i]);
U_ASSERT(opt != nullptr);
if (opt->getName() == s) {
return true;
}
}
return false;
}

OptionMap::Builder& OptionMap::Builder::add(Option&& opt, UErrorCode& status) {
THIS_ON_ERROR(status);

// If the option name is already in the map, emit a data model error
if (checkDuplicates && hasOptionNamed(*options, opt.getName())) {
status = U_MF_DUPLICATE_OPTION_NAME_ERROR;
} else {
Option* newOption = create<Option>(std::move(opt), status);
options->adoptElement(newOption, status);
}
return *this;
}

OptionMap::Builder::~Builder() {
if (options != nullptr) {
delete options;
}
}

const Reserved& Operator::asReserved() const {
U_ASSERT(isReserved());
Expand All @@ -332,13 +382,12 @@ Option& Option::operator=(Option other) noexcept {

Option::~Option() {}

Operator::Builder::Builder(UErrorCode& status) {
options = createStringUVector(status);
}
Operator::Builder::Builder(UErrorCode& status) : options(OptionMap::Builder(status)) {}

Operator::Builder& Operator::Builder::setReserved(Reserved&& reserved) {
isReservedSequence = true;
hasFunctionName = false;
hasOptions = false;
asReserved = std::move(reserved);
return *this;
}
Expand All @@ -355,31 +404,12 @@ const FunctionName& Operator::getFunctionName() const {
return std::get_if<Callable>(&contents)->getName();
}

static UBool hasOptionNamed(const UVector& v, const UnicodeString& s) {
for (int32_t i = 0; i < v.size(); i++) {
const Option* opt = static_cast<Option*>(v[i]);
U_ASSERT(opt != nullptr);
if (opt->getName() == s) {
return true;
}
}
return false;
}

Operator::Builder& Operator::Builder::addOption(const UnicodeString &key, Operand&& value, UErrorCode& errorCode) noexcept {
THIS_ON_ERROR(errorCode);

isReservedSequence = false;
hasOptions = true;
U_ASSERT(options != nullptr);
// If the option name is already in the map, emit a data model error
if (hasOptionNamed(*options, key)) {
errorCode = U_MF_DUPLICATE_OPTION_NAME_ERROR;
} else {
Option* newOption = create<Option>(Option(key, std::move(value)), errorCode);
THIS_ON_ERROR(errorCode);
options->adoptElement(newOption, errorCode);
}
options.add(Option(key, std::move(value)), errorCode);
return *this;
}

Expand All @@ -404,11 +434,7 @@ Operator Operator::Builder::build(UErrorCode& errorCode) {
errorCode = U_INVALID_STATE_ERROR;
return result;
}
if (options != nullptr) {
// Initialize options from what was set with setOptions()
optionMap = OptionMap(*options, errorCode);
}
result = Operator(functionName, optionMap);
result = Operator(functionName, options.build(errorCode));
}
return result;
}
Expand All @@ -425,11 +451,7 @@ Operator::Operator(const FunctionName& f, const UVector& optsVector, UErrorCode&

Operator::Operator(const FunctionName& f, const OptionMap& opts) : contents(Callable(f, opts)) {}

Operator::Builder::~Builder() {
if (options != nullptr) {
delete options;
}
}
Operator::Builder::~Builder() {}

Operator::~Operator() {}

Expand All @@ -444,26 +466,24 @@ Callable::~Callable() {}

// ------------ Markup

Markup::Builder::Builder(UErrorCode& status) {
options = createUVector(status);
attributes = createUVector(status);
}
Markup::Builder::Builder(UErrorCode& status)
: options(OptionMap::Builder(status)), attributes(OptionMap::Builder::attributes(status)) {}

Markup::Builder& Markup::Builder::setAttributeMap(OptionMap&& m) {
attributeMap = std::move(m);
delete attributes;
attributes = nullptr;
return *this;
}
Markup::Markup(UMarkupType ty, UnicodeString n, OptionMap&& o, OptionMap&& a)
: type(ty), name(n), options(std::move(o)), attributes(std::move(a)) {}

Markup::Builder& Markup::Builder::setOptionMap(OptionMap&& m) {
optionMap = std::move(m);
delete options;
options = nullptr;
Markup::Builder& Markup::Builder::addOption(const UnicodeString &key,
Operand&& value,
UErrorCode& errorCode) {
options.add(Option(key, std::move(value)), errorCode);
return *this;
}

Markup::Markup(UMarkupType ty, UnicodeString n, OptionMap&& o, OptionMap&& a) : type(ty), name(n), options(std::move(o)), attributes(std::move(a)) {
Markup::Builder& Markup::Builder::addAttribute(const UnicodeString &key,
Operand&& value,
UErrorCode& errorCode) {
attributes.add(Option(key, std::move(value)), errorCode);
return *this;
}

Markup Markup::Builder::build(UErrorCode& errorCode) {
Expand All @@ -479,34 +499,22 @@ Markup Markup::Builder::build(UErrorCode& errorCode) {
// setName() must be called before calling build()
errorCode = U_INVALID_STATE_ERROR;
} else {
if (options != nullptr) {
// Initialize options from what was done with setOptions
optionMap = OptionMap(*options, errorCode);
}
if (attributes != nullptr) {
attributeMap = OptionMap(*attributes, errorCode);
}
result = Markup(type, name, std::move(optionMap), std::move(attributeMap));
result = Markup(type,
name,
options.build(errorCode),
attributes.build(errorCode));
}
return result;
}

Markup::Builder::~Builder() {
if (options != nullptr) {
delete options;
}
if (attributes != nullptr) {
delete attributes;
}
}
Markup::Builder::~Builder() {}

Markup::~Markup() {}

// ------------ Expression

Expression::Builder::Builder(UErrorCode& status) {
attributes = createUVector(status);
}
Expression::Builder::Builder(UErrorCode& status)
: attributes(OptionMap::Builder::attributes(status)) {}

UBool Expression::isStandaloneAnnotation() const {
return rand.isNull();
Expand Down Expand Up @@ -549,10 +557,10 @@ Expression::Builder& Expression::Builder::setOperator(Operator&& rAtor) {
return *this;
}

Expression::Builder& Expression::Builder::setAttributeMap(OptionMap&& m) {
attributeMap = std::move(m);
delete attributes;
attributes = nullptr;
Expression::Builder& Expression::Builder::addAttribute(const UnicodeString& k,
Operand&& v,
UErrorCode& status) {
attributes.add(Option(k, std::move(v)), status);
return *this;
}

Expand All @@ -568,16 +576,14 @@ Expression Expression::Builder::build(UErrorCode& errorCode) {
return result;
}

if (attributes != nullptr) {
attributeMap = OptionMap(*attributes, errorCode);
}
OptionMap attributeMap = attributes.build(errorCode);
if (hasOperand && hasOperator) {
result = Expression(rator, rand, attributeMap);
result = Expression(rator, rand, std::move(attributeMap));
} else if (hasOperand && !hasOperator) {
result = Expression(rand, attributeMap);
result = Expression(rand, std::move(attributeMap));
} else {
// rator is valid, rand is not valid
result = Expression(rator, attributeMap);
result = Expression(rator, std::move(attributeMap));
}
return result;
}
Expand All @@ -591,11 +597,7 @@ Expression& Expression::operator=(Expression other) noexcept {
return *this;
}

Expression::Builder::~Builder() {
if (attributes != nullptr) {
delete attributes;
}
}
Expression::Builder::~Builder() {}

Expression::~Expression() {}

Expand Down
Loading

0 comments on commit 9e1c66d

Please sign in to comment.