[fixed] [sketcher] new dataloss warning

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

[fixed] [sketcher] new dataloss warning

Post by uwestoehr »

Compiling current master, i get this compiler warning:

Code: Select all

D:\FreeCADGit\src\Mod\Sketcher\App\planegcs\Geo.cpp(732,35): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
@jnxd, can you please have a look?
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: [sketcher] new dataloss warning

Post by uwestoehr »

For now I added a type cast: git commit e6c53d86
marioalexis
Posts: 124
Joined: Wed Jun 19, 2019 7:44 pm

Re: [sketcher] new dataloss warning

Post by marioalexis »

The use of the parameters k and i as size_t seems unnecessary. We can change it to int and change the System::addConstraintInternalAlignmentKnotPoint function accordingly.
All variables involved are actually int.

Edit: actually a cast is needed there too.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: [sketcher] new dataloss warning

Post by jnxd »

marioalexis wrote: Sun Nov 20, 2022 1:06 am The use of the parameters k and i as size_t seems unnecessary. We can change it to int and change the System::addConstraintInternalAlignmentKnotPoint function accordingly.
All variables involved are actually int.

Edit: actually a cast is needed there too.
They are used as, but i and k are also indices within of control points and intervals respectively. They need to be at least unsigned int to ensure that negative values aren't passed. In the problem line, however, d is a sub-sequence of control points, and values outside the indices may be valid, just that the value of the function there is zero.

EDIT: In a private conversation, @abdullah mentioned that under some cases unsigned int is more appropriate than size_t. This particular change I might not have made completely well, or did it in a different PR (for tangents, which is currently blocked by some hangs).
size_t should not be used as a function parameter, unless a size is really intended. There is case in the code were a knot index is passed (originally as int). This index is not checked for boundaries and it is assumed to be an unsigned int, although it is actually an int. Then, when there are comparisons with unsigned types this generates compiler warnings.

Here it is best to actually change the function to unsigned int, to convey this fact, that the function expects an unsigned int. This also bring a potential signed/unsigned warning to the user code, if the wrong type is used.
My latest (or last) project: B-spline Construction Project.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [sketcher] new dataloss warning

Post by abdullah »

Yes. I thought this was fix, as we were bringing changes in this direction to several related PRs at the same time. Sorry for that.

We have a small discussion via PR on how these should be handled. My position is this:
1. It is ok to use "int" for a value range that in principle requires only "unsigned int", if the negative(s) is used to code a certain state (e.g., -1 for uninitialised), but then checking that the range passed is ok is necessary. If not, then we should use "unsigned it" instead. The latter forces the client code to provide the right range via compiler warning and clearly shows the intention that an unsigned value is intended.
2. size_t is highly misleading as input parameter, unless it is really a size. If it is not a size, "unsigned int" should be used instead. If any has slip through, then we should correct this situation. If it is going to be corrected anyway by BSpline point on object PR (which is my current review target), we can stay the change for a while.
3. For loop indices that are compared against a size_t, if we are coding a strong type, size_t may be used, as this is quite customary. So yes, the index it is not a size, but it is used within context. e.g. for(size_t i=0;i<v.size();i++)
Post Reply