refactor: Remove superfluous “return” from “addUnchecked” in txmempool.cpp #21577

pull kiminuo wants to merge 1 commits into bitcoin:master from kiminuo:feature/2021-04-02-superfluous-return changing 1 files +1 −1

kiminuo commented at 8:15 pm on April 2, 2021: contributor

Honestly, I’m not sure whether it makes sense to fix it or not. Feel free to close this PR to avoid this being a precedent for “fixing” stuff like this.

Follow-up to #13774.
Remove superfluous "return" from "addUnchecked" in txmempool.cpp
Follow-up for #13774.
4e41cee9f5
DrahtBot added the label Mempool on Apr 2, 2021
DrahtBot added the label Refactoring on Apr 2, 2021
theStack approved
theStack commented at 8:41 pm on April 2, 2021: member

ACK 4e41cee9f5a0690e831f130837f110a89c4cbb0b

Removing confusing code and increasing readability is always welcome, even if it only consists of the removal of a single keyword :)

I’m wondering why the compiler doesn’t warn about trying to evaluate the result of a void-function-call in a non-trivial expression. I can’t think of a situation where this would make any sense.
Aristarkhov commented at 1:20 am on April 3, 2021: none

I’m wondering why the compiler doesn’t warn about trying to evaluate the result of a void-function-call in a non-trivial expression. I can’t think of a situation where this would make any sense.

I guess this happens because compilers support return void; in void function call, and addUnchecked returns void. In any other case there would have been a warning :)
curtcurt871 commented at 4:26 am on April 3, 2021: none

kiminuo:feature/2021-04-02-superfluous-return
MarcoFalke commented at 5:55 am on April 3, 2021: member

The return isn’t superfluous. It allows the compiler to enforce that the return types of the two identically named methods are identical.
theStack commented at 7:52 am on April 3, 2021: member

The return isn’t superfluous. It allows the compiler to enforce that the return types of the two identically named methods are identical.

Oh, now I get the point, I’m retreating my ACK then (though I still think it looks very confusing).
jnewbery commented at 8:40 am on April 3, 2021: member

ACK 4e41cee9f5a0690e831f130837f110a89c4cbb0b

At best, this is misleading for people reading the code.

The return isn’t superfluous. It allows the compiler to enforce that the return types of the two identically named methods are identical.

I don’t agree with this, or the argument here #13774 (review):

Imagine that Chainstate::ResetBlockFailureFlags returns bool in the future. This prevents the code from compiling, whereas removing the return would happily compile and hide a potential bug, no?

If the inner addUnchecked is modified to change its return type, then it’s perfectly fine for the outer addUnchecked to discard that return value. If the intent really isn’t for the return value to be discarded, then the bug is not adding a [[nodiscard]] attribute.
promag commented at 3:18 pm on April 7, 2021: member

I think the current code is fine, why the special treatment for type void? But the change is correct and I have no strong reason to ack/nack.

If the inner addUnchecked is modified to change its return type, then it’s perfectly fine for the outer addUnchecked to discard that return value. If the intent really isn’t for the return value to be discarded, then the bug is not adding a [[nodiscard]] attribute.

Agree.
hebasto commented at 11:02 am on May 23, 2021: member

@jnewbery

… then the bug is not adding a [[nodiscard]] attribute.

Is there a way to get a compiler to warn us about this bug? If not, then the current code looks less fragile and safer.
jnewbery commented at 4:18 pm on June 3, 2021: member

If not, then the current code looks less fragile and safer.

I can’t see how the proposed change in unsafe or fragile. Please can you explain how the change is unsafe?
kiminuo closed this on Sep 30, 2021
DrahtBot locked this on Oct 30, 2022

Contributors
kiminuo theStack Aristarkhov curtcurt871 MarcoFalke jnewbery promag hebasto

Labels
Refactoring Mempool