Uploaded patch set 1.

Patch Set 1: Verified-1

Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=20307-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01

Patch Set 1: Code-Review-2

No good. I was wondering if all the "const" were necessary. As it turns out, some of them are absolutely not even compile-able... ;-)

C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\INCLUDE\xutility(2633) : error C389 2: '_Dest' : you cannot assign to a variable that is const

At least for the fill one.

Uploaded patch set 2.

Patch Set 1:

David, Sorry. I uploaded the wrong patch. I fixed this before uploading, but my commit --amend had a typo.

Patch Set 2: -Code-Review

No worries... However, end of the day now, and I don't have time to try it now. Maybe I can get to it tomorrow.....

Patch Set 2: Verified+1

Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=20307-2&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01

Patch Set 2: Code-Review-1

This test shall not be done. The for loops embedded in the STL algorithms are already doing it.

We should first test with just this->m_Data. If VC++ is OK with this in debug mode, then we have our correction. If not, then we should fall back to plain loops.

Patch Set 2:

You cannot call std::fill_n with a nullptr for Debug VS 2013.

The implementation of fill_n looks like this:

template<class _OutIt,
class _Diff,
class _Ty> inline
_OutIt fill_n(_OutIt _Dest, _Diff _Count, const _Ty& _Val)
{ // copy _Val _Count times through [_Dest, ...)
_DEBUG_POINTER(_Dest);
return (_Fill_n(_Dest, _Count, _Val,
_Is_checked(_Dest)));
}

The _DEBUG_POINTER line is called unconditionally, and will throw this assert if the pointer is nullptr.

Patch Set 2:

Why are you against doing a nullptr check or a check on the number of elements prior to calling fill_n?

If you are calling fill_n so frequently that you're concerned about adding a micro-smidge of time to each call, perhaps you're calling it too frequently...

Patch Set 2:

If you want the fastest code possible, without doing the appropriate nullptr safety checks, then you must guarantee that the entire test suite never tries to execute a test case with a nullptr. So another solution would be simply NOT to execute tests that result in these calls for Debug Microsoft builds.

It is my contention that **if** there is a performance problem with code after simply adding "if (this->m_Data)" checks before fill and copy calls, that the performance problem is with the **callers** of these things.

Fill and copy effectively perform N assignment operations. If you cannot afford a single nullptr check when you are doing N assignment operations then you should write special case code to avoid the safety checks.

The rest of us would like to be alerted ASAP when we accidentally try to "fill" or "copy into" an empty container.

Patch Set 2:

David, I agree with ou completely.

I was trying to get this ball rolling. Will you please take over this patch set and change as necessary?

Patch Set 2:

You cannot call std::fill_n with a nullptr for Debug VS 2013.The implementation of fill_n looks like this:template<class _OutIt, class _Diff, class _Ty> inline _OutIt fill_n(_OutIt _Dest, _Diff _Count, const _Ty& _Val) { // copy _Val _Count times through [_Dest, ...) _DEBUG_POINTER(_Dest); return (_Fill_n(_Dest, _Count, _Val, _Is_checked(_Dest))); }The _DEBUG_POINTER line is called unconditionally, and will throw this assert if the pointer is nullptr.

That's odd. I don't read such a precondition in the standard. Is it the same with std::copy()?

> Why are you against doing a nullptr check or a check on the number
> of elements prior to calling fill_n?
>
> If you are calling fill_n so frequently that you're concerned about
> adding a micro-smidge of time to each call, perhaps you're calling
> it too frequently...

I totally agree that such algorithms are often called to many times. and yet, when they are, they are called on pixels. This means they are sometimes called billion times. We don't need to add a test that we could easily avoid and still stay robust.

What we need is a fill_n implementation that have as a precondition: "ptr!=nullptr || size==0", and not "ptr!=nullptr".

If you want the fastest code possible, without doing the appropriate nullptr safety checks, then you must guarantee that the entire test suite never tries to execute a test case with a nullptr. So another solution would be simply NOT to execute tests that result in these calls for Debug Microsoft builds.

We simply need algorithms with less restrictive preconditions. For instance

// untested code
template <typename OutIt, typename Size, typename T>
OutIt itk::fill_n(OutIt start, Size size, T const& value) {
assert(start ||size == 0); // the test is not correct, just to give an idea of what it should look like
for ( ; size != 0 ; --size, ++start)
*start = value;
return start;
}

> It is my contention that **if** there is a performance problem with
> code after simply adding "if (this->m_Data)" checks before fill and
> copy calls, that the performance problem is with the **callers** of
> these things.

Somehow I agree. Unfortunately, we tend to call these functions too many times, i.e. once per pixel if not more. We have a lot of pixels.

Here it's easy to have a correct code (that'll pass tests with VC++ in Debug Mode) without this redundant branching. Why pay for this branching?

> Fill and copy effectively perform N assignment operations. If you
> cannot afford a single nullptr check when you are doing N
> assignment operations then you should write special case code to
> avoid the safety checks.

N is often small (< 16)

The rest of us would like to be alerted ASAP when we accidentally try to "fill" or "copy into" an empty container.

We don't need to here. However, if you consider that filling or copying an empty vector is a programming error, then we should document it and formalize the related precondition (assertion, or C++17 [[expect: size != 0]]).

Abandoned

This was taken up under a different patch set by Luc Hermitte