Skip to content

Add experimental Converter::GetConversionCandidates behind a macro guard - #1431

Merged
frankslin merged 2 commits into
BYVoid:masterfrom
frankslin:add-conversion-candidates-api
Jul 15, 2026
Merged

frankslin merged 2 commits into
BYVoid:masterfrom
frankslin:add-conversion-candidates-api

Conversation

@frankslin

Copy link
Copy Markdown
Collaborator

Introduce a librime-style word-level candidate query that enumerates every form a single word can take through a converter's conversion chain (e.g. s2t expands 里 to 里 and 裏). The API is gated by OPENCC_ENABLE_UNSTABLE_API so it stays out of the installed headers and the OPENCC_ABI_VERSION contract until it is promoted at the next ABI bump.

The member is deliberately non-virtual for now, so enabling the macro does not alter the Converter vtable and remains ABI-safe; it delegates to a free function kept in a private (non-installed) header, making the eventual promotion to a virtual method a trivial change.

Detailed Changes:

  • New candidate API:
    • Add GetAllConversions(const Converter&, std::string_view) in the private ConversionCandidates.hpp/.cpp, porting librime's ConvertWord loop onto OpenCC's Dict/DictEntry primitives (exact match yields all values; otherwise longest-prefix default conversion carries partial matches down the chain).
    • Add the guarded, non-virtual Converter::GetConversionCandidates member that forwards to the free function; define it alongside the free function to avoid a Bazel dependency cycle with converter_lib.
  • Build wiring:
    • CMake: register the new source/private header/test and define OPENCC_ENABLE_UNSTABLE_API as PRIVATE on libopencc and the test target.
    • Bazel: add conversion_candidates_lib and its test using non-propagating local_defines, keeping the header out of exports_files.
  • Tests:
    • Add ConversionCandidatesTest covering multi-value expansion, chained branch carry-through, dedup, not-found and prefix-only empties, partial conversion in a later dict, PipelineConverter returning empty, and member/free-function agreement.

Introduce a librime-style word-level candidate query that enumerates every
form a single word can take through a converter's conversion chain (e.g. s2t
expands 里 to 里 and 裏). The API is gated by OPENCC_ENABLE_UNSTABLE_API so it
stays out of the installed headers and the OPENCC_ABI_VERSION contract until it
is promoted at the next ABI bump.

The member is deliberately non-virtual for now, so enabling the macro does not
alter the Converter vtable and remains ABI-safe; it delegates to a free function
kept in a private (non-installed) header, making the eventual promotion to a
virtual method a trivial change.

Detailed Changes:
- **New candidate API**:
  - Add `GetAllConversions(const Converter&, std::string_view)` in the private
    `ConversionCandidates.hpp/.cpp`, porting librime's ConvertWord loop onto
    OpenCC's Dict/DictEntry primitives (exact match yields all values; otherwise
    longest-prefix default conversion carries partial matches down the chain).
  - Add the guarded, non-virtual `Converter::GetConversionCandidates` member
    that forwards to the free function; define it alongside the free function to
    avoid a Bazel dependency cycle with converter_lib.
- **Build wiring**:
  - CMake: register the new source/private header/test and define
    `OPENCC_ENABLE_UNSTABLE_API` as PRIVATE on libopencc and the test target.
  - Bazel: add `conversion_candidates_lib` and its test using non-propagating
    `local_defines`, keeping the header out of `exports_files`.
- **Tests**:
  - Add ConversionCandidatesTest covering multi-value expansion, chained branch
    carry-through, dedup, not-found and prefix-only empties, partial conversion
    in a later dict, PipelineConverter returning empty, and member/free-function
    agreement.
Code review of the experimental candidate-enumeration API found a heap
over-read on truncated UTF-8, divergence from Convert() on IDS sequences and
key-only dictionary entries, a config normalization pre-pass being skipped,
and an ODR violation plus ineffective containment from the macro-guarded
Converter member.

Detailed Changes:
- **ConversionCandidates.cpp**:
  - Delegate the no-exact-match fallback to Conversion::Convert() instead of
    a hand-rolled prefix loop, fixing an out-of-bounds read on truncated
    UTF-8 input and restoring atomic handling of ideographic description
    sequences.
  - Treat a key-only dictionary entry as converting to itself, matching
    Convert()'s GetDefault() fallback, instead of silently dropping the word.
  - Normalize the input through ConfigBasedConverter's normalization
    pre-pass before walking the main chain, so standard configs like
    s2t.json no longer report "not found" for compatibility ideographs that
    Convert() handles correctly.
- **Converter.hpp / CMakeLists.txt / BUILD.bazel**: Remove the
  OPENCC_ENABLE_UNSTABLE_API-guarded Converter member and all four macro
  definition sites. The guard gave class Converter differing definitions
  across translation units (an ODR violation, live today in the Bazel
  build), and Converter.hpp is an installed public header, so the API was
  never actually contained. Only the private free function GetAllConversions
  remains; a member can be added in one step at the next ABI bump.
- **BUILD.bazel**: Restrict conversion_candidates_lib to package-private
  visibility, since exports_files does not gate cc_library header
  reachability as the previous comment assumed. Split ConfigBasedConverter.hpp
  out of config_lib into its own package-private target so both config_lib
  and conversion_candidates_lib can depend on it.
- **ConversionCandidatesTest.cpp**: Switch tests to call GetAllConversions
  directly; add regression coverage for truncated UTF-8, IDS atomicity,
  key-only entries, and the normalization pre-pass.
@frankslin
frankslin merged commit 9fef141 into BYVoid:master Jul 15, 2026
37 checks passed
@frankslin
frankslin deleted the add-conversion-candidates-api branch July 15, 2026 04:03
frankslin added a commit to frankslin/OpenCC that referenced this pull request Aug 26, 2026
GetAllConversions() and ConvertWithAmbiguities() were added in 1.4.2 for
input-method engines, but their headers stayed in LIBOPENCC_PRIVATE_HEADERS,
which CMake does not install. The symbols are exported, so a downstream
project links fine -- it just has no declaration to include when building
against an installed OpenCC, which is exactly how librime consumes us.

Move ConversionCandidates.hpp, ConversionAmbiguities.hpp and StreamWindow.hpp
(needed for AmbiguityStream's default argument) into the installed set, drop
the 'internal, unstable, not installed' caveats, and widen the Bazel targets'
visibility. Purely additive: SOVERSION stays 1.4 and no existing downstream
needs relinking.

The 1.4.2 changelog entry described a Converter::GetConversionCandidates
member function and an OPENCC_ENABLE_UNSTABLE_API macro from an intermediate
design in BYVoid#1431; neither shipped. Leave the published text as it stands and
add an erratum at the top of that section instead of rewriting it.
frankslin added a commit that referenced this pull request Aug 26, 2026
GetAllConversions() and ConvertWithAmbiguities() were added in 1.4.2 for
input-method engines, but their headers stayed in LIBOPENCC_PRIVATE_HEADERS,
which CMake does not install. The symbols are exported, so a downstream
project links fine -- it just has no declaration to include when building
against an installed OpenCC, which is exactly how librime consumes us.

Move ConversionCandidates.hpp, ConversionAmbiguities.hpp and StreamWindow.hpp
(needed for AmbiguityStream's default argument) into the installed set, drop
the 'internal, unstable, not installed' caveats, and widen the Bazel targets'
visibility. Purely additive: SOVERSION stays 1.4 and no existing downstream
needs relinking.

The 1.4.2 changelog entry described a Converter::GetConversionCandidates
member function and an OPENCC_ENABLE_UNSTABLE_API macro from an intermediate
design in #1431; neither shipped. Leave the published text as it stands and
add an erratum at the top of that section instead of rewriting it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant