Fixes the reported issue #371

[anyzelman]

Closes #371

[anyzelman]

Currently proposed C++11-style fix running CI. Suggest to run full unit test suite prior to merge. Issue is resolved as by the following manual test:

$ /tmp/grbinstall/bin/grbcxx test.cpp 
$ cat test.cpp 
#include <graphblas.hpp>

int main() {
	constexpr size_t n = 100;
	grb::Vector< double > mask( n ), y_comp( n );
	grb::RC ret = grb::set( y_comp, (double)3.14 );
	ret = ret ? ret : grb::set( mask, (double)1 );
	ret = ret ? ret : grb::eWiseApply< grb::descriptors::dense >(
		y_comp, mask, (double)0, grb::operators::right_assign< double >() );
	return ret;
}

$ ./a.out 
$ echo $?
0

[anyzelman]

Did a quick scan of the same issue in other places -- no such case. If CI and tests run, ready for merge.

[anyzelman]

@djelovina could you confirm this addresses your issue? I applied a C++11 style fix, the bumping to C++17 and use of if constexpr which would have prevented this issue from appearing also seems a lot of work and better first considered more deeply. Opened #373 to not forget and track it. Some more thoughts are tersely added there, and welcome to add more of course=)

[anyzelman]

CI OK. Unit tests (with LPF) are running

[anyzelman]

All unit tests OK; will merge

[anyzelman]

Actually no, I think the change in this MR is correct, but the code example, which was wrong in the above but now fixed (see below), should in fact trigger that assertion failure(!)

$ cat test.cpp 
#include <graphblas.hpp>

int main() {
	constexpr size_t n = 100;
	grb::Vector< double > mask( n ), y_comp( n );
	grb::RC ret = grb::set( y_comp, (double)3.14 );
	ret = ret ? ret : grb::set( mask, (double)1 );
	ret = ret ? ret : grb::eWiseApply< grb::descriptors::dense >(
		y_comp, (double)0, mask, grb::operators::left_assign_if< double >() );
	return ret;
}
$ /tmp/grbinstall/bin/grbcxx test.cpp
$ ./a.out 
$ echo $?
0

The reason this code should fail, is because even if y_comp is dense on input, under the combined semantics of that of eWiseApply (which semantically clears the output vector and then applies an operator on two dense inputs, thus having to produce a dense output also) and the left_assign_if operator (which may produce nothing on output when the if-value evaluates false), the call should indeed generate a dense output but the output element values for those entries where the mask evaluates false, are indeed undefined. It now indeed retain the old values -- but that's actually undefined behaviour. So in fact a static_assert is missing in the dense code path(!!!)

[anyzelman]

Now the behaviour is correct, same test.cpp as above:

$ /tmp/grbinstall/bin/grbcxx test.cpp 
In file included from /tmp/grbinstall/include/graphblas/blas1.hpp:29,
                 from /tmp/grbinstall/include/graphblas.hpp:538,
                 from test.cpp:1:
/tmp/grbinstall/include/graphblas/reference/blas1.hpp: In instantiation of ‘grb::RC grb::internal::dense_apply_generic(OutputType*, const InputType1*, const grb::internal::Coordinates<grb::reference>*, const InputType2*, const grb::internal::Coordinates<grb::reference>*, const OP&, size_t) [with bool left_scalar = true; bool right_scalar = false; bool left_sparse = false; bool right_sparse = false; unsigned int descr = 16; OP = grb::operators::left_assign_if<double>; OutputType = double; InputType1 = double; InputType2 = double; size_t = long unsigned int]’:
/tmp/grbinstall/include/graphblas/reference/blas1.hpp:5069:76:   required from ‘grb::RC grb::eWiseApply(grb::Vector<DataType, grb::reference, Coords>&, InputType1, const grb::Vector<InputType, grb::reference, Coords>&, const OP&, const grb::Phase&, const typename std::enable_if<((((! grb::is_object<InputType1>::value) && (! grb::is_object<InputType2>::value)) && (! grb::is_object<OutputType>::value)) && grb::is_operator<OP>::value), void>::type*) [with unsigned int descr = 16; OP = grb::operators::left_assign_if<double>; OutputType = double; InputType1 = double; InputType2 = double; Coords = grb::internal::Coordinates<grb::reference>; typename std::enable_if<((((! grb::is_object<InputType1>::value) && (! grb::is_object<InputType2>::value)) && (! grb::is_object<OutputType>::value)) && grb::is_operator<OP>::value), void>::type = void]’
test.cpp:8:62:   required from here
/tmp/grbinstall/include/graphblas/reference/blas1.hpp:2980:33: error: static assertion failed: Warning: you may be generating an output vector with uninitialised values. (Define the GRB_NO_NOOP_CHECKS macro to disable this check.)

 2980 |                                 !internal::maybe_noop< OP >::value,
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/grbinstall/include/graphblas/reference/blas1.hpp:2980:33: note: ‘!(bool)grb::internal::maybe_noop<grb::operators::left_assign_if<double> >::value’ evaluates to false

I also added a warning about this to the operators for which this might happen -- actually a warning was already there, but I just made it more explicit with this MR.

I am re-running the CI and unit tests. Meanwhile, @djelovina could you double-check what I wrote above here is correct and then also review the other changes? Many thanks!