These are chat archives for symengine/symengine

9th
Jan 2017
Ranjith Kumar
@ranjithkumar007
Jan 09 2017 11:54
@isuruf there are many instances like r->str()=="oo" instead of eq(r1,Inf) ..should they be changed ?
Isuru Fernando
@isuruf
Jan 09 2017 11:54
on most instances, yes
but I wouldn't worry too much about it
Ondřej Čertík
@certik
Jan 09 2017 15:40
@isuruf @ShikharJ let's figure out the get_coef() issue.
@isuruf said here that returning a const reference is the same as returning the internal member. That might be true, but I am not sure if it plays nicely with returning RCP as a reference.
they are pretty clear that smart pointers must be returned by value.
BUT --- there might be a performance hit in usage like e.get_coef()->do_something() vs e.coef_->do_something().
Ondřej Čertík
@certik
Jan 09 2017 15:46
For this reason, I suggested to also introduce get_coef_ptr() that returns a Ptr, then there will not be any performance hit in e.get_coef_ptr()->do_something().
Francesco Biscani
@bluescarni
Jan 09 2017 15:46
auto deduction works in the same template deduction works: if get_coef() returns a reference, then auto x = e.get_coef(); will create a non-reference object
Ondřej Čertík
@certik
Jan 09 2017 15:47
@bluescarni ok. But what should get_coef() return, based on the above?
Francesco Biscani
@bluescarni
Jan 09 2017 15:47
decltype(auto) x = e.get_coef(); will create a reference, but it's C++14 only
Ondřej Čertík
@certik
Jan 09 2017 15:48
@bluescarni I guess if you wanted a reference, could you do auto &x = e.get_coef()?
Francesco Biscani
@bluescarni
Jan 09 2017 15:48
returning a copy of the smart pointer is safer, but it has a higher cost... if I have understood correctly the question :)
yes
RCP is like shared_ptr right?
Ondřej Čertík
@certik
Jan 09 2017 15:49
@bluescarni that's right, RCP is like shared_ptr
Are you sure about the higher cost, even with return value optimization?
Francesco Biscani
@bluescarni
Jan 09 2017 15:50
the return value optimisation kicks in if you are returning an object that you created locally in the method, but as I understand you are returning a class member
you still need to bump up the refcount
Ondřej Čertík
@certik
Jan 09 2017 15:50
@bluescarni correct.
Francesco Biscani
@bluescarni
Jan 09 2017 15:51
if you return a reference you get a dangling reference if e is destroyed and the reference remains alive afterwards
so it will be ok to return a ref if you can prove the lifetime of the ref is smaller than the lifetime of the originating object
Ondřej Čertík
@certik
Jan 09 2017 15:51

Here are the two main use cases, in master:

auto x = e.coef_; // Must increment refcount
x->do_something;

and

if (e.coef_->something()) { ...  // Should not increment refcount
Francesco Biscani
@bluescarni
Jan 09 2017 15:53
if e is passed in as a function argument and it is not destroyed within that function, and x cannot escape the function scope, then you are fine with the reference
Ondřej Čertík
@certik
Jan 09 2017 15:53
@bluescarni we have usage where we assign the result of coef_ into some other object and that other object can outlive the original.
Francesco Biscani
@bluescarni
Jan 09 2017 15:53
it depends how the assignment happens. if it involves a copy of the original coef_ then you are still good
Ondřej Čertík
@certik
Jan 09 2017 15:54
coef_ is of a type RCP<Basic>, and the assignment is a copy, which causes the refcount to increase (which is what we want if we assign it --- but if we do not assign it, we do not want the refcount to increase)
if get_coef() returns a reference, I am worried the refcount might not always be incremented, am I right?
Francesco Biscani
@bluescarni
Jan 09 2017 15:55
it does not get incremented, but the moment you assign the result into another object by copy, the refcount will be increased
auto &x = get_coef(); // refcount stays the same
obj.set_x(x); //++refcount because copy is created into obj
Isuru Fernando
@isuruf
Jan 09 2017 15:56
@certik, .coef_ and .get_coef() are exactly the same. Both return references. Right, @bluescarni ?
Ondřej Čertík
@certik
Jan 09 2017 15:57

@bluescarni so you are saying that if get_coef() returns a reference to RCP, then the following two use cases are safe?

auto x = e.get_coef(); // Must increment refcount
x->do_something;

and

if (e.get_coef()->something()) { ...  // Should not increment refcount

If so, what is an unsafe usage of get_coef(), if it returns a reference?

Isuru Fernando
@isuruf
Jan 09 2017 15:58
An unsafe usage would be
const auto &x = e.coef_;
// delete e;
// use x
Francesco Biscani
@bluescarni
Jan 09 2017 15:58
something like this would be unsafe:
e = new(...);
auto &x = e->get_coef();
delete(e);
Ondřej Čertík
@certik
Jan 09 2017 15:58

Is the following equally unsafe:

e = new(...);
auto &x = e->coef_;
delete(e);

?

Francesco Biscani
@bluescarni
Jan 09 2017 15:59
I apologize but need to take the train back home, I'll be online later
Isuru Fernando
@isuruf
Jan 09 2017 15:59
@certik, I think you added an extra parenthesis in the last code
Francesco Biscani
@bluescarni
Jan 09 2017 15:59
it would still be unsafe
Ondřej Čertík
@certik
Jan 09 2017 16:00
@bluescarni, @isuruf so in light of this, coef_ and get_coef() (that returns a reference) are exactly equivalent.
Isuru Fernando
@isuruf
Jan 09 2017 16:00
Yes
Francesco Biscani
@bluescarni
Jan 09 2017 16:02
but in both cases copies of the member are done:
auto x = e.get_coef();
auto x = e.coef_;
Ondřej Čertík
@certik
Jan 09 2017 16:02
@isuruf in that case, I think get_coef() should return a reference, after all. Because if it returns a value, there will be a performance hit in the if (e.get_coef()->something()) { use case.
Isuru Fernando
@isuruf
Jan 09 2017 16:03
Yes, I wouldn't worry too much about RCPs as it's just a refcount increase and decrease, but for other data like vectors and dictionaries, it'll be expensive
So, I think #1150 is ready to go
Ondřej Čertík
@certik
Jan 09 2017 16:04
@isuruf actually, we have to worry about RCP. I saw big speedup by designing our own RCP class, and one should avoid refcount increases.
#1150 --- unfortunately it was ready to go, but @ShikharJ changed it to return a value, even though I asked him to wait until we resolve it. Based on our discussion above, I think it should return a reference.
Isuru Fernando
@isuruf
Jan 09 2017 16:06
Right.
Ondřej Čertík
@certik
Jan 09 2017 16:10
@bluescarni, @isuruf it seems the "safe" rule here is to "never create references like auto &x = e.coef_;". Then even get_coef() can return a reference, and as long as this rule is followed, it is always 100% safe.

How about the following usage:

void f(const RCP<Basic> &x) {
    store(x); // Store `x` somewhere, by copying, which increments the refcount
}

f(e.coef_);
f(e.get_coef());

Are both cases again equivalent?

Shikhar Jaiswal
@ShikharJ
Jan 09 2017 16:14
@certik I'm sorry, I misinterpreted the general consensus to be in favour of converting inline const RCP<const Number> &get_coef() to inline RCP<const Number> get_coef(). It is just a minor change, as only some classes had an RCP data member. Won't take me long to revert the changes.
Ondřej Čertík
@certik
Jan 09 2017 16:15
@ShikharJ I was going back and forth, as I didn't realize that if it returns a reference, it is no more unsafe than using coef_ directly.
I thought it is more unsafe, but that is not the case. It is not worse.
Isuru Fernando
@isuruf
Jan 09 2017 16:18
@certk, yes both are equivalent in the above code
Ondřej Čertík
@certik
Jan 09 2017 16:18
While there is almost certainly a performance hit if we return RCP by value.
Shikhar Jaiswal
@ShikharJ
Jan 09 2017 16:18
@certik What to do of the non-RCP data members? Should they also be accessed by returning a reference?
Ondřej Čertík
@certik
Jan 09 2017 16:18
@ShikharJ I think so.
I think member methods can and should return references to internal data members (of any kind) since as long as you follow my "safe" rule above, that is, you do not create a local reference somewhere, then things either will get copied like in auto x = e.get_internal_member() (safe), or things will just be used on a single line, like in if (e.get_internal_member()->do_something()), which is also safe.
I.e. what you cannot do is auto &x = e.get_internal_member().
Shikhar Jaiswal
@ShikharJ
Jan 09 2017 16:25
:+1:
Ondřej Čertík
@certik
Jan 09 2017 16:28

Now, there is actually one use case, that has always bothered me, but that already exists in master, so that's not relevant to the above discussion, but nevertheless I'd like to figure out some rules how to avoid it:

RCP<Basic> y;

void f(const RCP<Basic> &x) {
    y.reset() // Delete global `y`
    std::cout << *x; // x is invalid here
}

y = symbol("y");
f(y);

One can further simplify this:

RCP<Basic> y;

void f(const Basic &x) {
    y.reset() // Delete global `y`
    std::cout << x; // x is invalid here
}

y = symbol("y");
f(*y);

In other words, f is just a regular C++ function, that accepts a const reference. Unrelated to smart pointers. But if that function invalidates the global object y which holds x, then the argument x becomes invalid. I don't know how to avoid that.

Isuru Fernando
@isuruf
Jan 09 2017 16:30
@certik, you can't. .reset() should not be accessible
Ondřej Čertík
@certik
Jan 09 2017 16:31
In symengine, all global objects are const, so I guess reset() wouldn't compile, correct?
(because reset() modifies the const RCP)
However, their solution is to increment the refcount locally, and we do not follow that in symengine. In other words, we get some RCP somewhere (it's not local), and then we happily pass it to a function like f.
Ondřej Čertík
@certik
Jan 09 2017 16:36
We are diligent about f saving the argument x somewhere --- we would always call rcp_from_this, which increments the refcount and thus is safe. So that's not a problem. Our only problem is if f deleted the objects (they don't have to be global, just part of some expression that gets out of scope somehow) that hold x, during the duration of f.
Shikhar Jaiswal
@ShikharJ
Jan 09 2017 16:43
@certik Reverted the changes.
Ondřej Čertík
@certik
Jan 09 2017 16:46

The use case that could potentially break is something like this:

void f(const RCP<const Basic> &e, const Basic &x) {
    // Somehow change `e` to forget about `x` --- in this case `e` is const, so we probably can't.
    std::cout << x; // x is invalid here
}

RCP<const Basic> e, y;
e = add(symbol("x"), symbol("y"));
f(e, *e->get_args()[0]);

In this case it's still safe. Perhaps there is no way this could break.