util: Catch translation string errors at compile time #30383

1. 
   maflcko commented at 11:04 am on July 3, 2024: member

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

   0auto ptr{"wrong"};
   1_(ptr);
   2_(nullptr);
   3_(0);
   4_(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.

2. 
   DrahtBot commented at 11:04 am on July 3, 2024: contributor

   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.

3. DrahtBot added the label Utils/log/libs on Jul 3, 2024
4. 
   maflcko commented at 11:07 am on July 3, 2024: member

   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.
5. 
   fanquake commented at 11:12 am on July 3, 2024: member

    0  CXX      test/fuzz/fuzz-string.o
    1  CXX      libbitcoin_node_a-validation.o
    2test/fuzz/string.cpp:104:13: error: call to consteval function 'ConstevalStringLiteral::ConstevalStringLiteral' is not a constant expression
    3    (void)_(random_string_1.c_str());
    41 error generated.
    5
    6validation.cpp:5750:30: error: call to consteval function 'ConstevalStringLiteral::ConstevalStringLiteral' is not a constant expression
    7        return util::Error{_(reason)};
    8                             ^
    9validation.cpp:5750:30: note: function parameter 'reason' with unknown value cannot be used in a constant expression
   10validation.cpp:5734:49: note: declared here
   11    auto cleanup_bad_snapshot = [&](const char* reason) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
   12                                                ^
   131 error generated.
   

6. 
   hebasto commented at 11:18 am on July 3, 2024: member

   Concept ACK on the PR’s goal.
7. maflcko force-pushed on Jul 5, 2024
8. 
   DrahtBot commented at 9:55 am on July 5, 2024: contributor

   🚧 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

9. DrahtBot added the label CI failed on Jul 5, 2024
10. DrahtBot added the label Needs rebase on Jul 9, 2024
11. maflcko marked this as ready for review on Jul 10, 2024
12. util: Catch translation string errors at compile time fa601ab9f7
13. maflcko force-pushed on Jul 10, 2024
14. DrahtBot removed the label Needs rebase on Jul 10, 2024
15. 
    maflcko commented at 8:55 am on July 10, 2024: member

    Rebased to drop merged commit.

    Current CI failure is unrelated and can be ignored.

16. DrahtBot removed the label CI failed on Jul 10, 2024
17. ryanofsky approved
18. 
    ryanofsky commented at 2:03 pm on July 10, 2024: contributor

    Code review ACK fa601ab9f7142cdb18c18c1128fc893cdffb3463
19. DrahtBot requested review from hebasto on Jul 10, 2024
20. hebasto approved
21. 
    hebasto commented at 4:32 pm on July 10, 2024: member

    ACK fa601ab9f7142cdb18c18c1128fc893cdffb3463.

    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.

22. 
    maflcko commented at 4:51 pm on July 10, 2024: member

    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.

23. fanquake merged this on Jul 11, 2024
24. fanquake closed this on Jul 11, 2024

    ---

25. maflcko deleted the branch on Jul 12, 2024

Contributors
maflcko DrahtBot fanquake hebasto ryanofsky

Labels
Utils/log/libs