Improved efficiency in COutPoint constructors

[mm-s]

memset(0) executed twice, first in uint256_t default constructor and then in SetNull.

Remake of #10277

[kallewoof]

Can you leave the names as hashIn and nIn so we don't get hash(hash), n(n)?

[dcousens]

utACK with @kallewoof 's suggestion

[dcousens]

Inconsistent spacing, n((uint32_t) -1) is fine

[mm-s]

Fixed both suggestions. @dcousens & @kallewoof,
although I'd like to state my opinion on these reviews:
1.- "In" suffix does not add any value and makes the sentence more verbose and, in my opinion, less readable.
2.- Consistency rules often carry problems related to lack of diversity.

Thanks

[dcousens]

utACK 4fbae77

[kallewoof]

I would argue that foo(foo) is less ambiguous despite semantic equivalence. It's also the standard that's been adhered to in the code thus far so it'd require some amount of merit to break.

[mm-s]

it is just that c(p):p(p) is a better abstraction than c(pIn):p(pIn) since In does not add any meaning to the code, apart from remarking that it is an input parameter which is obvious.

[kallewoof]

That's all fine and dandy until you start doing things like

class foo {
    int x;
    foo(int x) { x = x * 48; } 
};

which, crazy as it may seem, is not very far away from your suggestion.

[JeremyRubin]

Weak Concept Ack.

Most likely this gets inlined and optimized out so this shouldn't impact the generated code?

If that isn't the case, I have a slight preference to make this happen by exposing an uninitialized constructor for uin256_t and then calling SetNull.

[mm-s]

@JeremyRubin Not easy decision: The default constructor is reseting to 0, adding a constructor that leaves the object uninitalized looks like having an ugly prototype; on the other hand changing the behavior by leaving the object uninitalized in the default constructor and adding another constructor accepting a uint8[] looks like it is the right design, but at this point it may confuse to whom is already assuming that the default constructor is reseting the memory, apart from checking every single call done in the codebase.

[JeremyRubin]

I'm not suggesting that it has to/should be the default constructor.

Also the main part is that this likely does not impact the actual code generated; you should try outputting optimized assembly for

#include <memory>
int main() {
    char x[32];
    memset((void*)*x, 0, 32);
}

and

#include <memory>
int main() {
    char x[32];
    memset((void*)*x, 0, 32);
    memset((void*)*x, 0, 32);
}

I didn't see a difference even at O1.

[sipa]

@JeremyRubin Also when the code is split over different modules?

[mm-s]

@JeremyRubin The compiler can resolve programmer's errors automatically, but it is preferably to have the high level code well designed before the optimizer checks ; with so many compiler vendors , so many flags and so many archs out there you can never say for sure the compiler will end up optimizing.
Also i commented on the possibility of not using the default constructor, and seemed to me ugly, which prototype would you use for a constructor that does not initialize?

[JeremyRubin]

@sipa I think the memset occurs in the header so it's my understanding it would be inlined.

edit: notice that memset is defined in a different header in my example.

@mm-s while this is true, your PR is not fixing an error it is an optimization, which introduces a dependency on the 0 initialization of uint256 that might introduce bugs later. Perhaps one easy way is:

class X {
    public:
        X(bool initialized=true) {
        }
};

[sipa]

utACK 4fbae77

I think this change is perfectly reasonable. There is a slight duplication between the SetNull function and the constructor, but it comes at almost no extra complexity. Please let's not introduce explicit non-initializing constructors.