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

Introduce TraceResolver::load_addresses() for raw address array #162

Merged
merged 1 commit into from
Apr 8, 2021
Merged

Introduce TraceResolver::load_addresses() for raw address array #162

merged 1 commit into from
Apr 8, 2021

Conversation

Gerold103
Copy link
Contributor

TraceResolver has a load_stacktrace() method accepting only an
object like StackTrace. That object is basically an array of
pointers.

This was not really convenient, because

  1. it required carrying of this StackTrace object from the moment
    of its creation to the load_stacktrace() invocation. Sometimes
    this is a long path. And that may break encapsulation, if a
    project, using Backward, does not want to include Backward
    header anywhere except for 1 or 2 C++ files, responsible for
    collection and resolution of addresses. So basically,
    load_stacktrace() prevented split of stack load and stack
    resolution into separate submodules of a project;

  2. that made impossible to use Backward to resolve a stack,
    collected not via Backward.

The patch introduces a new method load_addresses(), accepting an
array of addresses. That solves both problems.

Alongside, to avoid increase of code duplication by empty
load_addresses() methods in the classes where they are not needed,
now both load_stacktrace() and load_addresses() are in one base
class, used by all resolvers. load_addresses() is a virtual
method, which does nothing, except for a couple of classes, where
it is overridden as backtrace_symbols(). load_stacktrace() just
calls load_addresses() and doesn't need to be defined everywhere
again and again.

TraceResolver has a load_stacktrace() method accepting only an
object like StackTrace. That object is basically an array of
pointers.

This was not really convenient, because

1) it required carrying of this StackTrace object from the moment
  of its creation to the load_stacktrace() invocation. Sometimes
  this is a long path. And that may break encapsulation, if a
  project, using Backward, does not want to include Backward
  header anywhere except for 1 or 2 C++ files, responsible for
  collection and resolution of addresses. So basically,
  load_stacktrace() prevented split of stack load and stack
  resolution into separate submodules of a project;

2) that made impossible to use Backward to resolve a stack,
  collected not via Backward.

The patch introduces a new method load_addresses(), accepting an
array of addresses. That solves both problems.

Alongside, to avoid increase of code duplication by empty
load_addresses() methods in the classes where they are not needed,
now both load_stacktrace() and load_addresses() are in one base
class, used by all resolvers. load_addresses() is a virtual
method, which does nothing, except for a couple of classes, where
it is overridden as backtrace_symbols(). load_stacktrace() just
calls load_addresses() and doesn't need to be defined everywhere
again and again.
@Gerold103
Copy link
Contributor Author

Any chances on getting this merged someday?

1 similar comment
@Gerold103
Copy link
Contributor Author

Any chances on getting this merged someday?

@Gerold103
Copy link
Contributor Author

@bombela ping, what can be done to speed up the review and merge?

1 similar comment
@Gerold103
Copy link
Contributor Author

@bombela ping, what can be done to speed up the review and merge?

@bombela
Copy link
Owner

bombela commented Apr 8, 2021

Sorry, I kept pushing back.

Does the code still work on C++98? I see the use of override which I think was introduced in C++11?

@Gerold103
Copy link
Contributor Author

Hm. I didn't check, but I see it is used in the master branch too. I assumed it is ok to use it then.

@bombela
Copy link
Owner

bombela commented Apr 8, 2021

Ahah indeed, there is a macro making it a no-op!

@bombela bombela merged commit 6313577 into bombela:master Apr 8, 2021
@Gerold103
Copy link
Contributor Author

Great, thanks for the merge!

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.

None yet

2 participants