mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
195 stars 86 forks source link

WIP Fix some warnings about float conversion (-Wfloat-conversion) #633

Closed rbuj closed 4 years ago

rbuj commented 4 years ago

Test: marco.make.log (master), marco/make.log (PR)

$ CFLAGS="-O3 -fomit-frame-pointer -malign-double -fstrict-aliasing -ffast-math -Wconversion -Wunused-macros -Wunused-parameter" ./autogen.sh --prefix=/usr --enable-compile-warnings=maximum && make &> make.log && sudo make install

Comparing make logs

$ diff <(./warnings-file.sh marco.make.log) <(./warnings-file.sh marco/make.log)
3c3
< -Wconversion 978
---
> -Wconversion 951
5c5
< -Wfloat-conversion 97
---
> -Wfloat-conversion 18

warnings-file.sh

FILE=$1
for warning in $(grep warning $FILE | grep -Po '\[\-W.*\]' | sed 's/[][]//g' | sort -u)
do
 type=${warning#"-W"}
 num=$(echo "grep $type $FILE | wc -l" | sh)
 echo "$warning $num"
done

Testing scaled surfaces: save scaled surfaces to png files "/home/robert/SC_TIME " + TIMESTAMP + ".png"

benchmark: master vs PR (there is no significant difference)

$ marco-theme-viewer BlueMenta


### master

![Screenshot at 2020-08-09 21-53-07](https://user-images.githubusercontent.com/10171411/89740719-8e3ea180-da8b-11ea-9e9d-45fbeec80c6c.png)

### PR

![Screenshot at 2020-08-09 21-54-37](https://user-images.githubusercontent.com/10171411/89740723-9696dc80-da8b-11ea-9ff9-0499c86a80ce.png)
raveit65 commented 4 years ago

@rbuj We have a conflict...

rbuj commented 4 years ago

@raveit65 done. I fixed the merge conflict.

rbuj commented 4 years ago

@muktupavels We talked about this some time ago in another PR #476, I thought you participated in the conversation, but I was wrong. All I'm telling you is from memory:

At that moment of writing the other PR, I was focused on the performance after seeing an unusual comment on the code.

Doing it that way introduced another warning, From memory, do not convert directly the returned value of a function i.e. do the conversion using a variable, because of mismatch on datatypes, since round returned double and it was stored on a non double variable.

In other parts of the code I think I remember that they use the same principle, rounding up or down depending on whether the value is closer to one of the ends, i.e. using the round operation. I think it's done using macros to avoid the round function of math.h. What’s more, I think there is also a scaling macro, which cannot be reused by the data type, i.e. wide or sign, which is not much different from what this PR proposes.

I will see where what I told you comes from... perhaps from other projects.

./atril/backend/dvi/mdvi-lib/fontmap.c:#define DROUND(x)    ((x) >= 0 ? floor((x) + 0.5) : ceil((x) - 0.5))
./atril/backend/dvi/mdvi-lib/common.h:#define SFROUND(x)    (int)((x) >= 0 ? floor((x) + 0.5) : ceil((x) + 0.5))

Edit: I saw it on mate-desktop/libmate-desktop/mate-colorsel.c :)

#define SCALE(i) (i / 65535.)
#define UNSCALE(d) ((guint16)(d * 65535 + 0.5))
muktupavels commented 4 years ago

You asked for review and you got one...

rbuj commented 4 years ago

I'm very grateful to you, I will work to include the improvements you have asked me.

rbuj commented 4 years ago

@muktupavels I added the text below on the commit body:

- round on unscale (scale_factor * max_value + 0.5)
- do not require to invert float: cairo_scale (cr, 1/ratio, 1/ratio)
- use specific operations based on datatype: sqrtf for float
- no mixing single precision with double precision numbers
muktupavels commented 4 years ago

@rbuj Now imagine that it was not your work but was done by somebody else. You have found that this commit has introduced regression. Does that message gives anything useful to understand why change was made? Did it fix something?

Adding 0.5 can easily introduce regression because original author might have relied on truncating. Obviously you will not notice any regression in solid_picture()... Getting 32768 instead of 32767 for 0.5 changes nothing. If you would have looked how that function is used than you would notice that it is used only in 3 places, one is only if debugging redraws. You could use unsigned short for r, g, b parameters...

Also I still think this should be done in more smaller commits.

rbuj commented 4 years ago

Ok, I close this MR and I'll do small MRs, the 1st #638