[BR] var::equals string/double bug

1

this var::equals() comparison is failing :

var doubleVar { 1. };
var stringVar { "1." };
jassert (stringVar.equals (doubleVar));
2

Have you tried "1.0" or "1"?

3

yes. “1.0” works. I’m only reporting what does’t work :slight_smile:

4

I wouldn’t say that checking for "1." is a bug personally - that’s not the way juce::var formats floating-point numbers and adding the additional checks for different ways of formatting would make the comparison more expensive.

5

I’m not sure whether var.equals is supposed to be commutative. It’s a bit surprising that it’s not, but at the same time, the docs don’t make any guarantees, and modifying the behaviour would be a subtle breaking change.

In the snippet you posted, you can get the comparison to succeed by switching the order of the arguments:

var (1.0).equals ("1.");
    // true, converts string to double, then compares values as doubles

var ("1.").equals (1.0);
    // false, converts double to string ("1.0") and then compares values as strings
6

the thing is that I have some valueTrees/xml with such double values.

I then use getPropertyAsValue(), and use some sliders to control the value.
the problem is that this because of the equals() issue mentioned above that test in slider is failing :

here is a pip to reproduce (you will see that valueChanged() is called while it shouldn’t).

PIP 
/*******************************************************************************
 The block below describes the properties of this PIP. A PIP is a short snippet
 of code that can be read by the Projucer and used to generate a JUCE project.

 BEGIN_JUCE_PIP_METADATA

  name:             SliderTest

  dependencies:     juce_core, juce_data_structures, juce_events, juce_graphics, juce_gui_basics
  exporters:        XCODE_MAC

  moduleFlags:      JUCE_STRICT_REFCOUNTEDPOINTER=1

  type:             Component
  mainClass:        MyComponent

 END_JUCE_PIP_METADATA

*******************************************************************************/

#pragma once


//==============================================================================
class MyComponent   : public juce::Component
                    , private Value::Listener
{
public:
    //==============================================================================
    MyComponent()
    {
        value.addListener (this);
        slider.setRange (0., 100.);
        slider.getValueObject().referTo (value);

        addAndMakeVisible (slider);
        setSize (600, 400);
    }

    //==============================================================================
    void paint (juce::Graphics& g) override
    {
        g.fillAll (getLookAndFeel().findColour (juce::ResizableWindow::backgroundColourId));
    }

    void resized() override
    {
        slider.setBounds (50, 50, 400, 50);
    }

    void valueChanged (Value& value) override
    {
        DBG ("value changed " << (double) value.getValue());
    }

private:
    //==============================================================================
    ValueTree valueTree { "tree", {{ "value", "50." }} };
    Value value { valueTree.getProperty ("value") };

    Slider slider;

    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (MyComponent)
};
7

the doc clearly states :

a string var "123" and an integer var with the value 123 are considered to be equal.

so 1. beeing a valid double, I think that everybodys expect var.equals() to work with “1.” whatever the order of the arguments.
I understand that it might be a subtle breaking change for anyone relying on a weird behaviour :), but probably anyone parsing some doubles would expect that to work no?

8

But if you go one step further, that would mean, that the strings “1.” and “1.0” should be considered equal, because if they happen to be converted to double they would be equal.
That would be extremely surprising.

It’s hard to figure out which should be the base of comparison, if everything is freely convertible to everything.

I think the docs would need to be amended to point out that the comparison is done in a determinate domain which is the left value.

Good catch though

9

Daniel makes a good point here. I think it’s probably safest to leave the implementation of equals as-is - although it sounds like the docs could be a bit clearer.

I’d be more comfortable with changing the Slider implementation so that it explicitly casts currentValue and newValue to double before checking them for equality. I’ll have to think about it a bit more, but that feels safer to me at the moment.

2 Likes
10

yes, I agree it’s a tricky one.
I would be happy with just the Slider’s change. I already have that as workaround locally. thanks

11

FWIW, using the console in Chrome,

> 1.0 == "1."

returns true.

Whereas with juce::JavascriptEngine, the same expression returns false.

I believe juce::var is meant to work with JSON? So maybe the comparisons should work the same as JS?

12

I know why I never touch JS :wink:
– SCNR

13

Thanks, I’ve updated the slider implementation here:

The Variant equality docs have also been updated:

1 Like
14

I think it’s a genuine bug and should be squashed:

https://262.ecma-international.org/5.1/#sec-11.9.3

2.4. If Type( x ) is Number and Type( y ) is String,
return the result of the comparison x == ToNumber( y ).
2.5. If Type( x ) is String and Type( y ) is Number,
return the result of the comparison ToNumber( x ) == y .

Rest of this section is worth reading as well, glad to find that there’s method to JSs madness, making it even weirder doesn’t seem to be the right direction :wink:

15

Raises the question: just because juce::var is used in the JavaScript engine, does this mean it needs to adapt to the JS madness like you rightfully call it and make it weird for any non-JS usage too?

It would rather be fixed in the JS engine then…

16

It’d be a lot easier to explain "juce::var follows the JS standard" than it would to explain the unique behaviour it currently has. And removes any debate over what behaviour it should have.

2 Likes
17

As a user of a C++ framework never touching JS don’t appreciate that attitude :wink:

1 Like
18

I agree with ImJimmi. People expect var to behave like JS and it’s just easier to explain - and there’s already a specification the implementation could be checked against.

19

Again, that would render var useless for me, since the current behaviour makes sense and the JS one doesn’t IMHO, but I don’t care what JS does or doesn’t.
Changing var is extremely intrusive, it would change behaviour of ValueTree properties, CachedValue, basically everything.
That’s why I say a fix needs to be in the JS engine land only.

But ultimately the JUCE team will decide.

1 Like
20

FWIW I agree that var shouldn’t change - it’s so fundamental to other areas of the framework (like ValueTree as @daniel points out) that it would cause mayhem as a breaking change.