Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Make SignalHandling supports different printer #238

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

Open
z0ow wants to merge 1 commit into bombela:master
base: master
Choose a base branch
Loading
from z0ow:master

Conversation

@z0ow
Copy link

@z0ow z0ow commented Aug 19, 2021

The backward::SignalHandling only supports outputting traces via backward::Printer takes a disadvantage to designers to customize their output style. This PR makes a derived class of backward::Printer can override print functions and adds class backward::SignalHandlingBase to allow designers to set their inherited printer as default.

The std::shared_ptr is brought into backward::SignalHandling to support polymorphism. This feature could make backward better if this change does not affect performance and compatibility. 🙏

FederAndInk, nmaludy, jspelletier, and wention reacted with thumbs up emoji
Copy link

nmaludy commented Dec 7, 2021

+1 would like to see this merged, i had a similar problem trying to customize the formatting of the SignalHandler output

Copy link

Trafo commented Nov 18, 2022

me too

Copy link
Owner

bombela commented Nov 18, 2022

In this current state this would break backward compatibility by requiring C++11. So at least a new major version release would be required. But it is surely possible to maintain compatibility.

The printer and signal handlers were meant to be examples to use as a starting point for your own customization. You can write your own, specific to your project(s) and use what you want from backward-cpp.

With insight I should have had those examples in a different file. I wanted backward-cpp to be a single file to copy around for easy installation.

With that said, many people have asked for this feature over time. Maybe we can have our cake and eat it too.

Copy link

Any updates?
I was able to create a custom SignalHandling class, but to achieve this I had to copy more than 300 lines of code from the example. If only we could inherit from the base class and override the functions this could be achieved in only a few lines of code.

Copy link
Owner

bombela commented Jun 4, 2023

Why can't you inherit and override the base class?

Copy link

I tried inheriting from the base class and overriding only handleSignal (I'm on linux), and it didn't change anything. I had to copy also the constructor which in turn needed some private methods. I think this is because the function in not marked as virtual in the base class, but I am not very familiar with c++ so I could be completely wrong.

Anyway my objection remains, if the PR could be merged it would simplify customizing the printer a lot. I understand that SignalHandling was only meant as an example, but it implements a lot of things, reusing it would be nice

Copy link
Owner

bombela commented Jun 5, 2023

Dah; of course; the vtable is missing! In C++ the vtable pointer is within the object and I somehow forgot about that (I blame it on enjoying Rust too much ;)). I now remember that the destructor must be marked virtual too.

Instead of SignalHandlingBase::set_printer<Printer>() what about SignalHandling<P>(P&& printer)? That is, the signalhandling is given a Printer object to consume. Instead of setting a pointer to a printer. It somehow strikes me as being harder to misuse. It means you cannot modify the Printer after the fact. But maybe this is a good thing?

Copy link

Are there any plans to have this merged? It would be really nice to have this functionality.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /