Latest GCC (8) & fix for cse (fixes #1463)

[bjodah]

While #1465 is functional, in this PR I'll try to make Travis fail for the regression test, before applying the fix.

[bjodah]

The build with latest gcc uses -Werror, gcc-8 has introduced some new warnings. Should I drop -Werror from that build for now? (and add it to another build using an older gcc version?)

[bjodah]

Next warning to fail:

/home/travis/build/symengine/symengine/symengine/tests/basic/test_relationals.cpp: In function ‘void ____C_A_T_C_H____T_E_S_T____10()’:
/home/travis/build/symengine/symengine/symengine/tests/basic/test_relationals.cpp:178:43: error: catching polymorphic type ‘class SymEngine::SymEngineException’ by value [-Werror=catch-value=]
     CHECK_THROWS_AS(Lt(ComplexInf, zero), SymEngineException);
                                           ^~~~~~~~~~~~~~~~~~

[bjodah]

Success, the test triggers the failure:

/usr/include/c++/8/bits/stl_algo.h:5269:
Error: elements in iterator range [__first1, __last1) are not sorted.

[bjodah]

gcov seem to be failing:

==> Running gcov in /home/travis/build/symengine/symengine (disable via -X gcov)
/home/travis/build/symengine/symengine/build/symengine/CMakeFiles/symengine.dir/parser/parser.cpp.gcno:'_ZN7Teuchos14TypeNameTraitsINS_11RCPNodeTmplIN9SymEngine8RealMPFRENS_13DeallocDeleteIS3_EEEEE12concreteNameERKS6_' has arcs to entry block

is it cache related?

[bjodah]

Are you sure it's not cache related (we've changed gcc version here, right?) xref:
https://stackoverflow.com/a/41247820/790973

But then again, I also saw a bug report gcc which looks similar:
https://bugzilla.redhat.com/show_bug.cgi?id=1577508

[isuruf]

I reset the cache before the last job. (ccache does track the gcc version so that this shouldn't be an issue at all.)

[bjodah]

Didn't wait for Appyveyor to finish since tests passed for the previous commit, and the last commit only affected gcov.

[certik]

@bjodah thanks for fixing it. For reference, why does it now require to use NotImplementedError & instead of NotImplementedError? Is C++ now somehow returning a reference to the exception? I am confused.

[isuruf]

@certik,

We can either do,

try
{

}
catch (NotImplementedError e)
{

}

or

try
{

}
catch (NotImplementedError& e)
{

}

We had the first one, but we are catching a polymorphic type by value, (A type with a virtual function) which means we only have memory allocated for NotImplementedError type. If e was an object of a child class of NotImplementedError which had more data members than NotImplementedError, then this leads to crashes. (Or at least undefined behaviour)

[bjodah]

I simply followed @isurf's fix and extended it to the other exception types, since it's a macro I assumed it was simply used as a type specifier in the generated code. Looking at the implementation in catch (in our checked-in version) I see:

#define CHECK_THROWS_AS( expr, exceptionType ) INTERNAL_CATCH_THROWS_AS( "CHECK_THROWS_AS", exceptionType, Catch::ResultDisposition::ContinueOnFailure, expr )

and

#define INTERNAL_CATCH_THROWS_AS( macroName, exceptionType, resultDisposition, expr ) \
    do { \
        Catch::ResultBuilder __catchResult( macroName, CATCH_INTERNAL_LINEINFO, CATCH_INTERNAL_STRINGIFY(expr) ", " CATCH_INTERNAL_STRINGIFY(exceptionType), resultDisposition ); \
        if( __catchResult.allowThrows() ) \
            try { \
                static_cast<void>(expr); \
                __catchResult.captureResult( Catch::ResultWas::DidntThrowException ); \
            } \
            catch( exceptionType ) { \
                __catchResult.captureResult( Catch::ResultWas::Ok ); \
            } \
            catch( ... ) { \
                __catchResult.useActiveException( resultDisposition ); \
            } \
        else \
            __catchResult.captureResult( Catch::ResultWas::Ok ); \
        INTERNAL_CATCH_REACT( __catchResult ) \
    } while( Catch::alwaysFalse() )

so I guess it now catches the exception by reference

[certik]

I see. Yes, we have to use a reference for a polymorphic type. This looks almost like a bug in Catch itself, since it would be more logical (I would think) to simply write CHECK_THROWS_AS(c2->div(*c1), NotImplementedError); and the macro would add the & in there properly, since it doesn't make sense to call it without it. But perhaps some people prefer to use Catch without the & for non-polymorphic types.