Subtle bug in Slider::getValueFromText

General JUCE discussion
1 
double Slider::getValueFromText (const String& text)
{
    auto t = text.trimStart();
    
    const String szSuffix = getTextValueSuffix();
    
    if (szSuffix.isNotEmpty() && t.endsWith (szSuffix))
        t = t.substring (0, t.length() - szSuffix.length());

    if (valueFromTextFunction != nullptr)
        return valueFromTextFunction (t);

    while (t.startsWithChar ('+'))
        t = t.substring (1).trimStart();

    return t.initialSectionContainingOnly ("0123456789.,-")
            .getDoubleValue();
}

The existing code has

if (t.endsWith (getTextValueSuffix()))

which if there’s no suffix always evaluates to true.

Rail

2

I’m not sure if I’m missing something, but doesn’t this line also check is the suffix is empty?
And (unless I’m mistaken) in C++ if the first check of an && statement is false, it won’t even bother evaluating the next check since the entire statement is already false.
But even if that’s not the case, in your example the full evaluation will be false even if endsWith() returns true because isNotEmpty() will return false.

Perhaps your concern is that endsWith() returns true on empty strings, but that’s a different complaint (and not necessarily a bug, since a string will always have nothing (or a terminating character) at the end, so evaluating to true isn’t technically “wrong”).

3

The empty check is my correction. Obviously this isn’t a bug that’s gonna cause any issues but it’s bad code not to check if the suffix is empty - and Jules prided himself on having good code.

Rail

4

Oh yes, I misunderstood your post - thought the big code chunk was from the existing code.

So this is a code quality thing, and not a bug then. The outcome is the same with or without the isNotEmpty() check and it doesn’t crash. I wouldn’t assume one is more efficient than the other without testing.

I also wouldn’t assume Jules wrote this code, or that all of the JUCE code should be evaluated based on your personal assumption of what he might think is good or bad.

That being said, if you feel strongly you can make a pull request. JUCE is open source after all.

I personally have no strong feelings on this matter now that it’s clear what your post was about.

5

Yes, a code quality issue. Committed May 4, 2012 by Jules.

Rail

6

In that case this line can be removed altogether.

The undelying readDoubleValue() stops as soon as a character is hit, that doesn’t contribute to the number. No error is thrown, if that operation is skipped.