Divecomputer nickname handling..
Tomaz Canabrava
tcanabrava at kde.org
Thu Sep 17 15:18:02 PDT 2015
On Thu, Sep 17, 2015 at 7:03 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
> On Thu, Sep 17, 2015 at 02:38:20PM -0700, Linus Torvalds wrote:
> >
> > Ok, this is almost certainly entirely broken, because all I tested was
> > that it compiles for me. I'm not a huge fan of C++ ("really? Please don't
> > tell us again"),
>
> Wait, you DON'T like C++? Tell me more...
>
> > and I hate how I can't split up member functions into
> > static helper functions, so I got - once again - disgusted when I had to
> > add a "showchanges" member for no actual good reason just because I
> wanted
> > to split things up and make this more readable.
>
> You can. I found out that you can indeed.
>
> In a C++ file you can simply add a static function that is not part of the
> class. It doesn't have access to class members - so it behaves just like a
> standard C static function. I've use this somewhere already - forgot where
> now. But yes, you absolutely can do that.
>
but I think linus was refeering to having access to the members, there's a
way to do that, I don't think it is pretty, but it works:
*if* you need the access to the members:
move all members on the class to public access, then create static
functions on the .cpp file like this:
static void helper_function1(MyClass *self, var1, var2... ) {
...
}
it's the GObject approach to object orientation, and you both already
worked with gtk, will remember this. :)
Other thing is to do something like what dirk said, create a static
function that takes all parameters that you need for things to work and
work with them.
> > I also didn't want to change the prototype fro the "AddDC" member, but I
> > wanted to change the use of the variables from the "constant pass by
> > reference" to actual local variables that I can change, so I did that
> > insane dance with variables there too.
>
You don't need to do that dance. if you don't pass a QString via const
reference, and edit it, you are actually editing a copy of that string, so
a simpler way is just to remove the const reference from the method,
everything should work.
>
> > In other words, you should not apply this. You should probably go "Linus
> > is insane, and this is not good, but the *right* way to do it is Xyz",
> and
> > just do Xyz instead.
> >
> > For example, another alternative that I considered (and maybe should have
> > done) was to just create member functions to update the serial number,
> the
> > firmware version and the nickname. And then AddDC() would just do
> > something like
> >
> > (a) look up old entry with matching model/deviceid information.
> > (b) if no old entry exists, create a new entry with empty information
> > and insert that
> > (c) just call the update functions for the passed-in firmware/serial/
> > nickname.
> >
> > that would have been closer to what we used to do.
>
> Yes, I think I would like that logic. It breaks things down to a
> granularity that should be fairly easy to test. Because that's what I
> want. I want TESTS that make sure we don't break things all the time any
> more.
>
> > This patch seemed closer to the new model, but tries to just make sure
> > that if we pass in empty firmware/serial/nickname information, we take
> the
> > values from whatever old entry we find instead.
> >
> > And I *really* didn't test this. Yeah, I created a nickname for my EON,
> > and then I tried to download, and the nickname stayed around, but I
> didn't
> > actually test that that failed before, so the "test" is fundamentally
> > flawed crap.
> >
> > Also, to be honest, I did a "force download all dives", and then clicked
> > "ok", and now I have a couple of dives that have *two* EON Steel cases,
> > apparently because we merged things and didn't notice we already had the
> > first. That seems to be a separate issue entirely, though - the ones that
> > merged correctly had the original EON Steel import as the first one.
> >
> > I'll have to think more about that merging issue, but I'm pretty sure it
> > has nothing to do with this patch.
>
> Merging is causing problems here and there. Again something that we don't
> test enough. But a "force all" should merge every dive, shouldn't it?
> Unless you changed dive times in your dive log or something...
>
> > Anyway, despite the crap testing, this *looks* like it might work. That
> > must count for something? I mean, it did actually compile, so it must
> > be perfect ...
>
> That's better than some of the things that I pushed upstream. Err, I
> mean... really, Linus, you can do better than that, right?
>
:)
>
>
> > Not-Yet-Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
> > ---
> >
> > You can remove the "Not-Yet-" part of my signoff if you actually review
> > this. Or just consider it a failed attempt if you want to do the other
> > approach.
>
> Review below. But I was hoping to motivate you to rewrite this with the
> more granular API and utilizing cute little helper functions :-)
>
> > diff --git a/divecomputer.cpp b/divecomputer.cpp
> > index b45b74a449b9..a487578e7144 100644
> > --- a/divecomputer.cpp
> > +++ b/divecomputer.cpp
> > @@ -58,26 +58,43 @@ const DiveComputerNode *DiveComputerList::get(const
> QString &m)
> > return NULL;
> > }
> >
> > -void DiveComputerList::addDC(const QString &m, uint32_t d, const
> QString &n, const QString &s, const QString &f)
> > +void DiveComputerNode::showchanges(const QString &n, const QString &s,
> const QString &f) const
> > +{
> > + if (nickName != n)
> > + qDebug("new nickname %s for DC model %s deviceId 0x%x",
> n.toUtf8().data(), model.toUtf8().data(), deviceId);
> > + if (serialNumber != s)
> > + qDebug("new serial number %s for DC model %s deviceId
> 0x%x", s.toUtf8().data(), model.toUtf8().data(), deviceId);
> > + if (firmware != f)
> > + qDebug("new firmware version %s for DC model %s deviceId
> 0x%x", f.toUtf8().data(), model.toUtf8().data(), deviceId);
> > +}
>
> So this does nothing when built in Release mode... which makes me wonder
> why we would call it unconditionally below. But then again, we call so
> much overhead code, it won't make a huge difference. And it's definitely a
> useful level of debug information.
>
> > +
> > +void DiveComputerList::addDC(const QString &m, uint32_t d, const
> QString &_n, const QString &_s, const QString &_f)
> > {
> > if (m.isEmpty() || d == 0)
> > return;
> > - const DiveComputerNode *existNode = this->getExact(m, d);
> > - DiveComputerNode newNode(m, d, s, f, n);
> > - if (existNode) {
> > - if (newNode.changesValues(*existNode)) {
> > - if (n.size() && existNode->nickName != n)
> > - qDebug("new nickname %s for DC model %s
> deviceId 0x%x", n.toUtf8().data(), m.toUtf8().data(), d);
> > - if (f.size() && existNode->firmware != f)
> > - qDebug("new firmware version %s for DC
> model %s deviceId 0x%x", f.toUtf8().data(), m.toUtf8().data(), d);
> > - if (s.size() && existNode->serialNumber != s)
> > - qDebug("new serial number %s for DC model
> %s deviceId 0x%x", s.toUtf8().data(), m.toUtf8().data(), d);
> > - } else {
> > + const DiveComputerNode *Old = this->getExact(m, d);
>
> while the existing code also used "this", it's not really needed and we
> are trying to get rid of this pattern in the code.
>
> Also, variables should start with a lower case letter... so 'old'.
>
> > + QString n = _n, s = _s, f = _f;
>
> So you pass them in as reference but then copy them inside the function to
> local variables. why?
>
> > + if (Old) {
> > + // Update any non-existent fields from the old entry
> > + if (!n.size())
>
> I think n.isEmpty() is an easier to read way to implement this
>
> > + n = Old->nickName;
> > + if (!s.size())
> > + s = Old->serialNumber;
> > + if (!f.size())
> > + f = Old->firmware;
> > +
> > + // Do all the old values match?
> > + if (n == Old->nickName && s == Old->serialNumber && f ==
> Old->firmware)
>
> Maybe you could turn "showChanges()" into a function that returns bool. It
> shows the changes when compiled for debugging, but either way it returns
> true it things are changed... so maybe isDifferent(...)
>
> > return;
> > - }
> > - dcMap.remove(m, *existNode);
> > +
> > + // debugging: show changes
> > + Old->showchanges(n, s, f);
> > + dcMap.remove(m, *Old);
> > }
> > - dcMap.insert(m, newNode);
> > +
> > + DiveComputerNode New(m, d, s, f, n);
>
> see above, variables start with lower case letters and let's not use 'new'
> as variable name... newNode wasn't entirely stupid. So maybe use oldNode
> above instead of Old
>
> > + dcMap.insert(m, New);
> > }
>
> The logic seems sound to me. I'm not sure why we remove the old node and
> then add a new node instead of updating the old node, but we did the same
> thing in the existing code, so maybe there's a reason to do so.
>
> So conceptually I like what you said above about "if it exists, update,
> otherwise add" better...
>
> Thanks for working on this, Linus.
>
> /D
> _______________________________________________
> subsurface mailing list
> subsurface at subsurface-divelog.org
> http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20150917/b3843890/attachment-0001.html>
More information about the subsurface
mailing list