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.

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

Issue 727b - A second option #33

Closed
wants to merge 13 commits into from

Conversation

jmjatlanta
Copy link

@jmjatlanta jmjatlanta commented Mar 26, 2018

This is another option for adding stacktrace to bitshares. Instead of porting fc/stacktrace from steem, I pulled stacktrace from the 1.65 version of boost.

Boost has more lines of code, but code quality appears better, and is probably tested to a higher degree (more eyes on it).

@jmjatlanta jmjatlanta changed the title Issue 727b Issue 727b - A second option Mar 26, 2018
@jmjatlanta jmjatlanta mentioned this pull request Apr 2, 2018
Copy link

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Much better than #23 IMO.
Question: is it important for us to have stacktraces for pre-1.65 boost?
I think it isn't, and therefore I'd propose to not copy the old boost code but instead rebase on #36 and detect boost version at compile-time and replace the stacktrace stuff with NOPs if boost is too old.

@jmjatlanta
Copy link
Author

Question: is it important for us to have stacktraces for pre-1.65 boost?
I think it isn't,

I agree. If everyone else does, I'll rebase on the Boost 1.66 stuff and a skip the stacktrace logic pre boost 1.65.

@pmconrad
Copy link

pmconrad commented Apr 8, 2018

@abitmore ?

@abitmore
Copy link
Member

abitmore commented Apr 8, 2018

Question: is it important for us to have stacktraces for pre-1.65 boost?
I think it isn't,

I agree. If everyone else does, I'll rebase on the Boost 1.66 stuff and a skip the stacktrace logic pre boost 1.65.

I agree.

@jmjatlanta
Copy link
Author

jmjatlanta commented Apr 9, 2018

The rebase included commits that muddy the waters. I have created a new PR.
#38

@jmjatlanta jmjatlanta closed this Apr 9, 2018
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.

3 participants