GNOME Bugzilla – Bug 605543
Misc improvements based on feedback by clang
Last modified: 2010-01-23 21:11:08 UTC
Created attachment 150448 [details] [review] initial testing for cookies I ran a build of libsoup using clang, and it reported a number of issues - all of which are not really bugs currently, but at least one (the GSList one) uncovers reliance on implementation details of stuff. The first commit is a new test. Feel free to cherry pick those you think are useful.
Created attachment 150449 [details] [review] stop relying on GSList implementation details
Created attachment 150450 [details] [review] unused variable
Created attachment 150451 [details] [review] make soup_form_request_for_data more readable
Created attachment 150452 [details] [review] unnecessary priv variable
Created attachment 150453 [details] [review] unnecessary initialization Oh, btw, if you decide to push some, please let me know which - the patches are using my Collabora email, but should actually carry my gnome.org one, and I'd like to fix this before pushing =).
Comment on attachment 150449 [details] [review] stop relying on GSList implementation details So, I disagree that this is "an implementation detail that might change in the future". It's true that the docs don't explicitly say you can do that, but IMHO that's because the docs are incomplete not because the behavior is undefined. Anyway, the change from append to prepend might break cookie sorting. I would need to think about that. (Also, I need to import abarth's cookie tests into libsoup...) What exactly does clang complain about?
Comment on attachment 150450 [details] [review] unused variable >+ /* result[1] is currently not used; it's a boolean value >+ * regarding whether the cookie should be used for sub-domains >+ * of the domain that is set for the cookie */ >+ The reason why it's unused is because it's redundant; it's TRUE if result[0] starts with a ".", and FALSE if it doesn't. Also, end the comment with a ".", and put the "*/" on the next line by itself. (See other multi-line comments.)
Comment on attachment 150451 [details] [review] make soup_form_request_for_data more readable leaks msg in the new error case. this method is definitely weird. it would probably make a lot more sense if it didn't try to share the soup_message_new_from_uri line between the two cases, but instead had a "GET" case with one soup_message_new_from_uri call, a "PUT"/"POST" case with a separate soup_message_new_from_uri call, and an error case.
Comment on attachment 150452 [details] [review] unnecessary priv variable odd. i would have thought gcc would catch that
(In reply to comment #6) > (From update of attachment 150449 [details] [review]) > So, I disagree that this is "an implementation detail that might change in the > future". It's true that the docs don't explicitly say you can do that, but IMHO > that's because the docs are incomplete not because the behavior is undefined. Yeah, I can agree with this =). > Anyway, the change from append to prepend might break cookie sorting. I would > need to think about that. (Also, I need to import abarth's cookie tests into > libsoup...) OK > What exactly does clang complain about? clang complain that the value assigned to prev is never read. I spent a number of minutes trying to figure out why that assignment even existed, which is why I decided to write a test, and rework the function.
(In reply to comment #10) > clang complain that the value assigned to prev is never read. oh, right, that old trick. we have: prev = g_slist_append (prev, cookie); and clang warns because we never use prev after that. But if we *don't* assign the return value to prev, then gcc warns because g_slist_append is tagged G_GNUC_WARN_UNUSED_RESULT. I guess prev->next = g_slist_append (NULL, cookie); would make both of them happy. Maybe "prev" should be called "last" in that case.
Committed alternate soup-cookie-jar and soup-form patches as suggested above. I was going to commit the cookie-test, but it doesn't use the style of the existing tests (eg, using test-utils and keeping a tally of errors rather than aborting on the first error), so I didn't.