refactor: convert ContainsNoNUL to ContainsNUL

PastaPastaPasta commented at 5:34 am on November 17, 2024: contributor

This allows us to convert all the double negative calls such as !ContainsNoNUL() -> ContainsNUL().

There are no usages which aren’t double negatives. This makes the code more understandable

Also, use str.find instead of manually looping

DrahtBot commented at 5:34 am on November 17, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31301.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label Refactoring on Nov 17, 2024

PastaPastaPasta force-pushed on Nov 17, 2024

DrahtBot added the label CI failed on Nov 17, 2024

DrahtBot commented at 5:42 am on November 17, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33094507644

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

DrahtBot removed the label CI failed on Nov 17, 2024

in src/util/string.h:216 in a019c2d6d7 outdated

214@@ -214,12 +215,9 @@ inline std::string MakeUnorderedList(const std::vector<std::string>& items)
215 /**
216  * Check if a string does not contain any embedded NUL (\0) characters

flack commented at 10:47 am on November 17, 2024:

I guess this should be updated, too

in src/util/string.h:220 in a019c2d6d7 outdated

220 {
221-    for (auto c : str) {
222-        if (c == 0) return false;
223-    }
224-    return true;
225+    return std::ranges::any_of(str, [](char c) { return c == 0; });

l0rinc commented at 11:14 am on November 17, 2024:

We should be able to simplify further:

0    return str.find('\0') != std::string_view::npos;

PastaPastaPasta commented at 4:01 am on November 18, 2024:

Applied in https://github.com/bitcoin/bitcoin/pull/31301/commits/6077262788333759f1ddc27539e86cd04ac8d076

in src/util/string.h:217 in a019c2d6d7 outdated

214@@ -214,12 +215,9 @@ inline std::string MakeUnorderedList(const std::vector<std::string>& items)
215 /**
216  * Check if a string does not contain any embedded NUL (\0) characters
217  */
218-[[nodiscard]] inline bool ContainsNoNUL(std::string_view str) noexcept
219+[[nodiscard]] inline bool ContainsNUL(std::string_view str) noexcept

l0rinc commented at 11:14 am on November 17, 2024:

[[nodiscard]] doesn’t make a lot of sense here, since the method doesn’t have any side-effect

PastaPastaPasta commented at 4:02 am on November 18, 2024:

I think I disagree here. If the function had side effects, it may make sense to not have [[nodiscard]] as you may intend to call the function, without caring what it returns. In this instance though, there is no purpose to call ContainsNUL and not use the returning value, so that would be indicative of a bug.

l0rinc commented at 5:14 pm on November 18, 2024:

what was the side effect and why is that important? Calling this without the return value would just be removed as dead code.

PastaPastaPasta commented at 5:21 pm on November 18, 2024:

Calling this without the return value would just be removed as dead code.

Yes, it almost certainly would; but with [[nodiscard]] it simply won’t compile. This is better as having some code such that calls ContainsNUL but doesn’t use it, must be a bug, as there is no purpose to call it, and not use it.

l0rinc changes_requested

l0rinc commented at 11:25 am on November 17, 2024: contributor

While refactor-only PRs are often frowned upon (since the benefit is small, but the risk is big), I think this is small enough for us to consider.

You could simplify further by changing the occurrences via https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs (making sure all commit compile)

Note that the utxo_snapshot.cpp(45,1): error C1001: Internal compiler error seems unrelated to this change.

(nit: typo in description)

PastaPastaPasta force-pushed on Nov 18, 2024

PastaPastaPasta renamed this:
~~refactor: covert ContainsNoNUL to ContainsNUL~~
refactor: convert ContainsNoNUL to ContainsNUL
on Nov 18, 2024

in src/util/string.h:220 in 6077262788 outdated

220 {
221-    for (auto c : str) {
222-        if (c == 0) return false;
223-    }
224-    return true;
225+    return str.find('\0') != std::string_view::npos;;

maflcko commented at 7:47 am on November 18, 2024:

I guess once there is C++23, we could just remove this function and use str.contains('\0') directly everywhere instead of ContainsNUL(str)? If so, a simple comment to say so may be sufficient to avoid having to touch the same lines twice?

flack commented at 4:22 pm on November 18, 2024:

Nit: double semicolon

PastaPastaPasta commented at 4:28 pm on November 18, 2024:

resolved both!

PastaPastaPasta force-pushed on Nov 18, 2024

refactor: convert ContainsNoNUL to ContainsNUL

This allows us to convert all the double negative calls such as `!ContainsNoNUL()` -> `ContainsNUL()`. There are no usages which aren't double negatives.

Also, use std::any_of instead of manually looping

3f3baa24cf

PastaPastaPasta force-pushed on Nov 18, 2024

in src/util/string.h:215 in 3f3baa24cf

211@@ -212,14 +212,12 @@ inline std::string MakeUnorderedList(const std::vector<std::string>& items)
212 }
213 
214 /**
215- * Check if a string does not contain any embedded NUL (\0) characters
216+ * Check if a string contains any embedded NUL (\0) characters

l0rinc commented at 5:13 pm on November 18, 2024:

the comment just repeats what the code already states clearly, I’d just remove it

PastaPastaPasta commented at 5:34 pm on November 18, 2024:

I would disagree, this way IDEs can show it as popup / doxygen can use it. Plus it is still accurate

l0rinc commented at 6:07 pm on November 18, 2024:

Given that you forgot to update it, I’d say it doesn’t have a big added value, let’s just delete it.

PastaPastaPasta commented at 6:09 pm on November 18, 2024:

I’d like to see another concept ACK to removing the comment I think before I do so, if someone else thinks it’s valuable to drop the comment, I will.

l0rinc changes_requested

l0rinc commented at 5:16 pm on November 18, 2024: contributor

Based on #31301 (review), to set your expectations, this likely won’t be merged (we can just postpone the fix until C++23, we’re not in a rush), but I don’t mind reviewing anyway. Please update the description after every change.

PastaPastaPasta commented at 5:27 pm on November 18, 2024: contributor

Based on #31301 (comment), to set your expectations, this likely won’t be merged (we can just postpone the fix until C++23, we’re not in a rush)

I would argue against this mentality :) I can’t tell for sure, but my guess is requiring c++23 is at least a year away from being merged? Makes more sense imo, to merge this in, and then once c++23 is available, update this code. (Also, I try to do PRs like this to move towards contributing upstream to bitcoin core more, and having PRs like this stall makes it hard to be motivated to try larger tasks)

refactor: convert ContainsNoNUL to ContainsNUL #31301

Code Coverage & Benchmarks

Reviews