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

[David Cole]

Thanks for your work so far on all this. I really appreciate the effort,
and it has already paid off: the test passed yesterday and today on my
Debug VS dashboard.

I will give a try to your latest patch set with the added assert calls
later today.


Thanks,
David


On Tuesday, October 27, 2015, Luc Hermitte <luc.hermitte at c-s.fr> wrote:

> Le 27/10/2015 17:04, David Cole a écrit :
> > I do prefer a test.
>
> The alternatives are not just test+std::copy VS no-test+std::copy. But
> test and std::copy VS manual loop where the test is pointless as it's
> already part of for() invariant.
>
> > 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.
>
> It's the "something bad could happen if you don't check" that makes all
> the difference here and that explain my reluctance.
>
> i.e. nothing bad should happen with usual/naive implementations of
> std::copy with std::copy(p,p, nullptr) -- if memcpy is triggered, I must
> admit my ignorance, and as others have said, it would be very
> surprising:
>
> http://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0
>
> [On a side note, it's up to the client code to check for nullity if
> nullity is rejected by the contract. Within the service code, we could
> wait for C++17, or more likely use assertions.]
>
> > 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.
>
> That's my problem. I see it exactly as defensive programming. Unlike
> (narrow) Design By Contract, the (wide) defensive programming approach
> adds tests that make no sense (in context were the contract is
> respected), and complicate the nominal code.
>
>
> > 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.
>
> I completely agree they are worth it. This time, the test (on copy -- as
> I've workaround the one on fill) is as you said : too aggressive.
>
>
> > 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.
>
> Yes please, I'd like to.
>
> Here is a patched (I hope -- memset and memcpy may trigger assertions, I
> don't know) version of the code.
>
> http://review.source.kitware.com/#/c/20313/ (I didn't know if, and how,
> I could take over the previous patch set on the subject. Sorry for the
> inconvenience)
>
> On the way, I've added missing documentation regarding invariants,
> preconditions and postconditions, plus the related assertions. This is a
> first draft. It's likely to contains English errors. I hope, however I
> did not assert incorrect contracts in the documentation. I'll read it
> again more carefully later.
>
> I've also added code to test operations on empty VLV.
>
>
> Thank you.
>
> --
> Luc Hermitte
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/insight-developers/attachments/20151029/82865aab/attachment.html>