-
Notifications
You must be signed in to change notification settings - Fork 12
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
Don't default initialize NumberArray data #34
Conversation
The difference in idioms is part of the standard:
How much of a performance boost do you get by getting rid of the redundant zeroing? Can you see any way to get the same behavior out of a user-defined type? I haven't yet (although Initialization in C++ is bonkers so there may just be something I haven't considered yet). I'd love to let users get rid of redundant initialization, I just don't see how to do so without creating nasty situations for generic algorithm writers where a standard |
Ok yea sure for built-ins I understand this. I guess my statement was that I don't assume
I haven't quantified it at this point. I'll try to get back to you later today. It may not be a lot.
No I don't 😞 |
Ok, I may still be confused, but: Can we just declare the constructor "= default" (or let it be implicitly defined as default)? From what I'm reading on StackOverflow and in explanations of the standard regarding default versus zero initialization, it looks like that will do what we want: zero initialize in idioms where a builtin would zero initialize, but leave uninitialized in idioms where a builtin would be left uninitialized. |
#include "metaphysicl/numberarray.h"
#include <iostream>
using namespace MetaPhysicL;
int main()
{
NumberArray<2, double> n1;
NumberArray<2, double> n2 = NumberArray<2, double>();
std::cout << n1[0] << std::endl;
std::cout << n2[0] << std::endl;
} So we would want to give the default constructor for |
In the ALE problem, the profiling looks great with the defaulted constructors |
Fantastic! I know that was based on a close reading of the standard, but part of me still can't believe that worked.
Definitely. And NumberVector... and I think that's it, and #13 doesn't suggest anything I forgot... I guess the compile-time sparse classes might have the same concern and same resolution, but don't worry about those for now. How do we properly test this, though? It feels like the sort of thing that would be really easy to regress down the line. We can have a unit test that uses the |
That seems like it would be the most robust test |
Thanks! |
@roystgnr can you bootstrap this? Or if you want to give me push rights, I could also do it. |
Thanks; you're long overdue for push rights at this point. |
I'd say make a "pr34_bootstrapped" branch this time, but if you think this merits a release (or if you just want to benefit from the release-oriented bootstrap+tag+etc script we have) then we could do that instead. |
Don't define the build date twice
I know we have #13, and I was the one to make the change for
NumberArray
in #11, but I do think we should allow intentional uninitialized data. I see a lot ofbzero
show up in profiling. I don't think that a user who callsNumberArray a;
andNumberArray a = NumberArray()
should expect different behavior. I re-read this discussion quite a few times this morning and had @jwpeterson look at it as well; I guess I'm just not familiar with the second idiom. We both look at the first and second expressions in that discussion and expect to observe the exact same behavior between the two.