New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clean old style codes #1250

Merged
merged 3 commits into from Aug 16, 2018

Conversation

Projects
8 participants
@cwyyprog

cwyyprog commented Aug 13, 2018

fix annoying warnings when compile with gcc 8

@@ -66,7 +66,7 @@ namespace graphene { namespace db {
catch ( const fc::exception& e )
{
elog( "${e}", ("e",e.to_detail_string() ) );
throw; // maybe crash..
//throw; // maybe crash..
}

This comment has been minimized.

@oxarbitrage

oxarbitrage Aug 13, 2018

Member

i am not very sure if we can just remove the throw here or replace it with FC_THROW with message and remove elog.

This comment has been minimized.

@pmconrad

pmconrad Aug 13, 2018

As the comment indicates, the program crash at this point is deliberate. The problem is that the internal database may be in an inconsistent state at this point (i. e. the current session was only partially undone). The node cannot reliably continue to operate, so commenting this out is definitely wrong.

This comment has been minimized.

@cwyyprog

cwyyprog Aug 13, 2018

So, the node should exit immediately, but why not just use std::exit?

This comment has been minimized.

@jmjatlanta

jmjatlanta Aug 13, 2018

there is a (slight) chance that this throw does not actually end the program, and it has to do with stack unwinding. I believe that is why the comment reads "maybe crash..."

See https://akrzemi1.wordpress.com/2011/09/21/destructors-that-throw/

More thought should be put into what should happen in this case.

This comment has been minimized.

@clockworkgr

clockworkgr Aug 13, 2018

Member

anything commented "Maybe crash..." is something worth looking into imho :D

This comment has been minimized.

@jmjatlanta

jmjatlanta Aug 13, 2018

I know GCC 7.3.0 gives a warning about throw within a destructor. That's the main reason I created issue #1246

This comment has been minimized.

@pmconrad

pmconrad Aug 14, 2018

Hm, according to the article referenced above, the destructor is implicitly declared as noexcept, and should therefore always terminate the program when it throws.
Termination is what we want here, because the database is inconsistent and we cannot repair it.

This comment has been minimized.

@cwyyprog

cwyyprog Aug 14, 2018

The GCC 8 says throw in destructor will terminate the program, and that is now what we wanted, but why not exit the program in a normal way instead of throwing an exception implicitly? can we replace the throw with std::exit()?

This comment has been minimized.

@cogutvalera

cogutvalera Aug 14, 2018

Member

we cannot exit, terminate in libraries, in libraries we can only return or throw

This comment has been minimized.

@cogutvalera

cogutvalera Aug 14, 2018

Member

look please #1050 why we cannot exit or terminate in libraries

cwyyprog added some commits Aug 14, 2018

Stay unchanged for agreement
As no agreement is reached, undo the modifying
Merge pull request #1 from cwyyprog/cwyyprog-patch-1
Stay unchanged for agreement
@oxarbitrage

ok with me now the throw change is reversed. thanks.

@abitmore abitmore added this to In progress in Feature release (201810) via automation Aug 16, 2018

@abitmore abitmore merged commit 559db7b into bitshares:develop Aug 16, 2018

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature release (201810) automation moved this from In progress to Done Aug 16, 2018

@ryanRfox

This comment has been minimized.

Member

ryanRfox commented Aug 20, 2018

@cwyyprog Thank you for the contributions to BitShares development. I hope to connect with you directly about the Community Claims compensation available through the BitShares Core Team Worker Proposal. You may reach me on Telegram or WeChat: ryanRfox

Thank you.

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