Added a GCC 6 Build to Travis by ShikharJ · Pull Request #1142 · symengine/symengine

ShikharJ · 2016-12-02T16:08:52Z

Relevant: #1134
@isuruf @certik -Wno-error=terminate has been added for now. What should be our approach? Should we modify the teuchos library code or rather suppress the -werror=terminate flag?

certik · 2016-12-02T16:59:35Z

@ShikharJ thanks for looking into this. What is the issue with the Wno-error=terminate --- what happens if you don't use it? I read online, that it terminates the program if there is an exception thrown? So it means that by default, you can't use exceptions? That seems weird to me, perhaps I am misunderstanding.

Why is the indentation fix needed in catch.hpp?

The fix in symengine_assert.h seems fine --- for reference, what is the error raised by gcc there?

ShikharJ · 2016-12-02T17:11:30Z

@certik Setting up a GCC 6 build initially gave this result:
https://travis-ci.org/ShikharJ/symengine/jobs/180008838 which is currently being suppressed by -Wno-error=terminate right now. Actually, -werror=terminate flag is set by default in GCC 6, which raises an error whenever it detects a possibility of an exception being thrown out of a destructor. (GCC 6 defaults to C++14 if I am not wrong, and destructors are noexcept by default since C++11).
After suppressing that, the next issue faced was this:
https://travis-ci.org/ShikharJ/symengine/jobs/180661418 that required the change in symengine_assert.h
The last error raised was this:
https://travis-ci.org/ShikharJ/symengine/jobs/180666131
which checks for indentation problems using the flag -Werror=misleading-indentation.

certik · 2016-12-02T17:29:42Z

Ok, so regarding the throwing in destructors, that's a bug in Teuchos, that should be possible to fix by:

commit 490b9391f616e42814ba2428e277e3b50d301dd1
Author: Ondřej Čertík <ondrej.certik@gmail.com>
Date:   Fri Dec 2 10:27:06 2016 -0700

    Allow the destructor to throw exceptions

diff --git a/symengine/utilities/teuchos/Teuchos_RCPNode.cpp b/symengine/utilities/teuchos/Teuchos_RCPNode.cpp
index 20afd2c..8adee12 100644
--- a/symengine/utilities/teuchos/Teuchos_RCPNode.cpp
+++ b/symengine/utilities/teuchos/Teuchos_RCPNode.cpp
@@ -624,7 +624,7 @@ ActiveRCPNodesSetup::ActiveRCPNodesSetup()
 }
 
 
-ActiveRCPNodesSetup::~ActiveRCPNodesSetup()
+ActiveRCPNodesSetup::~ActiveRCPNodesSetup() noexcept(false)
 {
 #ifdef TEUCHOS_SHOW_ACTIVE_REFCOUNTPTR_NODE_TRACE
   std::cerr << "\nCalled ActiveRCPNodesSetup::~ActiveRCPNodesSetup() : count = " << count_ << "\n";
diff --git a/symengine/utilities/teuchos/Teuchos_RCPNode.hpp b/symengine/utilities/teuchos/Teuchos_RCPNode.hpp
index 775e45b..b6aa97c 100644
--- a/symengine/utilities/teuchos/Teuchos_RCPNode.hpp
+++ b/symengine/utilities/teuchos/Teuchos_RCPNode.hpp
@@ -649,7 +649,7 @@ public:
   /** \brief . */
   ActiveRCPNodesSetup();
   /** \brief . */
-  ~ActiveRCPNodesSetup();
+  ~ActiveRCPNodesSetup() noexcept(false);
   /** \brief . */
   void foo();
 private:

certik · 2016-12-02T17:33:00Z

The other changes are good, thanks for the clarification. The noexcept thing should fix it, so you can then remove the -Wno-error=terminate flag. Indeed, by default destructors cannot raise exceptions.

One thing that I don't know is if this change wouldn't propagate to all destructors of classes that use RCP. I hope not, but if that's the case, then we need to think of another solution. Let me know.

ShikharJ · 2016-12-02T17:33:45Z

I'll keep you posted. Thanks!
@certik
On another note, I am unable to grab this commit from your repo. Can I replicate this in mine?

ShikharJ · 2016-12-02T18:25:06Z

@certik The results that I got on a trial branch using the above patch and removing -Wno-error=terminate
https://travis-ci.org/ShikharJ/symengine/jobs/180780650
The problem is that the above patch cannot be applied to ~RCPNodetmpl() destructor as it leads to this:

[  1%] Building CXX object symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_dyn_cast.cpp.o
[  2%] Building CXX object symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_Ptr.cpp.o
[  2%] Building CXX object symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_RCPNode.cpp.o
[  3%] Building CXX object symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_TestForException.cpp.o
[  3%] Building CXX object symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_TypeNameTraits.cpp.o
[  4%] Building CXX object symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_stacktrace.cpp.o
In file included from /home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCPDecl.hpp:51:0,
                 from /home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCP.hpp:58,
                 from /home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_stacktrace.cpp:31:
/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCPNode.hpp: In instantiation of ‘class Teuchos::RCPNodeTmpl<{anonymous}::StacktraceAddresses, Teuchos::DeallocDelete<{anonymous}::StacktraceAddresses> >’:
/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCP.hpp:93:86:   required from ‘Teuchos::RCPNode* Teuchos::RCP_createNewRCPNodeRawPtr(T*, bool) [with T = {anonymous}::StacktraceAddresses]’
/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCP.hpp:210:65:   required from ‘Teuchos::RCP<T>::RCP(T*, bool) [with T = {anonymous}::StacktraceAddresses]’
/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCP.hpp:574:28:   required from ‘Teuchos::RCP<T1> Teuchos::rcp(T*, bool) [with T = {anonymous}::StacktraceAddresses]’
/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_stacktrace.cpp:469:31:   required from here
/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCPNode.hpp:513:3: error: looser throw specifier for ‘Teuchos::RCPNodeTmpl<T, Dealloc_T>::~RCPNodeTmpl() noexcept (false) [with T = {anonymous}::StacktraceAddresses; Dealloc_T = Teuchos::DeallocDelete<{anonymous}::StacktraceAddresses>]’
   ~RCPNodeTmpl() noexcept(false)
   ^
/home/shikhar/symengine/symengine/utilities/teuchos/Teuchos_RCPNode.hpp:156:11: error:   overriding ‘virtual Teuchos::RCPNode::~RCPNode() noexcept’
   virtual ~RCPNode()
           ^
symengine/utilities/teuchos/CMakeFiles/teuchos.dir/build.make:182: recipe for target 'symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_stacktrace.cpp.o' failed
make[2]: *** [symengine/utilities/teuchos/CMakeFiles/teuchos.dir/Teuchos_stacktrace.cpp.o] Error 1
CMakeFiles/Makefile2:313: recipe for target 'symengine/utilities/teuchos/CMakeFiles/teuchos.dir/all' failed
make[1]: *** [symengine/utilities/teuchos/CMakeFiles/teuchos.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

I am currently trying a noexcept(false) on ~RCPNode() destructor to see if it works.

ShikharJ · 2016-12-02T18:47:06Z

@certik @isuruf Applying virtual ~RCPNode() noexcept(false) worked. So I guess we are left with three choices:

Suppress werror=terminate using -Wno-error=terminate and only issue warnings in GCC 6 build (no changes to Teuchos). Though I must say that the warnings make the output size really huge.
Apply virtual ~RCPNode() noexcept(false), in addition to the above patch by @certik, and exclude the Teuchos library as it is. (Teuchos library is the only place where the errors were raised, the code compiles cleanly otherwise)
Work on a major overhaul of Teuchos library code.

certik · 2016-12-02T19:30:55Z

@ShikharJ when you use virtual ~RCPNode() noexcept(false), then everything compiles? What do you mean by excluding Teuchos?

I suggest we do not use -Wno-error=terminate at all, and fix Teuchos if needed. It shouldn't be a major overhaul. As a last resort, we can also make Teuchos not to raise exceptions, and just terminate the program. But let's first understand the problem. (Teuchos is only used in Debug mode, when you want to have a loud error when something goes wrong, so even terminating the program is fine.)

ShikharJ · 2016-12-03T05:05:15Z

@certik What I meant to say was that changing virtual ~RCPNode() to virtual ~RCPNode() noexcept(false) and applying the above patch provided by you leads to a clean compilation on the GCC 6 build. The Teuchos library is the only place where the werror=terminate flag sets off errors. (That's what I meant by "excluding Teuchos").
Also, in a number of places where contributors have had this problem, some of them applied the noexcept workaround and some replaced the throw statement altogether.

ShikharJ · 2016-12-03T11:28:33Z

@certik @isuruf A review?

isuruf · 2016-12-03T16:14:56Z

symengine/utilities/teuchos/Teuchos_RCPNode.hpp

    }
  /** \brief . */
-  virtual ~RCPNode()
+  virtual ~RCPNode() noexcept(false)


Can you wrap noexcept(false) in a #ifdef TECUHOS_DEBUG block?

certik

The PR looks very nice now, I just left some more simplifications to do.

certik · 2016-12-04T03:51:32Z

symengine/utilities/teuchos/Teuchos_RCPNode.hpp

      if(extra_data_map_)
        delete extra_data_map_;
    }
+#endif


I would put it like this:

virtual ~RCPNode() #ifdef TEUCHOS_DEBUG noexcept(false) #endif { ... }

certik · 2016-12-04T03:52:38Z

symengine/utilities/teuchos/Teuchos_RCPNode.cpp

+  }
+}
+#else
 ActiveRCPNodesSetup::~ActiveRCPNodesSetup()


Just like I wrote below, just do:

ActiveRCPNodesSetup::~ActiveRCPNodesSetup() #ifdef TEUCHOS_DEBUG noexcept(false) #endif { ... }

I'll make the changes. Thanks @certik. This is a nice implementation.

isuruf · 2016-12-05T08:37:48Z

.travis.yml

          - binutils-dev
          - g++-5
          - clang-format-3.7
+    - env: BUILD_TYPE="Debug" WITH_BFD="yes" WITH_GCC_6="yes" WITH_COVERAGE="yes"


Can you remove WITH_COVERAGE=yes here, as running coverage for gcc-6 is not different from gcc-5

isuruf · 2016-12-05T08:39:00Z

symengine/utilities/teuchos/Teuchos_RCPNode.hpp

+  ~ActiveRCPNodesSetup() noexcept(false);
+#else
  ~ActiveRCPNodesSetup();
+#endif


Can you make this consistent with the others by wrapping only noexcept(false)?

Since you need a ; this is fine.

isuruf · 2016-12-05T08:52:34Z

symengine/utilities/teuchos/Teuchos_RCPNode.hpp

+  ~ActiveRCPNodesSetup() noexcept(false);
+#else
  ~ActiveRCPNodesSetup();
+#endif


Since you need a ; this is fine.

isuruf · 2016-12-05T08:53:00Z

symengine/utilities/teuchos/Teuchos_RCPNode.hpp

  /** \brief . */
  virtual ~RCPNode()
+#ifdef TEUCHOS_DEBUG
+  noexcept(false)


Can you add 4 spaces before noexcept?

isuruf · 2016-12-05T08:53:13Z

symengine/utilities/teuchos/Teuchos_RCPNode.cpp


 ActiveRCPNodesSetup::~ActiveRCPNodesSetup()
+#ifdef TEUCHOS_DEBUG
+noexcept(false)


Can you add 4 spaces before noexcept?

certik · 2016-12-05T15:54:30Z

That looks good to me now. It looks like just a simple change was needed in Teuchos. @isuruf are you ok with that as well?

ShikharJ force-pushed the Issue1134 branch 2 times, most recently from b37be37 to 38960ef Compare December 3, 2016 10:14

isuruf reviewed Dec 3, 2016

View reviewed changes

certik requested changes Dec 4, 2016

View reviewed changes

ShikharJ force-pushed the Issue1134 branch 2 times, most recently from 27d4555 to d21a1e6 Compare December 4, 2016 12:30

isuruf requested changes Dec 5, 2016

View reviewed changes

isuruf reviewed Dec 5, 2016

View reviewed changes

ShikharJ and others added 6 commits December 5, 2016 14:39

Added GCC 6 Build to Travis

ad4c6de

Condition Comparison Fix in symengine_assert.h

19ea746

Indentation Fix in catch.hpp

d42081b

Added noexcept(false) in ~RCPNode() and ~ActiveRCPNodesSetup()

1b3249f

Wrap Up for noexcept(false) clauses

6be1f15

Removed WITH_COVERAGE=yes and Added Indentation to noexcept(false)

d13ca93

ShikharJ force-pushed the Issue1134 branch from d21a1e6 to d13ca93 Compare December 5, 2016 09:11

certik approved these changes Dec 5, 2016

View reviewed changes

isuruf approved these changes Dec 5, 2016

View reviewed changes

certik merged commit 970f2fd into symengine:master Dec 5, 2016

ShikharJ deleted the Issue1134 branch December 5, 2016 17:28

isuruf mentioned this pull request Dec 6, 2016

Try a gcc 6 build in travis-ci #1134

Closed

Conversation

ShikharJ commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Dec 2, 2016

Uh oh!

ShikharJ commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Dec 2, 2016

Uh oh!

certik commented Dec 2, 2016

Uh oh!

ShikharJ commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShikharJ commented Dec 2, 2016

Uh oh!

ShikharJ commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

certik commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShikharJ commented Dec 3, 2016

Uh oh!

ShikharJ commented Dec 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

certik commented Dec 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShikharJ commented Dec 2, 2016 •

edited

Loading

ShikharJ commented Dec 2, 2016 •

edited

Loading

ShikharJ commented Dec 2, 2016 •

edited

Loading

ShikharJ commented Dec 2, 2016 •

edited

Loading

certik commented Dec 2, 2016 •

edited

Loading