gil_concept.hpp triggers self-assignment warnings

[gromer@…]

boost/gil/gil_concept.hpp contains several instances of assigning a variable to itself. In context, this is perfectly safe (since the code should never actually be executed), but it nonetheless triggers gcc's -Wself-assign, cluttering the build output for those who have that flag set, and breaking the build for those who also set -Werror.

I am unable to suggest a fix for the instance in PointNDConcept, because the line in question ("point[0] = point[0];", line 276) appears to be checking properties that are not part of the concept as documented; I can find no documentation of the fact that models of PointNDConcept are expected to have an operator[], much less what the parameter and return types of that operator are.

The attached patch contains trivial fixes for the other instances, which I believe have no impact on correctness or performance.

[gromer@…]

Partial fix for the bug

[gromer@…]

Please note that the attached patch does not completely resolve the issue; as noted in the summary I do not know what the correct resolution is for the self-assignment in PointNDConcept, because either the code is incorrect or the documentation is incomplete, and I don't know which.

[Dean Michael Berris]

Any updates on this? Can we remove the self-assignment check in the concept instead completely? There are other ways to test for assignable which doesn't involve self-assignment.

[Dean Michael Berris]

Replying to gromer@…:

Please note that the attached patch does not completely resolve the issue; as noted in the summary I do not know what the correct resolution is for the self-assignment in PointNDConcept, because either the code is incorrect or the documentation is incomplete, and I don't know which.

Okay, I had a look at this and it seems that PointNDConcept<T> does imply that it should be Regular<T>. This means it supports all the regular type requirements.

Because this is a concept check it means whatever is in the concept definition determines the requirements. I'm inclined to think that merely switching the assignment to:

value_type v = p[0];
p[0] = v;

Should fix the issue properly. I'll attach a new patch that should fix this.

[Dean Michael Berris]

Also, as an addendum, the concept does require that the type T has a subscript operator, as this deals with axes of a Point. Subsequent Point*DConcept tests that depend on PointNDConcept where subscripting is required. In my patch I have updated the concept documentation to reflect this.

[Dean Michael Berris]

Updated documentation and self-assignments.

[Dean Michael Berris]

Apologies for the delay. Marshall, would you mind having a look at the updated patch?

[Marshall Clow]

I spoke to Lubomir, and he said:

"PointNDConcept is an N-dimensional point where each dimension may have a different type. Therefore it cannot have operator[]. So the documentation is correct and the line point[0] = point[0] should be removed."

[Marshall Clow]

Revised GIL self-assignment patch

[Dean Michael Berris]

Okay, this looks and sounds good to me! Can we commit this change and perhaps get this to a release branch too? :)

[Marshall Clow]

(In [82092]) Apply patch for Lubomir; Refs #4919

[Dean Michael Berris]

Can we get an update on when [82092] will make it into a release? 1.54.0 still doesn't have this IIUC.

[Mateusz Loskot]

The patch was released in GIL with Boost 1.68

​https://github.com/boostorg/gil/blob/boost-1.67.0/include/boost/gil/gil_concept.hpp

vs

​https://github.com/boostorg/gil/blob/boost-1.68.0/include/boost/gil/gil_concept.hpp

github ref: ​https://github.com/boostorg/gil/projects/4#card-12439478