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

[Luc Hermitte]

Le 29/10/2015 13:42, David Cole a écrit :
> Thanks for your work so far on all this. 

You're welcome.

> I really appreciate the effort,
> and it has already paid off: the test passed yesterday and today on my
> Debug VS dashboard.

Wonderful!

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

OK. Thank you.
--Luc

> 
> Thanks,
> David
> 
> 
> On Tuesday, October 27, 2015, Luc Hermitte <luc.hermitte at c-s.fr
> <mailto: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
>