ConstantIterator

[AntonReinhard]

• Add ConstantIterator class
• Yields the same value it was initialized with every time
• Add testing for the class

[SimeonEhrig]

Please add also a test case to integ/transform. I think int operator()(int i) { return 2*i;} is a good functor for certificate the result (sum = size * 2 * constant).

[SimeonEhrig]

@bernhardmgruber Can you please review it.

[bernhardmgruber]

If the whole point of this iterator is to just return the same value whenever dereferences, I see no need to store an index. Then all iterator functions that move the iterator become noops. When do you need the index of the iterator?

You can probably make all member functions of ConstantIterator constexpr and noexcept.

[bernhardmgruber]

alpaka moved to C++17 recently. So maybe vikunja should be on C++17 as well. In this case you would not need the #if defined __has_include.

Furthermore, it seems you require only the language feature and not the library feature. In this case you can just test for __cpp_impl_three_way_comparison which is predefined by the compiler and does not require including the <version> header. See: https://en.cppreference.com/w/cpp/feature_test

[SimeonEhrig]

After some discussion today, we decided to move to C++17. I want to provide a PR with the necessary changes this week.

[AntonReinhard]

Furthermore, it seems you require only the language feature and not the library feature. In this case you can just test for __cpp_impl_three_way_comparison

You're completely right, somehow I was under the impression that you always needed the header

[bernhardmgruber]

Such comments don't increase clarity of the code and only add noise. Try to avoid comments for the sake of comments, if they don't add any additional information the code is not already giving.

[AntonReinhard]

There is also the philosophy to have a doxygen comment for every function, even if it's fairly (or very) obvious what the function does. But I can also remove them if that's not the case for this project. Should I also remove them for the other straight forward arithmetic operators then?

[SimeonEhrig]

To be honest, there is no real guide for the doxygen yet. The student who started this project, did it in some way. I don't believe, that he got some feedback. I want to overwork this in the near future, see #65 . But Bernhard is right, the function is already defined by the concept iterator. Therefore the one line which explains the obvious is not necessary.

[bernhardmgruber]

When implementing comparison operators, you usually only implement operator< and operator==. The rest should be based on those:

Suggested change

	NODISCARD ALPAKA_FN_INLINE bool operator>(const ConstantIterator& other) const noexcept
	{
	if(v > other.v)
	return true;
	if(v < other.v)
	return false;
	return index > other.index;
	}
	NODISCARD ALPAKA_FN_INLINE bool operator>(const ConstantIterator& other) const noexcept
	{
	return other < *this;
	}

This way you can avoid accidentially only defining a partial order for your type. Or inconsistent comparions (e.g. a < b and a > b at the same time).

Applies to other operators below.

[AntonReinhard]

I know this can be done and reduces the probability to make mistakes, but I was trying to be explicit so I was sure the compiler wouldn't add unnecessary function calls. But I can change it anyways, if you think that this is unnecessary

[bernhardmgruber]

Compiler inlining heuristics are quite good! I am certain, the compiler would inline the delegation to operator< inside operator>.

[bernhardmgruber]

By making v const, your type is not movable (which might not be a problem), but also not swapable. I would avoid it, but I leave the choice to you.

[SimeonEhrig]

If the whole point of this iterator is to just return the same value whenever dereferences, I see no need to store an index. Then all iterator functions that move the iterator become noops. When do you need the index of the iterator?

You can probably make all member functions of ConstantIterator constexpr and noexcept.

The index is required for begin and end pointer:

ConstantIterator c_begin(10);
ConstantIterator c_end = c_begin + 4;

for(; c_begin < c_end; ++c_begin){
   std::cout << *c_begin << " "; // 10 10 10 10
}

Do you have a better idea?

[bernhardmgruber]

The index is required for begin and end pointer:

ConstantIterator c_begin(10);
ConstantIterator c_end = c_begin + 4;

for(; c_begin < c_end; ++c_begin){
   std::cout << *c_begin << " "; // 10 10 10 10
}

No, makes perfect sense now :D Without the index you would have an infinite range.

[SimeonEhrig]

@AntonReinhard Can you please rebase to the current master. I removed the C++ 14 support. New default is C++ 17.

[AntonReinhard]

Ah, I knew you had to include for the spaceship operator: https://godbolt.org/z/soa55sP73
Apparently gcc somehow handles it without, but clang needs it, because the return type of the defaulted operator<=> is the std::strong_ordered type, which is defined in

[SimeonEhrig]

Two points:

• You need to rebase on the this commit, otherwise the constant iterator will not work on CPU: dbc5d3a
• We cannot use the CI for benchmarking. All resources are virtualized and shared. Therefor the results are not reproduceable.

Can you execute your results on a local system or Taurus? If not, we have a Quadro V100 in our dev system. I can execute the benchmarks and send you the results.

[SimeonEhrig]

By the way, I was aware about the wrong results on the serial backend in the reduce benchmark. I think, it is a precession problem of float and the many summations on a single value. But I had no time yet to investigate this.