[hist] Fix pattern-matching bug in TFormula::HandleLinear()#8801
[hist] Fix pattern-matching bug in TFormula::HandleLinear()#8801guitargeek merged 2 commits intoroot-project:masterfrom
Conversation
701948a to
d033187
Compare
Simple C++ code to demonstrate the problem:
```C++
TF1{"f1", "1.0++x++x*x++x*x*x", -1.0, 1.0}.Print();
```
Before this commit, running this line gave you:
```
Formula based function: f1
f1 : 1.0++x++x*x++x*x*x Ndim= 1, Npar= 3, Number= 0
Formula expression:
([p0]*(1.0))+([p1]*(x))+([p2]*(x*x))+([p2]*(x*x))*x
```
The problem is that `TFormula::HandleLinear()` uses pattern-matching to
replace the strings representing the linear terms. This is problematic
if one of the strings is a substring of another one.
d033187 to
9bec4b5
Compare
lmoneta
left a comment
There was a problem hiding this comment.
Thank you for the fix Jonas !
Looks good, I have only a comment, it would be nice to have a new test showing the previous failure in test/TFormulaParsingTests.h
The formula is such that earlier terms are substrings of later terms, triggering a bug that has recently been fixed.
|
Starting build on |
Axel-Naumann
left a comment
There was a problem hiding this comment.
LGTM but see commented. Thanks for fixing this "properly"!
|
|
||
| namespace { | ||
|
|
||
| // Borrowed from RooHelpers.h, should this go to a central place? |
There was a problem hiding this comment.
I agree, this should totally go into core/foundation/inc/ROOT/. Should this return vector<string> or vector<string_view>? The latter is dangerous if people do tokenize("abc cde",...) but I find that unlikely to happen.
There was a problem hiding this comment.
Okay! I don't have time to do this now, but I'll open a GitHub issue about it. Maybe we should have two versions: one returning vector<string> and one returning vector<string_view>.
| namespace { | ||
|
|
||
| // Borrowed from RooHelpers.h, should this go to a central place? | ||
| std::vector<std::string> tokenise(const std::string &str, const std::string &delims, bool returnEmptyToken) { |
There was a problem hiding this comment.
We generally use US spelling, e.g. TString::Tokenize
There was a problem hiding this comment.
Okay, this should be changed then when the function is moved to core/foundation.
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
Failing tests: |
Simple C++ code to demonstrate the problem:
TF1{"f1", "1.0++x++x*x++x*x*x", -1.0, 1.0}.Print();Before this commit, running this line gave you:
The problem is that
TFormula::HandleLinear()uses pattern-matching toreplace the strings representing the linear terms. This is problematic
if one of the strings is a substring of another one.