Skip to content

Add explicit conversion to std::string#1019

Merged
kelson42 merged 1 commit intoopenzim:mainfrom
ojwb:xapian-2.0-string_view-fix
Dec 12, 2025
Merged

Add explicit conversion to std::string#1019
kelson42 merged 1 commit intoopenzim:mainfrom
ojwb:xapian-2.0-string_view-fix

Conversation

@ojwb
Copy link
Copy Markdown
Contributor

@ojwb ojwb commented Dec 11, 2025

In Xapian 2.0.x most API methods which previously took std::string or const std::string& now take std::string_view, which can avoid the need to copy data which isn't already in a std::string.

That causes a compilation failure here - the compiler knows how to convert zim::Formatter to std::string and how to convert std::string to std::string_view but it isn't allowed to chain implicit conversions.

We could use https://en.cppreference.com/w/cpp/io/basic_ostringstream/view.html to add a conversion to std::string_view to zim::Formatter, but that was added in C++20 and meson.build currently requires C++17. I don't see a good way to add a conversion operator which works for older C++ standards.

Incidentally, storing the word count in a value slot in this way doesn't seem the best idea. Sorting by value performs a string sort, so sorting by word count would currently give e.g. 999999 > 9 > 80 > 700 > 6000 > 50000 > 400000 > 3000000 > 20000000 > 1000000.

Xapian::sortable_serialise() encodes numbers as strings which sorts the same way as the numbers do, but a switch to that would presumably need to consider how to handle existing databases.

In Xapian 2.0.x most API methods which previously took std::string
or const std::string& now take std::string_view, which can avoid
the need to copy data which isn't already in a std::string.

That causes a compilation failure here - the compiler knows how to
convert zim::Formatter to std::string and how to convert std::string
to std::string_view but it isn't allowed to chain implicit conversions.
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 12, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 56.18%. Comparing base (514d446) to head (8ac9421).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
src/writer/xapianWorker.cpp 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1019   +/-   ##
=======================================
  Coverage   56.18%   56.18%           
=======================================
  Files         101      101           
  Lines        4989     4989           
  Branches     2170     2170           
=======================================
  Hits         2803     2803           
  Misses        739      739           
  Partials     1447     1447           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@veloman-yunkan
Copy link
Copy Markdown
Collaborator

Incidentally, storing the word count in a value slot in this way doesn't seem the best idea. Sorting by value performs a string sort, so sorting by word count would currently give e.g. 999999 > 9 > 80 > 700 > 6000 > 50000 > 400000 > 3000000 > 20000000 > 1000000.

Xapian::sortable_serialise() encodes numbers as strings which sorts the same way as the numbers do, but a switch to that would presumably need to consider how to handle existing databases.

To the best of my knowledge the word count is only used to decorate the search results with additional info about the documents with a match. Currently there is no support for sorting/filtering the search results based on word count.

@kelson42 kelson42 added this to the 9.5.0 milestone Dec 12, 2025
@kelson42 kelson42 merged commit 9ca8eb0 into openzim:main Dec 12, 2025
25 of 27 checks passed
@kelson42 kelson42 modified the milestones: 9.5.0, 9.4.1 Jan 2, 2026
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.

3 participants