Enrico Tröger schrieb:
On Sat, 05 Sep 2009 18:30:13 +0200, Colomban wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
Thomas Martitz a écrit :
My patch was buggy. I don't know how I didn't notice it while testing. This should fix it http://pastie.org/606912
Nothing important, but I wonder why you test for (!reload) as you already check for (reload)? The check at the right of a || switch will be tested only if the left one is not true, then if the right check happens, then reload is necessarily false, isn't it? But it just seems strange and complicated to me, not a problem anyway.
Same here.
Before the patch we only checked for "if (reload)". The function documentation already says: if reload is set, the doc argument has to be non-NULL, if reload is not set, doc is NULL. So, the new, additional check is somewhat unnecessary. It could be argumented as an additional sanity check but generally, document_open_file_full() is an internal function and so if the above conditions of the input argumens are not fullfilled, I would consider this as a bug in the calling code (still sanity checks are good, no doubt).
Regards, Enrico
Well, before my patch document_open_file_full() *returned* st
raightly after finding the file in the internal open file list (in the !reload case). I added the goto to maintain that behavior without duplicating the "grab focus" code. Now that my latest fix removed the goto also, the original behavior should be still maintained, hence I added the if () (it went that road always if (reload == true), and only if (!reload) *and* it wasn't found in the internal document list (i.e. it's still NULL).
I'm not sure if it's really a bug of the calling code, and I'm also not sure why I got that if () wrong in the first place without noticing it at testing.
The culprit is probably that the function basically does a reload, even if (doc != NULL). But that was the original behavor which I never intended to change.
Btw, I'm thinking the goto saved a difficult to figure out/parse if () construct, but that may just be me :)
Best regards.