GNOME Bugzilla – Bug 624284
Improve check in guess_language function.
Last modified: 2010-07-16 11:50:08 UTC
Created attachment 165833 [details] [review] Improves check in guess_language. Now gtksourceview check if filename is not NULL before it tries to guess language from it. It should also check if filename is not empty string (*filename != 0), because it won't deduce anything useful from empty string and will waste time on getting all language ids and so on. Attached patch add the check for filename as empty string.
Review of attachment 165833 [details] [review]: Commented ::: gtksourceview/gtksourcelanguagemanager.c @@ +639,3 @@ */ + if (filename != NULL && *filename != 0) Good catch though, I'd prefer *filename != '\0'
Created attachment 165834 [details] [review] Reworked patch improving a check. The version with '\0' was my first one, but I changed it to 0, because I saw such check in assertion above. :)
Now I have noticed that because of one assertion an addition in my patch is never executed - please see https://bugzilla.gnome.org/show_bug.cgi?id=624287
I'm now starting to wonder if this is ok. Is it ok having an empty name? I think that the assertion is ok and the new check is not really needed. Paolo any thoughts about this?
Currently passing empty strings causes assertion fail (as written bug mentioned in comment 4). So the only accepted parameters are non empty strings and NULL strings. I just thought that empty strings should be treated like NULL strings. So, if you pass two empty strings or two NULL strings or one empty string and one NULL string, then this is programming error. But passing an empty and non empty should be ok - if filename is an empty string then it is not taken into consideration (with my patch merged). In that case language is guessed on content_type which, at this point we know, is neither NULL nor empty string.
After talking in paolo we are ok by allowing empty strings and the check should be definetly '\0' instead of 0. Also can you provide a unit test? You can find the unit test for the guess language in the test directory.
I'll do it today evening.
Created attachment 165999 [details] [review] Updated patch, now with tests. I'm attaching a patch containing an assertion replacing two old ones, said additional check in `if' statement and additional tests.
Review of attachment 165999 [details] [review]: It looks good to me. Paolo can you give it a second review?
Review of attachment 165999 [details] [review]: Yep, looks good
Ignacio, could you commit this patch for me? I won't have access to my computer this weekend. Thanks.
Pushed the patch. Thanks a lot.