[ITK-dev] Fwd: Change in ITK[master]: BUG: Make valid calling with m_NumElements == 0

[David Cole]

I do prefer a test. This is C++. You should always check for nullptr
if it is possible to get nullptr, and something bad could happen if
you don't check. If you could prove to me that there is never a
nullptr in this context, I would not care. But there is. The failing
test proves it.

Rather than thinking of this as "just to workaround the implementation
of a function in Debug mode and with a single family of compiler,"
perhaps you should instead think of it as defensive programming.

The fact is, these debug checks from Microsoft have helped and
assisted me over the years FAR MORE than they have hurt. They help to
detect problems early on, as soon as incorrect code is introduced and
run. Yes, they're painful, and yes, sometimes slightly too aggressive.
But they're worth it.

Let me know if you would like me to build/run your patch set after you
think you have it to the point where the Debug MS build should run
without raising these assert dialogs.


Thanks,
David C.





On Tue, Oct 27, 2015 at 8:11 AM, Luc Hermitte <luc.hermitte at c-s.fr> wrote:
> Hi,
>
> Le 26/10/2015 16:41, David Cole via Insight-developers a écrit :
>> I would really really really like to see some evidence from you that
>> adding nullptr checks is materially harmful to performance in some
>> real world fill/copy scenario before agreeing to any patch which does
>> not include nullptr checks.
>
> Here are some micro-benchmarks (done with google-benchmark framework):
>
> https://gist.github.com/LucHermitte/7f1b8d5dd61c861e6ef9
>
> Adding the test around std::copy in the assignment operator doesn't
> change anything. Nor the manual implementation.
> (These are not real world scenarios, however the result would be the
> same : no observable difference).
>
> I still find that adding a redundant test, just to workaround the
> implementation of a function in Debug mode and with a single family of
> compiler, is a clumsy solution.
>
> What the manual implementation does is as good as the defensive one.
> However, we could loose potential optimizations (like memcpy being used
> with POD types) when the size of the vector is big enough if the STL
> implementation tries to do it.
>
> BTW, memcpy(0,0,x) is officially an UB. And the more I read, the less
> I'm convinced that func(nullptr, null_size) is officially defined
> behaviour.
>
> So, if you prefer a test, let's go for a test -- which should be
> required only in the assignment operator.
>
> Regards,
>
> --
> Luc Hermitte