Skip to content

Commit

Permalink
Eliminate Impl pointer (facebook#803) (facebook#803)
Browse files Browse the repository at this point in the history 
Summary:
Original Author: jpporto@fb.com
Original Git: 163f705

Pull Request resolved: facebook#803

Removes the pImpl indirection in Intl implementations. This is possible due to
the fact that there's only ever one Intl implementation compiled in hermes, so we
can safely down cast from the base Intl classes to the Native ones.

Original Reviewed By: neildhar

Original Revision: D39115523

Reviewed By: neildhar

Differential Revision: D41240795

fbshipit-source-id: c64a048fee27f67362fc5f9700fbf6ba0207d046
  • Loading branch information
@avp @neildhar
avp authored and neildhar committed Nov 18, 2022
1 parent 7dd4ced commit 05f002d
Show file tree
Hide file tree
Showing 5 changed files with 656 additions and 443 deletions.
36 changes: 15 additions & 21 deletions include/hermes/Platform/Intl/PlatformIntl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,11 @@ enum class NativeType {
};

class Collator : public vm::DecoratedObject::Decoration {
public:
protected:
Collator();
~Collator();

public:
~Collator() override;

static constexpr NativeType getNativeType() {
return NativeType::Collator;
Expand All @@ -113,23 +115,21 @@ class Collator : public vm::DecoratedObject::Decoration {
const std::vector<std::u16string> &locales,
const Options &options) noexcept;

vm::ExecutionStatus initialize(
static vm::CallResult<std::unique_ptr<Collator>> create(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept;
Options resolvedOptions() noexcept;

double compare(const std::u16string &x, const std::u16string &y) noexcept;

private:
struct Impl;
std::unique_ptr<Impl> impl_;
};

class DateTimeFormat : public vm::DecoratedObject::Decoration {
public:
protected:
DateTimeFormat();
~DateTimeFormat();

public:
~DateTimeFormat() override;

static constexpr NativeType getNativeType() {
return NativeType::DateTimeFormat;
Expand All @@ -140,24 +140,22 @@ class DateTimeFormat : public vm::DecoratedObject::Decoration {
const std::vector<std::u16string> &locales,
const Options &options) noexcept;

vm::ExecutionStatus initialize(
static vm::CallResult<std::unique_ptr<DateTimeFormat>> create(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept;
Options resolvedOptions() noexcept;

std::u16string format(double jsTimeValue) noexcept;
std::vector<Part> formatToParts(double jsTimeValue) noexcept;

private:
struct Impl;
std::unique_ptr<Impl> impl_;
};

class NumberFormat : public vm::DecoratedObject::Decoration {
public:
protected:
NumberFormat();
~NumberFormat();

public:
~NumberFormat() override;

static constexpr NativeType getNativeType() {
return NativeType::NumberFormat;
Expand All @@ -168,18 +166,14 @@ class NumberFormat : public vm::DecoratedObject::Decoration {
const std::vector<std::u16string> &locales,
const Options &options) noexcept;

vm::ExecutionStatus initialize(
static vm::CallResult<std::unique_ptr<NumberFormat>> create(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept;
Options resolvedOptions() noexcept;

std::u16string format(double jsTimeValue) noexcept;
std::vector<Part> formatToParts(double jsTimeValue) noexcept;

private:
struct Impl;
std::unique_ptr<Impl> impl_;
};

} // namespace platform_intl
Expand Down
183 changes: 141 additions & 42 deletions lib/Platform/Intl/PlatformIntlAndroid.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,17 +316,34 @@ class JCollator : public jni::JavaClass<JCollator> {
}
};

} // namespace
class CollatorAndroid : public Collator {
public:
CollatorAndroid() = default;
~CollatorAndroid() {
jni::ThreadScope::WithClassLoader([&] { jCollator_.reset(); });
}

vm::ExecutionStatus initialize(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept;

Options resolvedOptions() noexcept {
return optionsFromJava(jCollator_->resolvedOptions());
}

double compare(const std::u16string &x, const std::u16string &y) noexcept {
return jCollator_->compare(stringToJava(x), stringToJava(y));
}

struct Collator::Impl {
private:
jni::global_ref<JCollator> jCollator_;
};
} // namespace

Collator::Collator() : impl_(std::make_unique<Impl>()) {}
Collator::Collator() = default;

Collator::~Collator() {
jni::ThreadScope::WithClassLoader([&] { impl_.reset(); });
}
Collator::~Collator() = default;

vm::CallResult<std::vector<std::u16string>> Collator::supportedLocalesOf(
vm::Runtime &runtime,
Expand All @@ -342,12 +359,12 @@ vm::CallResult<std::vector<std::u16string>> Collator::supportedLocalesOf(
}
}

vm::ExecutionStatus Collator::initialize(
vm::ExecutionStatus CollatorAndroid::initialize(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept {
try {
impl_->jCollator_ = jni::make_global(
jCollator_ = jni::make_global(
JCollator::create(localesToJava(locales), optionsToJava(options)));
} catch (const std::exception &ex) {
return runtime.raiseRangeError(ex.what());
Expand All @@ -356,14 +373,27 @@ vm::ExecutionStatus Collator::initialize(
return vm::ExecutionStatus::RETURNED;
}

vm::CallResult<std::unique_ptr<Collator>> Collator::create(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept {
auto instance = std::make_unique<CollatorAndroid>();
if (LLVM_UNLIKELY(
instance->initialize(runtime, locales, options) ==
vm::ExecutionStatus::EXCEPTION)) {
return vm::ExecutionStatus::EXCEPTION;
}
return instance;
}

Options Collator::resolvedOptions() noexcept {
return optionsFromJava(impl_->jCollator_->resolvedOptions());
return static_cast<CollatorAndroid *>(this)->resolvedOptions();
}

double Collator::compare(
const std::u16string &x,
const std::u16string &y) noexcept {
return impl_->jCollator_->compare(stringToJava(x), stringToJava(y));
return static_cast<CollatorAndroid *>(this)->compare(x, y);
}

namespace {
Expand Down Expand Up @@ -411,17 +441,43 @@ class JDateTimeFormat : public jni::JavaClass<JDateTimeFormat> {
}
};

} // namespace
class DateTimeFormatAndroid : public DateTimeFormat {
public:
DateTimeFormatAndroid() = default;
~DateTimeFormatAndroid() {
jni::ThreadScope::WithClassLoader([&] { jDateTimeFormat_.reset(); });
}

vm::ExecutionStatus initialize(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept;

struct DateTimeFormat::Impl {
Options resolvedOptions() noexcept {
return optionsFromJava(jDateTimeFormat_->resolvedOptions());
}

std::u16string format(double jsTimeValue) noexcept {
// I don't believe the Java logic can throw an exception (the JS
// method can, but the errors all come from the Intl.cpp logic). If
// I am incorrect, this will need to add a try/catch and take a
// runtime to call raiseRangeError on it. This is true for all the
// format methods.
return stringFromJava(jDateTimeFormat_->format(jsTimeValue));
}

std::vector<Part> formatToParts(double jsTimeValue) noexcept {
return partsFromJava(jDateTimeFormat_->formatToParts(jsTimeValue));
}

private:
jni::global_ref<JDateTimeFormat> jDateTimeFormat_;
};
} // namespace

DateTimeFormat::DateTimeFormat() : impl_(std::make_unique<Impl>()) {}
DateTimeFormat::DateTimeFormat() = default;

DateTimeFormat::~DateTimeFormat() {
jni::ThreadScope::WithClassLoader([&] { impl_.reset(); });
}
DateTimeFormat::~DateTimeFormat() = default;

vm::CallResult<std::vector<std::u16string>> DateTimeFormat::supportedLocalesOf(
vm::Runtime &runtime,
Expand All @@ -437,12 +493,12 @@ vm::CallResult<std::vector<std::u16string>> DateTimeFormat::supportedLocalesOf(
}
}

vm::ExecutionStatus DateTimeFormat::initialize(
vm::ExecutionStatus DateTimeFormatAndroid::initialize(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept {
try {
impl_->jDateTimeFormat_ = jni::make_global(JDateTimeFormat::create(
jDateTimeFormat_ = jni::make_global(JDateTimeFormat::create(
localesToJava(locales), optionsToJava(options)));
} catch (const std::exception &ex) {
return runtime.raiseRangeError(ex.what());
Expand All @@ -451,21 +507,29 @@ vm::ExecutionStatus DateTimeFormat::initialize(
return vm::ExecutionStatus::RETURNED;
}

vm::CallResult<std::unique_ptr<DateTimeFormat>> DateTimeFormat::create(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept {
auto instance = std::make_unique<DateTimeFormatAndroid>();
if (LLVM_UNLIKELY(
instance->initialize(runtime, locales, options) ==
vm::ExecutionStatus::EXCEPTION)) {
return vm::ExecutionStatus::EXCEPTION;
}
return instance;
}

Options DateTimeFormat::resolvedOptions() noexcept {
return optionsFromJava(impl_->jDateTimeFormat_->resolvedOptions());
return static_cast<DateTimeFormatAndroid *>(this)->resolvedOptions();
}

std::u16string DateTimeFormat::format(double jsTimeValue) noexcept {
// I don't believe the Java logic can throw an exception (the JS
// method can, but the errors all come from the Intl.cpp logic). If
// I am incorrect, this will need to add a try/catch and take a
// runtime to call raiseRangeError on it. This is true for all the
// format methods.
return stringFromJava(impl_->jDateTimeFormat_->format(jsTimeValue));
return static_cast<DateTimeFormatAndroid *>(this)->format(jsTimeValue);
}

std::vector<Part> DateTimeFormat::formatToParts(double jsTimeValue) noexcept {
return partsFromJava(impl_->jDateTimeFormat_->formatToParts(jsTimeValue));
return static_cast<DateTimeFormatAndroid *>(this)->formatToParts(jsTimeValue);
}

namespace {
Expand Down Expand Up @@ -513,17 +577,44 @@ class JNumberFormat : public jni::JavaClass<JNumberFormat> {
}
};

} // namespace
class NumberFormatAndroid : public NumberFormat {
public:
NumberFormatAndroid() = default;

~NumberFormatAndroid() {
jni::ThreadScope::WithClassLoader([&] { jNumberFormat_.reset(); });
}

vm::ExecutionStatus initialize(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept;

Options resolvedOptions() noexcept {
return optionsFromJava(jNumberFormat_->resolvedOptions());
}

std::u16string format(double number) noexcept {
// I don't believe the Java logic can throw an exception (the JS
// method can, but the errors all come from the Intl.cpp logic). If
// I am incorrect, this will need to add a try/catch and take a
// runtime to call raiseRangeError on it. This is true for all the
// format methods.
return stringFromJava(jNumberFormat_->format(number));
}

struct NumberFormat::Impl {
std::vector<Part> formatToParts(double number) noexcept {
return partsFromJava(jNumberFormat_->formatToParts(number));
}

private:
jni::global_ref<JNumberFormat> jNumberFormat_;
};
} // namespace

NumberFormat::NumberFormat() : impl_(std::make_unique<Impl>()) {}
NumberFormat::NumberFormat() = default;

NumberFormat::~NumberFormat() {
jni::ThreadScope::WithClassLoader([&] { impl_.reset(); });
}
NumberFormat::~NumberFormat() = default;

vm::CallResult<std::vector<std::u16string>> NumberFormat::supportedLocalesOf(
vm::Runtime &runtime,
Expand All @@ -539,12 +630,12 @@ vm::CallResult<std::vector<std::u16string>> NumberFormat::supportedLocalesOf(
}
}

vm::ExecutionStatus NumberFormat::initialize(
vm::ExecutionStatus NumberFormatAndroid::initialize(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept {
try {
impl_->jNumberFormat_ = jni::make_global(
jNumberFormat_ = jni::make_global(
JNumberFormat::create(localesToJava(locales), optionsToJava(options)));
} catch (const std::exception &ex) {
return runtime.raiseRangeError(ex.what());
Expand All @@ -553,21 +644,29 @@ vm::ExecutionStatus NumberFormat::initialize(
return vm::ExecutionStatus::RETURNED;
}

vm::CallResult<std::unique_ptr<NumberFormat>> NumberFormat::create(
vm::Runtime &runtime,
const std::vector<std::u16string> &locales,
const Options &options) noexcept {
auto instance = std::make_unique<NumberFormatAndroid>();
if (LLVM_UNLIKELY(
instance->initialize(runtime, locales, options) ==
vm::ExecutionStatus::EXCEPTION)) {
return vm::ExecutionStatus::EXCEPTION;
}
return instance;
}

Options NumberFormat::resolvedOptions() noexcept {
return optionsFromJava(impl_->jNumberFormat_->resolvedOptions());
return static_cast<NumberFormatAndroid *>(this)->resolvedOptions();
}

std::u16string NumberFormat::format(double number) noexcept {
// I don't believe the Java logic can throw an exception (the JS
// method can, but the errors all come from the Intl.cpp logic). If
// I am incorrect, this will need to add a try/catch and take a
// runtime to call raiseRangeError on it. This is true for all the
// format methods.
return stringFromJava(impl_->jNumberFormat_->format(number));
return static_cast<NumberFormatAndroid *>(this)->format(number);
}

std::vector<Part> NumberFormat::formatToParts(double number) noexcept {
return partsFromJava(impl_->jNumberFormat_->formatToParts(number));
return static_cast<NumberFormatAndroid *>(this)->formatToParts(number);
}

} // namespace platform_intl
Expand Down
Loading

0 comments on commit 05f002d

Please sign in to comment.