Bug 21261 - [PATCH] Improved dbus_string_replace_len()
Summary: [PATCH] Improved dbus_string_replace_len()
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: low enhancement
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: r+ from Havoc, 1.5?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2009-04-17 13:57 UTC by Roberto Guido
Modified: 2011-02-25 08:52 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Improved dbus_string_replace_len() (3.10 KB, patch)
2009-04-17 13:57 UTC, Roberto Guido
Details | Splinter Review
Reworked patch for improved _dbus_string_replace_len() (6.20 KB, patch)
2009-04-20 09:09 UTC, Roberto Guido
Details | Splinter Review

Description Roberto Guido 2009-04-17 13:57:06 UTC
Created attachment 24905 [details] [review]
Improved dbus_string_replace_len()

Patch which try a different implementation for dbus_string_replace_len() in dbus-string.c : this provides different behaviours in front of different relation among overwriting and overwritten text.
Some new test is also added in dbus-string-utils.c, so to verify correct behaviour with different string lengths.
Comment 1 Havoc Pennington 2009-04-17 19:32:36 UTC
Can you explain what problem or bug you are trying to solve with this patch?
Comment 2 Roberto Guido 2009-04-18 03:08:04 UTC
(In reply to comment #1)
> Can you explain what problem or bug you are trying to solve with this patch?
> 
The "@todo" found above the function herself.
Are not "todo"s and "fixme"s eligible for patching?
Comment 3 Havoc Pennington 2009-04-18 10:37:03 UTC
> The "@todo" found above the function herself.
> Are not "todo"s and "fixme"s eligible for patching?

I see. Yes, this is good to fix, I just did not know what motivated the patch. Thanks.

Some comments:

* patch should remove the @todo (which will also document what the patch does)

* body of "if (len == replace_len)" should have braces around it even though it is only one line

* for the unit tests, I would suggest "starting over" rather than using the somewhat odd strings left over from the previous tests, to make them clearer.
For example start with "Hello World" and replace "Hello" with "Replacement Text" to get "Replacement Text World", or whatever. Avoiding things like "HWorldloHello Hello" where it's hard to understand.

* would recommend adding tests for start != 0, since an easy bug would be to fail to add start to len

* in the len > replace_len case, a _dbus_assert(len > replace_len) is as good at documenting as the comment, but also checks during testing, so can replace the comment with assert
Comment 4 Roberto Guido 2009-04-18 15:43:45 UTC
> * would recommend adding tests for start != 0, since an easy bug would be to
> fail to add start to len
> 
I've missed this point: what means "test for start != 0" ?
If you refer on unit testing, the latest case added by the patch already starts from position 6 of the source string, while if you refer on addictional checks in the function I cannot understand why "start" could not be 0...
Comment 5 Havoc Pennington 2009-04-18 18:13:15 UTC
well there are three cases in the function (len == replace_len, len < replace_len, len > replace_len). I was just saying it makes sense to unit test both start==0, and start>0, for each case. Because a possible bug is to fail to offset by "start". Also useful I think is to be sure to test with the replace_at != start, to catch any confusion between those two.

Comment 6 Roberto Guido 2009-04-20 09:09:48 UTC
Created attachment 24970 [details] [review]
Reworked patch for improved _dbus_string_replace_len()

Re-worked patch, as suggested
Comment 7 Havoc Pennington 2009-04-21 14:46:27 UTC
Looks great. Someone else will need to apply... it seems my ssh key doesn't work anymore. But I think the patch is ready.
Comment 8 Simon McVittie 2011-01-11 05:47:32 UTC
I think this is a job for 1.5, when we've branched 1.4.
Comment 9 Simon McVittie 2011-02-18 06:32:45 UTC
Applied to master, will be in dbus 1.5.0. Thanks!

In future it'd be useful if you could provide patches in "git format-patch" format (or as a git branch we can add as a remote), which makes them easier to merge.

Providing a commit message saying what you changed and why would also have avoided Havoc asking why you wanted this change, back in Comment #1 - see http://spheredev.org/wiki/Git_for_the_lazy#Writing_good_commit_messages for some good advice on commit messages.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.