Added support for down_cast() and made changes in the codebase

[ShikharJ]

This is a follow up for PR #746 by Abhimanyu Siwach.
down_cast() has been used in place of static_cast() strictly in places where the arguments were of the type Basic or any of its derived classes.
Please suggest any changes if required.

[ShikharJ]

@isuruf Ping.

[isuruf]

I think all we need is something like

 +template <typename To, typename From>
 +inline To& down_cast(From &f)
 +{
 +    SYMENGINE_ASSERT(dynamic_cast<To*>(&f) != NULL);
 +    return *static_cast<To*>(&f);
 +}

EDIT: Ignore above
Also, if you are using another person's PR, please use their commit instead of copy-pasting.

[certik]

@isuruf are you against using the code from Google? I think it does some extra compile time checks, etc.

@ShikharJ indeed, as Isuru said, please use the commits from the old PR, to attribute the original author's work, and then add your own commits on top, fixing it up.

[isuruf]

@certik, implicit_cast<From *, To>(0); is this valid C++?

[certik]

@isuruf why not? Does any compile produce a warning on that code?

[ShikharJ]

@isuruf @certik I'll have to change static_cast() to down_cast() wherever Basic or its derivative (Set, Symbol, Constant etc.) is present as an argument right?

[certik]

@ShikharJ I think so. I think that's also the only casting we do, don't we?

[ShikharJ]

@certik I am not really comfortable with Teuchos and related stuff. At some places though I did see casts like duration_cast, reinterpret_cast and some other RCP casts, if we're talking about all types of casting. But they are not related to this PR, are they?

[certik]

Don't modify Teuchos. That library we just use as-is, and we assume it is correct. If it has bugs, then we'll report them upstream and they'll get fixed.

We use reinterpret_cast at just 9 places, leave that be for now.

Also do not modify the ptr_dynamic_cast, rcp_dynamic_cast and rcp_static_cast. The dynamic_cast is not really used either.

So I would suggest to only replace static_cast with down_cast (and only in the symengine directory, but excluding utilities).

[ShikharJ]

@certik Should I also make the changes when the argument is of a template type? Like the following case:

SYMENGINE_ASSERT(is_a<T>(x))
        return number(std::sin(static_cast<const T &>(x).i));

[certik]

@ShikharJ I thinks so --- is there a reason not to?

[ShikharJ]

@certik The Series type is not a derivative of Basic, and may still creep up as an argument in one of the above cases, if I am not wrong (please correct me otherwise). Are we changing the code only for the derivatives of Basic or the entire mentioned domain, where static_cast has been used for down-casting?

[certik]

@ShikharJ I think static_cast can only be used if it's an actual subclass, of either Basic, or some other ancestor. I don't see how templates change that. So I think down_cast should be used there too.

[ShikharJ]

@isuruf @certik
Most of the codebase has been covered in the above commits. Still there are a number of files for which the changes to down_cast () or implicit_cast() have to be made (11 files to be exact). I would like to address them through another PR, as those files tend to raise errors, and proper debugging would be required, which would still eat up a lot of time (if required we can raise an issue with the name of the files that need to be worked upon). Kindly suggest any necessary changes.

[certik]

I think this looks very good. +1 to merge if tests pass. I left just a minor comment.

[certik]

Remove this line. The Teuchos library should not be modified.

[isuruf]

Why this change?

[ShikharJ]

@isuruf I must've accidentally made the change. I'll correct it (anyways the build needs to be re-run as Travis had an issue with OSX related jobs previously).

[ShikharJ]

@isuruf A final review?

[certik]

I think that this is good. Thanks @ShikharJ !

[ShikharJ]

@certik Just a suggestion. #746 can also be closed now, since it has been incorporated above.