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