Skip to content
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.

Sign up for GitHub

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
Merged

clean old style codes #1250

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c90bd3
clean old style codes
Aug 13, 2018
c064d19
Stay unchanged for agreement
Aug 14, 2018
14e6cee
Merge pull request #1 from cwyyprog/cwyyprog-patch-1
Aug 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libraries/chain/protocol/fee_schedule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ namespace graphene { namespace chain {
{
try {
return op.calculate_fee( param.get<OpType>() ).value;
} catch (fc::assert_exception e) {
} catch (fc::assert_exception& e) {
fee_parameters params; params.set_which(current_op);
auto itr = param.parameters.find(params);
if( itr != param.parameters.end() ) params = *itr;
Expand Down
2 changes: 1 addition & 1 deletion libraries/db/include/graphene/db/generic_index.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ namespace graphene { namespace chain {
[&m, &exc](ObjectType& o) mutable {
try {
m(o);
} catch (fc::exception e) {
} catch (fc::exception& e) {
exc = std::current_exception();
elog("Exception while modifying object: ${e} -- object may be corrupted",
("e", e));
Expand Down
2 changes: 1 addition & 1 deletion libraries/db/include/graphene/db/undo_database.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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..
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jmjatlanta jmjatlanta Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@pmconrad pmconrad Aug 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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()?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

if( _disable_on_exit ) _db.disable();
}
Expand Down
4 changes: 2 additions & 2 deletions libraries/plugins/market_history/market_history_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,12 @@ struct operation_process_fill_order
db.modify( *bucket_itr, [&]( bucket_object& b ){
try {
b.base_volume += trade_price.base.amount;
} catch( fc::overflow_exception ) {
} catch( fc::overflow_exception& ) {
b.base_volume = std::numeric_limits<int64_t>::max();
}
try {
b.quote_volume += trade_price.quote.amount;
} catch( fc::overflow_exception ) {
} catch( fc::overflow_exception& ) {
b.quote_volume = std::numeric_limits<int64_t>::max();
}
b.close_base = fill_price.base.amount;
Expand Down