-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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.
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
Remove checks for nullptr from BlockAssembler::CreateNewBlock #19273
Remove checks for nullptr from BlockAssembler::CreateNewBlock #19273
Conversation
operator new() does not normally ever return nullptr unless one explicitly uses the `std::nothrow` version or unless one redefines it. Neither is the case in this entire codebase, hence the check for nullptr for the return value from BlockAssembler::CreateNewBlock was entirely superfluous and all brances that test it would always evaluate to false. So, it can be safely removed in the interests of code quality. See: https://en.cppreference.com/w/cpp/memory/new/operator_new tl;dr: operator new() either succeeds or throws std::bad_alloc, it can never return nullptr.
If it is impossible to return a nullptr, then why not return a plain |
@MarcoFalke Good question. I'm not sure what the motivation is behind the Why it allocates a |
What I meant is that simply removing nullptr checks is scary and will also fail to compile on some compilers/sanitizers/static analysers with warnings and errors enabled. Also, if the template ever only exists on the stack, then I don't see why it should live in the heap. I am not sure if there is a way in C++ to ensure an object can be efficiently moved, but a pre-C++11 alternative would be: CBlockTemplate on_the_stack;
BlockAssembler().CreateNewBlock(coinbase_script, on_the_stack); |
Yeah that was the pre-C++11 canonical way to do it. Also just returning the value was pretty much always guaranteed to not produce any copies and be equivalent to the "pass down a non-const ref" way.. (RVO/copy elision basically does that for you internally anyway). The example you show is the cheapest way (as is a proper RVO return by value), for sure. Move semantics are "almost free" too -- you just have to pay the extra cost of swapping pointers, etc (CBlockTemplate's heaviest members are the vector of tx refs and other stuff and so you pay the cost of that pointer swap...).
:) It is only scary if you are thinking about it as just a fancy However, in C++, If you find this PR useless or scary, feel free to close it. I figured rather than complain I could help out and fix the little nits here and there. No worries either way. |
Oh, I meant that compilers can't know that const auto b = CreateNewBlock();
b->Get(); // <-- Warning/Error here
We do appreciate refactoring changes that improve the code base. My feedback was about the current version of the pull request, which I believe can not be merged as-is. |
No static analyzer or compiler expects you to check every single pointer you get given or receive as a return value. Having such a strict requirement would mean your code would be riddled with boilerplate checks everywhere. (instead, at best, some static analyzers sometimes use heuristics to warn you about obviously bad usages). Just to reiterate -- code like this: auto ptr = std::make_unique<MyClass>(arg1, arg2);
if (!ptr)
// handle error here Is unidiomatic and kind of silly... since it can never be false. For that reason no static analyzer that I know of expects you to check Background: Usually static analyzers do not expect you to check for NULL for every single pointer return type you use. Doing so would be boilerplate city. They usually expect you to check for NULL before using only if the analyzer concludes (based on heuristics) that the pointer may be NULL sometimes. For example, if you later check for NULL in the function -- this indicates to the analyzer that you expect it to be NULL sometimes. If it cannot conclude that the pointer may be NULL, it will not complain. |
For our codebase, unique_ptr means that the pointer can be null. E.g.
So I think it would be confusing to have one exception where a pointer type is used but at the same time it can not be null. |
Right, but in this case the contract is that it cannot be null. I get it -- this is a minor nit and not worth worrying about -- no worries. I still maintain that it would be a code quality improvement. It's a bit silly to see people checking pblocktemplate.reset(new CBlockTemplate());
if(!pblocktemplate.get())
return nullptr; Then that's your prerogative, I guess. But I get it. You don't want to worry about this. Feel free to close this. Like I said -- no harm done, no offense taken, and no worries. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
I agree with @MarcoFalke. Please make it either impossible to return a nullptr (by returning a while object) or keep the checks somehow. Simply removing them is scary. |
🐙 This pull request conflicts with the target branch and needs rebase. |
My point is -- It's already impossible to return a nullptr. Which is why the check is silly. It's the equivalent of doing:
The above example may seem like an exaggeration, of course -- but if you stop to think about it -- it really is that silly. Anyway, I'm closing this. I feel like I'm fighting against the wind here. Good luck. |
Motivation/rationale: Minor nit.
Background:
See: https://en.cppreference.com/w/cpp/memory/new/operator_new
operator new()
does not normally ever returnnullptr
unless oneexplicitly uses the
std::nothrow
version or unless one redefines it. Neitheris the case in this entire codebase, hence the check for
nullptr
for thereturn value from
BlockAssembler::CreateNewBlock
was entirelysuperfluous and all branches that test it would always evaluate to
false
. So, it can be safely removed in the interests of code quality.tl;dr:
operator new()
either succeeds or throwsstd::bad_alloc
, it cannever return
nullptr
.