Skip to content

[RNE Rewrite] Deduplicate tokenizer result-unwrapping with a local unwrap helper - #1320

Merged
msluszniak merged 1 commit into
rne-rewritefrom
@ms/tokenizer-unwrap
Jul 20, 2026
Merged

[RNE Rewrite] Deduplicate tokenizer result-unwrapping with a local unwrap helper#1320
msluszniak merged 1 commit into
rne-rewritefrom
@ms/tokenizer-unwrap

Conversation

@msluszniak

Copy link
Copy Markdown
Member

Description

The five TokenizerHostObject methods (encode, decode, idToToken, tokenToId, and the encode path) each hand-rolled the same block around a tokenizers::Result:

auto result = self->tokenizer_->encode(...);
if (!result.ok()) {
    throw jsi::JSError(rt, std::format("encode: Failed to encode input: {}", toString(result.error())));
}
return conversions::toJsiArray(rt, result.get());

Replaced with a local unwrap helper in the file's anonymous namespace, mirroring the one already in core/model.cpp. That existing helper is file-local and typed to executorch::runtime::Result, so it cannot be reused here — the tokenizer returns tokenizers::Result carrying a distinct tokenizers::Error (which is why this file already has its own toString). Following the established per-file convention rather than hoisting a shared helper into core keeps the change scoped and leaves core untouched.

Behaviour is identical: same error messages, same exceptions.

Split out of #1319 so the std::span refactor and this cleanup can be reviewed independently. Part of the broader tokenizer host-object cleanup tracked in #1316.

Introduces a breaking change?

  • Yes
  • No

Type of change

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Documentation update (improves or adds clarity to existing documentation)
  • Other (chores, tests, code style improvements etc.)

Tested on

  • iOS
  • Android

Testing instructions

Behaviour-preserving; verification is compile-only, not yet run on a device.

tokenizer.cpp compiles clean under the project's strict warning set from .clangd:

cd packages/react-native-executorch
clang++ -fsyntax-only $(tr '\n' ' ' < compile_flags.txt) \
  -Wall -Wextra -Wpedantic -Wconversion -Wsign-conversion cpp/extensions/nlp/tokenizer.cpp

To exercise at runtime: the tokenizer screen in apps/nlp.

Related issues

Part of #1316

Checklist

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings

…helper

The five tokenizer host-object methods each hand-rolled the same
`if (!result.ok()) throw` block around a tokenizers::Result. Replace them
with a local `unwrap` helper, mirroring the one in core/model.cpp (that
one is file-local and typed to executorch::runtime::Result, so it cannot
be reused for tokenizers::Result).
@msluszniak
msluszniak merged commit 0f2fa2a into rne-rewrite Jul 20, 2026
3 checks passed
@msluszniak
msluszniak deleted the @ms/tokenizer-unwrap branch July 20, 2026 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants