Skip to content

Conversation

@amadio
Copy link
Member

@amadio amadio commented Jun 7, 2017

No description provided.

@phsft-bot
Copy link

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

const char * GSLMultiRootFinder::Name() const {
// get GSL name
return (fSolver != 0) ? fSolver->Name().c_str() : "";
return (fSolver != 0) ? fSolver->Name().c_str() : nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should not be needed? That address actually has static storage duration as per [lex.string]. Are you sure you're solving a problem in the current code version? (Note that we had a problem here because fSolver->Name() returned a std::string; it now returns a std::string&.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler is complaining about it, so it could be a problem. I just noticed that the problem is not about the empty string, though, but about fSolver->Name().c_str(). So, in hindsight, while my "fix" is probably pointless, it does seem like the issue with returning a local address still remains.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem of fSolver->Name()->c_str() should have been fixed by my commit of yesterday.
Strange the compiler is still complaining. Which version ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's GCC 7 complaining. The warning in CDash can be found here.

@Axel-Naumann
Copy link
Member

Right, it says:

/.../root/math/mathmore/src/GSLMultiRootFinder.cxx:161:42: note: declared here
     return (fSolver != 0) ? fSolver->Name().c_str() : "";

(And why can I not link to a warning in CDash?)

It was actually fixed today by

commit c9f996a7bd13f955eab41760c9839ffe697e750e
Author:     moneta <[email protected]>
AuthorDate: Tue Jun 6 12:05:47 2017 +0200
Commit:     moneta <[email protected]>
CommitDate: Wed Jun 7 15:17:33 2017 +0200

    Add test for multi root solver and test Name() method

(Yes, the log didn't really help in finding that commit...)

@amadio
Copy link
Member Author

amadio commented Jun 7, 2017

Ok, indeed I don't see this on master anymore. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants