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

[Luc Hermitte]

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