add replace_all documentation #3478
Conversation
kroening
commented
Nov 29, 2018
kroening
commented
- Each commit message has a non-empty body, explaining why the change was made.
- n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
- n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
- Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
- n/a My commit message includes data points confirming performance improvements (if claimed).
- My PR is restricted to a single feature or bugfix.
- n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.
kroening
requested review from
peterschrammel,
pkesseli,
smowton and
tautschnig
as code owners
src/util/string_utils.cpp
Outdated
| void replace_all( | ||
| std::string &str, | ||
| const std::string &from, | ||
| const std::string &to) | ||
| { | ||
| PRECONDITION(from.size() != 0); |
There was a problem hiding this comment.
I think !from.empty() is more idiomatic?
src/util/string_utils.cpp
Outdated
| /// \param to: string to replace with | ||
| /// Copyright notice: | ||
| /// Attributed to Gauthier Boaglio | ||
| /// Source: https://stackoverflow.com/a/24315631/7501486 | ||
| /// Used under MIT license | ||
| /// Note that this has quadratic complexity if from.size() != to.size() |
There was a problem hiding this comment.
Quadratic in ... what? The length of str? That's equally true then of the implementation that triggered this discussion in #3463.
kroening
force-pushed
the
replace_all_doc
branch
from
3491de0 to
4ecf211
Compare
kroening
force-pushed
the
replace_all_doc
branch
from
4ecf211 to
366d563
Compare
There was a problem hiding this comment.
🚫
This PR failed Diffblue compatibility checks (cbmc commit: 4ecf211).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/92976712
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.
Common spurious failures:
- the cbmc commit has disappeared in the mean time (e.g. in a force-push)
- the author is not in the list of contributors (e.g. first-time contributors).
The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 366d563).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/92977248
| /// \param to: string to replace with | ||
| /// Copyright notice: | ||
| /// Attributed to Gauthier Boaglio | ||
| /// Source: https://stackoverflow.com/a/24315631/7501486 | ||
| /// Used under MIT license | ||
| /// Note that this is quadratic in str.size() if from.size() != to.size(). | ||
| /// There is no guarantee that it is linear in str.size() if | ||
| /// from.size() == to.size(). |
| /// \param to: string to replace with | ||
| /// Copyright notice: | ||
| /// Attributed to Gauthier Boaglio | ||
| /// Source: https://stackoverflow.com/a/24315631/7501486 | ||
| /// Used under MIT license | ||
| /// Note that this is quadratic in str.size() if from.size() != to.size(). |
There was a problem hiding this comment.
... , because each replacement entails a copy of the contents of the string.
Or maybe something else?
There was a problem hiding this comment.
No, that would not change the complexity in str.size() -- unfortunately, std::string::replace does not provide the guarantee that replacing equal-length strings is not independent of the length of the containing string.
|
The precondition is important, but instead of the documentation dance about complexity we might just fix the implementation? str result;
std::string::size_type cursor = 0;
for(std::string::size_type replace_at = str.find(from, 0); replace_at != std::string::npos; replace_at = str.find(from, replace_at))
{
result += str.substr(cursor, replace_at - cursor);
result += to;
cursor += from.length();
replace_at += from.length();
}
if(cursor != str.size())
result += str.substr(cursor);
str.swap(result); |
|
There are three cases:
On an experiment with strings of length 6000: Thus, I'd propose to stick to the current one, with a warning that it may be slow for long strings. |
|
@kroening many thanks for the analysis. What about inserting a |
|
The .reserve() makes hardly any measurable difference (the default enlargement strategy seems to work well). |
There was a problem hiding this comment.
Approving after sitting on it for way too long, but really I think this function should just be removed:
- We do not have a single use in the code base.
- It's completely unrelated to program analysis or any other CBMC matter.
- If anyone else uses it (AFAIK TG does), they are more than welcome to copy the implementation.
Any user can then optimise it for their use case.
