The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, Assert can be used to abort the program and avoid UB should the assumption ever be violated.
Concept ACK. Two questions:
Assert can be confusing because of assert? (but I don’t have a better name)
Why would they be confusing? Replacing assert with Assert by accident will compile into the same binary. And replacing Assert with assert will either compile into the same binary or it won’t compile at all.
Changed to a macro, thx for the suggestion.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Assert() seems weird to me too; I don’t expect it to return a value. ValidPointer() or having it convert a T* to a T& and using it as SafeDeref(ptr).member or similar would make more sense to me.
Either way, might as well replace the remaining two EnsureChainman() calls in init.cpp and rpc/blockchain.cpp and remove the function from node/context.h entirely by the looks.
This used to be called Ensure() with a deref built in, but I changed it to Assert for the following reason: #18923 (review)
I am happy to pick either version, as long as reviewers are happy.
replace the remaining two …
Done in the last force push.
54+}
55+
56+/** Identity function. Abort if the value compares equal to zero */
57+#define Assert(val) \
58+ [&](decltype(get_pure_r_value(val))&& value) -> decltype(get_pure_r_value(val)) { \
59+ assert(value); \
In commit “util: Add Assert identity function” (faba565dbddc215ef3734b138b673263786fae53)
I don’t think this is doing what it’s intended to do. It always prints Assertion 'value' failed. regardless of what val macro argument was passed in, I think defeating the purpose of using a macro.
To make it actually print the macro argument you could use the stringfication operator # like
0#define ASSERT(x) [&]() -> decltype(x)& { auto& check = (x); assert(#x && check); return check; }()
For printf("i = %i\n", *Assert(i)); this changes output from
0output.s: ./example.cpp:34: main(int, char**)::<lambda(std::unique_ptr<int>&)>: Assertion `value' failed.
to
0output.s: ./example.cpp:34: main(int, char**)::<lambda()>: Assertion `"i" && check' failed.
The utility is primarily useful to dereference pointer types, which are
known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are
started. Instead of silently relying on that assumption, Assert can be
used to abort the program and avoid UB should the assumption ever be
violated.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i -e 's/EnsureChainman\((m?_?node)\)\./Assert(\1.chainman)->/g' $(git grep -l EnsureChainman)
-END VERIFY SCRIPT-
For reviewers who think assert is “weird” and “confusing” and wouldn’t “expect it to return a value,” I don’t see what point you are making. If you saw fun(*ASSERT(ptr)) and had to guess what it was doing, would you actually guess anything other than “this is going to assert ptr is true, dereference it and pass it to fun? If not, what would your guess be?
I like the name ASSERT for this macro, because c++ developers will generally know that a false assert will abort the program. If the name ASSERT is misleading or confusing in a specific way, something longer but more explicit like ABORT_IF_NULL would serve the purpose as well.
As long as this is aborting the program on failure, I think calling this SafeDeref or Ensure or hiding the * operation from the reader is exactly what we don’t want here. We don’t want a reviewer to look at this code and think that dereferencing the pointer is safe. We want a reviewer to look at this code and see that a pointer is being dereferenced, and that something bad will happen if the pointer is null, and look closely to make sure there are no code paths where it could be null. That is why it is better to keep the *, because reviewers will generally already do this when they see a pointer being dereferenced.
I think the name SafeDeref is specifically bad because I’d expect a function with that name to either safely dereference the pointer or return a default constructed value. I think the name Ensure is specifically bad because there are other cases where Ensure is used in the code and it’s basically harmless (throws JSON-RPC a REST exception and returns it to the client).
ASSERT() instead of assert().
it’s valid to use ASSERT() instead of assert()
Yes, it is valid and should compile to the same binary. (Just like it is fine to replace (++i) with (i++) and it compiles to the same binary). Is there some obvious risk or downside I am missing?
assert() should be used over ASSERT().
52+{
53+ return std::forward<T>(val);
54+}
55+
56+/** Identity function. Abort if the value compares equal to zero */
57+#define Assert(val) [&]() -> decltype(get_pure_r_value(val))& { auto& check = (val); assert(#val && check); return check; }()
In commit “util: Add Assert identity function” (fa6ef701adba1cb48535cac25fd43c742a82e40d)
Note: decltype(get_pure_r_value(val)) is needed here in case the val expression is const, because just decltype(val) drops the const and results in an error trying to return a const reference from a lambda effectively declared to return a non-const reference. I think returning decltype(auto) would be another way to work around this once we update past c++11
For reviewers who think assert is “weird” and “confusing” and wouldn’t “expect it to return a value,”
I think it’s important to keep scrutiny on the changes, not on the people. “Tough on the changes, easy on the people.” I’ve deleted my review.
Code review ACK fab80fef61ddd4afeff6e497c7e76bffcd05e8a4
I mean confusing not because of return value but because now it’s valid to use
ASSERT()instead ofassert().
Oh, thanks for explaining. I wouldn’t be too worried about which assert someone picks when writing code, but the macro documentation could be updated to say something about when it should be used, if there are opinions about that. I think it’s basically fine to use or not to not use, as long as it’s reasonably clear what it does when it is used.
I’ve deleted my review
I found the review useful and I adjusted the commit based on the review to include motivation for the change (and fix the typo). Sometimes my commit messages are too short (i.e. empty) when they instead could include further background information and motivation for the changes, so at least I found the review helpful.
re: @jonatack #19277 (comment)
For reviewers who think assert is “weird” and “confusing” and wouldn’t “expect it to return a value,”
I think it’s important to keep scrutiny on the changes, not on the people. “Tough on the changes, easy on the people.” I’ve deleted my review.
I’m confused. What did your review say? In my email it was a concept ACK with some suggestions not related to this.
Did I phrase my comment badly? I don’t think I was scrutinizing anything, just asking a literal, non-rhetorical question to reviewers expressing an opinion I didn’t understand. It turned one reviewer clarified and didn’t actually have that opinion, so I’m glad I asked. But if I should have asked differently, I’d like to know.
Tested ACK fab80fef61ddd4afeff6e497c7e76bffcd05e8a4.
Indeed it’s better with macro and templating ninjatsus. Applying
0--- a/src/bitcoind.cpp
1+++ b/src/bitcoind.cpp
2@@ -44,6 +44,7 @@ static void WaitForShutdown(NodeContext& node)
3 static bool AppInit(int argc, char* argv[])
4 {
5 NodeContext node;
6+ Assert(node.chain);
7 node.chain = interfaces::MakeChain(node);
8
9 bool fRet = false;
Gives
0src/bitcoind -regtest
1Assertion failed: ("node.chain" && check), function operator(), file bitcoind.cpp, line 47.
2[1] 90659 abort src/bitcoind -regtest
If you saw
fun(*ASSERT(ptr))and had to guess what it was doing,
Why would you ever have to guess? I’d think “ugh, what’s going on, assert doesn’t work like that” and look at the source, and conclude “oh, they’re just doing some weird local convention where assert in all caps is slightly different than regular assert”. That’s not a showstopper, it’s just confusing.
If I saw fun(Frobniz(ptr)), I’d think “well, Frobniz is new to me, better look at the source to see what it does” followed by “oh, it asserts if it’s null, and otherwise passes it through, cool”.
+1 on having Ensure* only do JSON RPC errors. Though, looking at it, the RPC system does both that and CHECK_NONFATAL which throws runtime_error instead, and points directly to where in the source the problem is; might be better to make use of that more consistently instead.
Anything along the lines of ABORT_IF_NULL, if_null_assert, BUG_IF_NULL, fatal_if_null, Require, RequireNonNull or similar wouldn’t be confusing to me. fatal_if_null might go nicely with CHECK_NONFATAL. Again, not a showstopper, I just think not overloading standard terms is an improvement.
If you saw
fun(*ASSERT(ptr))and had to guess what it was doing,Why would you ever have to guess?
Thanks, that’s kind of my point. You don’t have to guess. If you have any question, you can look at the ASSERT macro and see that it wraps standard assert. But when reading code, people make assumptions about how things work, and when the likely assumptions are wrong, it can be a problem for readability and a future source of bugs. I still don’t understand why you are saying something is “confusing” when it doesn’t appear there is any actual fact you think someone might be confused about, but it’s clear you don’t like the aesthetics of overloading the word “assert” here. This is bad, but not as bad as if you thought it caused a practical problem.
Re suggestions:
Any case, I’m glad we seem to agree there’s no readability problem here. I think current PR is fine. My favorite name would probably be ASSERT, followed by one of the names that contains ABORT or FATAL, because REQUIRE doesn’t sound scary enough, and doesn’t make me think that world may end if the pointer is null. But Marco’s the guy putting in the work here, and whatever name he likes I am happy with.
Another reason I like the name Assert is that this plays nicely with our long-term goal to have different levels of checks. See #4576 (comment) for context. It says: “I guess we can have two levels, optional and mandatory checks.”
The thread also mentions the danger of building with NDEBUG, which is forbidden since 9b59e3bda8c137bff885db5b1f9150346e36e076. Given that it is impossible to build with NDEBUG, no one is testing this configuration and it is effectively unsupported. However, the check is only in one place (main.cpp, or current master: validation and net_processing, or now: check.h). So certainly it is impossible to build bitcoind with NDEBUG. Though, I am less sure about other binaries. And certainly it is possible to (accidentally) build single modules with NDEBUG and then accidentally link them. Haven’t checked if this is possible, but the potential risk seems already too high. To fix that, I could imagine a scripted-diff that changes assert to Assert and adds includes for check.h, which would abort compilation under NDEBUG.
Yet another reason I like Assert is that it could make it easier to privide a “pimp-my-assert” version of a plain assert. As an idea it could print additional information, for example:
0 tfm::format(std::cerr, "%s:%d (%s)\n" \
1 "Internal bug detected: '%s'\n" \
2 "Aborting!\n" \
3 "You may report this issue here: %s\n", \
4 __FILE__, __LINE__, __func__, \
5 (#condition), \
6 PACKAGE_BUGREPORT); \
Example taken from #17408.
So using the name Assert instead of e.g. Require would make the least amount of changes in the places where previously assert was used.
Unrelated, but the discussion about Assume was in #16136.