Modify

Ticket #4919 (new Patches)

Opened 3 years ago

Last modified 8 months ago

gil_concept.hpp triggers self-assignment warnings

Reported by: gromer@… Owned by: marshall
Milestone: To Be Determined Component: GIL
Version: Boost 1.45.0 Severity: Problem
Keywords: Cc:

Description

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.

Attachments

selfassign.patch Download (770 bytes) - added by gromer@… 3 years ago.
Partial fix for the bug
boost-gil-self-assignment-fix.patch Download (2.3 KB) - added by mikhailberis 17 months ago.
Updated documentation and self-assignments.
gil.patch Download (2.0 KB) - added by marshall 16 months ago.
Revised GIL self-assignment patch

Change History

Changed 3 years ago by gromer@…

Partial fix for the bug

comment:1 Changed 3 years ago by Vicente Botet <vicente.botet@…>

  • Type changed from Bugs to Patches

comment:2 follow-up: ↓ 4 Changed 3 years ago by 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.

comment:3 Changed 18 months ago by mikhailberis

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.

comment:4 in reply to: ↑ 2 Changed 18 months ago by mikhailberis

  • Owner changed from hljin to marshall

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.

comment:5 Changed 18 months ago by mikhailberis

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.

Changed 17 months ago by mikhailberis

Updated documentation and self-assignments.

comment:6 Changed 17 months ago by mikhailberis

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

comment:7 Changed 16 months ago by marshall

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."

Changed 16 months ago by marshall

Revised GIL self-assignment patch

comment:8 Changed 16 months ago by mikhailberis

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

comment:9 Changed 16 months ago by marshall

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

comment:10 Changed 8 months ago by mikhailberis

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

View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as new
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.