mate-desktop / libmateweather

Library to access weather information from online services
https://mate-desktop.org
GNU Lesser General Public License v2.1
21 stars 24 forks source link

test_sun_moon.c: remove unused variable #56

Closed lukefromdc closed 6 years ago

lukefromdc commented 6 years ago

Found this in terminal text while building after the last PR was merged

lukefromdc commented 6 years ago

Where is it used? it generated a build warning about not being used, and a quick test of displaying weather showed it working same as before. If this is in fact used somewhere and the compiler is wrong, feel free to close this PR and forget about it. I don't use the weather features in daily use and could miss an intermittant issue.

All I was trying to do was clean up something the compiler flagged as unused. Maybe its used in some other file and the compiler is missing it? Remember that I am not university trained in this stuff, I am self-taught

muktupavels commented 6 years ago

Variable is unused, but I would suggest to look what that function does and whether it affects remaining code... sunrise and sunset is calculated and there are printf's that uses this info.

lukefromdc commented 6 years ago

Looking for all instances in all files of calc_sun_time shows the function itself defined in weather-sun.c, listed in weather-priv.h, and used exactly twice: once to load that unused variable that having been set is not used, and once in calc_sun also in weather-sun.c . In that same file weather_info_next_sun_event uses calc_sun, so it looks like the removed variable is one of two uses of the function, so the function itself cannot also be removed but it need not be invoked to load an unused variable. weather_info_next_sun_event loads its result into the #WeatherInfo structure and in general these files are well commented.

vkareh commented 6 years ago

I'm not on my computer, but a quick look at the code shows that that function might be use to populate some properties of info, very likely info.sunrise and info.sunset used below.

@lukefromdc - when you run it before or after the patch, does the printed output change for those variables?

So it seems that although the bsun variable is not used, the function itself is, but I would need to look deeper in the code just to be sure

muktupavels commented 6 years ago

What are you trying to say? Without calc_sun_time you can as well remove two printf lines, because sunriseValid and sunsetValid now always will be false...

lukefromdc commented 6 years ago

I said the function can't be removed, it is just not used in the case of loading an unused variable. It is used elsehere, in calc_sun in the same file.

How would I get printout of those functions? I am only testing the library by using it in mate-panel's calendar weather functiions, which seem to work same as before. It's daytime here, sun it out, and that is displayed

muktupavels commented 6 years ago

./test_sun_moon?

lukefromdc commented 6 years ago

OK, I see the "test_sun_moon" binary in the built source directory-and indeed I get sunrise: (invalid) sunset: (invalid)

in Master I get

sunrise: Sun Apr 29 01:53:00 2018 sunset: Sun Apr 29 13:59:48 2018

To be exact, the function CALL that was removed is one of two calls of that function, the other being in the calc_sun function that seems to be a wrapper around it and in turn is used at least in weather_info_next_sun_event . The question is this: how did the variable loaded from the other function call get flagged as unused, when there is clearly a difference in the results when it exists and when it does not exist. Is this a compiler issue in gcc7? Closing the PR

lukefromdc commented 6 years ago

Remaining issue is the warning:

test_sun_moon.c: In function ‘main’:
test_sun_moon.c:21:21: warning: variable ‘bsun’ set but not used [-Wunused-but-set-variable]
     gboolean        bsun, bmoon;
                     ^~~~

This warning is flagging as unused a variable that clearly is doing something, as taking it out changes the results

lukefromdc commented 6 years ago

./test_sun_moon is indeed what showed the "unused" variable as doing something.

muktupavels commented 6 years ago

Compiler is right, bsun is unused. You can remove that variable, but function call should be there...

Change bsun = calc_sun_time(&info, info.update); to calc_sun_time(&info, info.update); and warning will be fixed.

lukefromdc commented 6 years ago

That works, will force-push the branch and reopen the PR

lukefromdc commented 6 years ago

new PR at https://github.com/mate-desktop/libmateweather/pull/57 as it was not possible to reopen the same PR after a force-push