Re: [PATCH] Planner: ascent and descent rates should be rounded rather than truncated
On Mon, Mar 13, 2017 at 12:35 AM, Rick Walsh wrote:
>
> void PlannerSettingsWidget::setAscRate75(int rate)
> {
> -
> SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate *
> UNIT_FACTOR);
> +
> SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate *
> UNIT_FACTOR + 0.5);
> }
These should all definitely use "lrint()" rather than " + 0.5"
together with the integer truncation.
In practice it's the same for positive values, of course, but it's the
right thing to do.
.. and this all shows that we should also enable the
-Wfloat-conversion warning for our C++ code too. Of course, it's
possible that that fails miserably for some random reason (eg "tons of
warnings from Qt header files" or whatever).
Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Planner: ascent and descent rates should be rounded rather than truncated
On Mon, Mar 13, 2017 at 09:07:31AM -0700, Linus Torvalds wrote:
> On Mon, Mar 13, 2017 at 12:35 AM, Rick Walsh wrote:
> >
> > void PlannerSettingsWidget::setAscRate75(int rate)
> > {
> > -
> > SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate *
> > UNIT_FACTOR);
> > +
> > SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate *
> > UNIT_FACTOR + 0.5);
> > }
>
> These should all definitely use "lrint()" rather than " + 0.5"
> together with the integer truncation.
Agreed since UNIT_FACTOR is a floating point number.
Can you send a patch / pull request that does that instead?
Thanks
/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Planner: ascent and descent rates should be rounded rather than truncated
Ho!
I didn't realized I used the -Wfloat-conversion on C code only and not C++
when I did my previous round of float/double to int patch!
Looking into that now. There is a bunch of new warnings adding the flag but
seems (at first look) to be managable.
Working on the patch right now...
2017-03-14 0:11 GMT+07:00 Dirk Hohndel :
> On Mon, Mar 13, 2017 at 09:07:31AM -0700, Linus Torvalds wrote:
> > On Mon, Mar 13, 2017 at 12:35 AM, Rick Walsh
> wrote:
> > >
> > > void PlannerSettingsWidget::setAscRate75(int rate)
> > > {
> > > -
> > > SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate
> * UNIT_FACTOR);
> > > +
> > > SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate
> * UNIT_FACTOR + 0.5);
> > > }
> >
> > These should all definitely use "lrint()" rather than " + 0.5"
> > together with the integer truncation.
>
> Agreed since UNIT_FACTOR is a floating point number.
>
> Can you send a patch / pull request that does that instead?
>
> Thanks
>
> /D
> ___
> subsurface mailing list
> subsurface@subsurface-divelog.org
> http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
>
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Planner: ascent and descent rates should be rounded rather than truncated
On Mon, Mar 13, 2017 at 11:29 AM, Jérémie Guichard wrote: > > Looking into that now. There is a bunch of new warnings adding the flag but > seems (at first look) to be managable. Yeah, I was afraid there would be some infrastructure stuff, but it all looks like our source. > Working on the patch right now... Thanks. Linus ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Planner: ascent and descent rates should be rounded rather than truncated
Pull request ready :) https://github.com/Subsurface-divelog/subsurface/pull/254 2017-03-14 2:00 GMT+07:00 Linus Torvalds : > On Mon, Mar 13, 2017 at 11:29 AM, Jérémie Guichard > wrote: > > > > Looking into that now. There is a bunch of new warnings adding the flag > but > > seems (at first look) to be managable. > > Yeah, I was afraid there would be some infrastructure stuff, but it > all looks like our source. > > > Working on the patch right now... > > Thanks. > > Linus > ___ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
Re: [PATCH] Planner: ascent and descent rates should be rounded rather than truncated
On 14 Mar 2017 5:29 AM, "Jérémie Guichard" wrote:
Ho!
I didn't realized I used the -Wfloat-conversion on C code only and not C++
when I did my previous round of float/double to int patch!
Looking into that now. There is a bunch of new warnings adding the flag but
seems (at first look) to be managable.
Working on the patch right now...
Excellent!
2017-03-14 0:11 GMT+07:00 Dirk Hohndel :
> On Mon, Mar 13, 2017 at 09:07:31AM -0700, Linus Torvalds wrote:
> > On Mon, Mar 13, 2017 at 12:35 AM, Rick Walsh
> wrote:
> > >
> > > void PlannerSettingsWidget::setAscRate75(int rate)
> > > {
> > > -
> > > SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate
> * UNIT_FACTOR);
> > > +
> > > SettingsObjectWrapper::instance()->planner_settings->setAscrate75(rate
> * UNIT_FACTOR + 0.5);
> > > }
> >
> > These should all definitely use "lrint()" rather than " + 0.5"
> > together with the integer truncation.
>
And that's what happens when a civil engineer tries to teach himself C.
Thanks for showing me the correct way.
> Agreed since UNIT_FACTOR is a floating point number.
>
> Can you send a patch / pull request that does that instead?
>
I would when I found time, which is hard for me at the moment even if it is
just making a simple change to a few lines. Luckily Jérémie has beaten me
to it.
Cheers,
Rick
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
