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

Hide implementations of complex data structures in SymtabAPI::Symtab #1531

Merged
merged 13 commits into from Sep 22, 2023

Conversation

hainest
Copy link
Contributor

@hainest hainest commented Sep 20, 2023

This allows us to more easily modify the parallelism around the big tables (e.g., indexed_modules) without bumping the ABI around, meaning we don't have to wait until a major release to make those changes. This also removes internal details like concurrent.h which puts TBB into the pubic API and the multiple Boost headers needed for multi_index.

Although ModRangeLookup* mod_lookup(); was a public member, it was never documented, and should never be called by a user. I'm not considering this an API-breaking change.

@hainest hainest added ABI-BREAKER This change alters the Dyninst public ABI code cleanup Bring the code up to modern standards or remove deprecated features Symtab This issue is directly related to SymtabAPI labels Sep 20, 2023
@hainest hainest requested a review from kupsch September 20, 2023 23:53
@hainest hainest self-assigned this Sep 20, 2023
@hainest
Copy link
Contributor Author

hainest commented Sep 21, 2023

This will have to wait until dyninst/testsuite#233 is merged.

Copy link
Contributor

@kupsch kupsch left a comment

Choose a reason for hiding this comment

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

Only suggestion: impl could be const

@@ -111,6 +91,9 @@ class SYMTAB_EXPORT Symtab : public LookupInterface,
friend class relocationEntry;
friend class Object;

// Hide implementation details that are complex or add large dependencies
std::unique_ptr<symtab_impl> impl;
Copy link
Contributor

Choose a reason for hiding this comment

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

const std::unique_ptr...? The impl never changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Symtab isn't moveable, that is a good idea.

@hainest hainest merged commit 9a88a32 into master Sep 22, 2023
4 checks passed
@hainest hainest deleted the thaines/symtab_impl_hiding branch September 22, 2023 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI-BREAKER This change alters the Dyninst public ABI code cleanup Bring the code up to modern standards or remove deprecated features Symtab This issue is directly related to SymtabAPI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants