Skip to content

util: Catch translation string errors at compile time #30383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2024
Merged

util: Catch translation string errors at compile time #30383

merged 1 commit into from
Jul 11, 2024

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 3, 2024

The translation helper function _() has many problems. For example, the following compiles:

auto ptr{"wrong"};
_(ptr);
_(nullptr);
_(0);
_(NULL);

However, it is wrong, because none of the arguments passed to the function can be picked up by the translation tooling for transifex.

Fix all issues by enforcing only real string literals can be passed to the function.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 3, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ryanofsky, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@maflcko
Copy link
Member Author

maflcko commented Jul 3, 2024

Can be tested by looking at the CI failure, which should pop up soon, or by manually inserting the C++ code from the pull request description.

@maflcko maflcko mentioned this pull request Jul 3, 2024
Merged
@fanquake
Copy link
Member

fanquake commented Jul 3, 2024 

  CXX      test/fuzz/fuzz-string.o
  CXX      libbitcoin_node_a-validation.o
test/fuzz/string.cpp:104:13: error: call to consteval function 'ConstevalStringLiteral::ConstevalStringLiteral' is not a constant expression
    (void)_(random_string_1.c_str());
1 error generated.

validation.cpp:5750:30: error: call to consteval function 'ConstevalStringLiteral::ConstevalStringLiteral' is not a constant expression
        return util::Error{_(reason)};
                             ^
validation.cpp:5750:30: note: function parameter 'reason' with unknown value cannot be used in a constant expression
validation.cpp:5734:49: note: declared here
    auto cleanup_bad_snapshot = [&](const char* reason) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
                                                ^
1 error generated.

@hebasto
Copy link
Member

hebasto commented Jul 3, 2024

Concept ACK on the PR's goal.

@ryanofsky ryanofsky mentioned this pull request Jul 3, 2024
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 5, 2024

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/27075181532

@DrahtBot DrahtBot mentioned this pull request Jul 5, 2024
Merged
@maflcko maflcko marked this pull request as ready for review July 10, 2024 07:39
@maflcko
Copy link
Member Author

maflcko commented Jul 10, 2024

Rebased to drop merged commit.

Current CI failure is unrelated and can be ignored.

ryanofsky
ryanofsky approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa601ab

@DrahtBot DrahtBot requested a review from hebasto July 10, 2024 14:03
hebasto
hebasto approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa601ab.

nit: The error messages for the cases _(0); and _(NULL); can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.

@maflcko
Copy link
Member Author

maflcko commented Jul 10, 2024

nit: The error messages for the cases _(0); and _(NULL); can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.

Unrelated to this codebase, but I'd say this should be fixed upstream by doing #define NULL nullptr (under C++11 or later) instead of using an integer literal.

@fanquake fanquake merged commit a231cfe into bitcoin:master Jul 11, 2024
@maflcko maflcko deleted the 2407-trans branch July 12, 2024 09:19
@bitcoin bitcoin locked and limited conversation to collaborators Jul 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @DrahtBot @fanquake @hebasto @ryanofsky