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

ref: Add File/FunctionIterator and lifetimes to DebugSession #279

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Oct 8, 2020

continuation of #277

@Swatinem Swatinem changed the title WIP: Add File/FunctionIterator and lifetimes to DebugSession ref: Add File/FunctionIterator and lifetimes to DebugSession Oct 9, 2020
@Swatinem Swatinem marked this pull request as ready for review October 9, 2020 11:30
@Swatinem Swatinem requested a review from a team October 9, 2020 11:30
@@ -602,21 +602,27 @@ pub type DynIterator<'a, T> = Box<dyn Iterator<Item = T> + 'a>;
/// - Read headers of compilation units (compilands) to resolve cross-unit references.
///
/// [`ObjectLike::debug_session`]: trait.ObjectLike.html#tymethod.debug_session
pub trait DebugSession {
pub trait DebugSession<'session> {
Copy link
Member

Choose a reason for hiding this comment

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

Usually, we don't need the 'data lifetime here, but do we need the 'object one?

Copy link
Member

Choose a reason for hiding this comment

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

On taking a second look, I think we can get away without, since we can still bind it in the associated types on the concrete implementation. In that case, 'object: 'session would need to be fulfilled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think that should be enough.

Copy link
Member

Choose a reason for hiding this comment

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

What if we do the same for the ObjectLike trait, to only have the 'object lifetime there?

Copy link
Member Author

Choose a reason for hiding this comment

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

To return references tied to the objects lifetime?

Copy link
Member

Choose a reason for hiding this comment

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

Correct. This would give us the benefit of not having to clone in PdbObject::symbols, and I think that use cases where you need the 'data life time but cannot hold the objects are very scarce in hindsight.

* master:
  fix: Implement new clippy advice (#280)
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Merged the clippy fix from master. This is g2g now.

Comment on lines +1084 to +1085
type FunctionIterator = BreakpadFunctionIterator<'session>;
type FileIterator = BreakpadFileIterator<'session>;
Copy link
Member

Choose a reason for hiding this comment

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

Just as a thought experiment. You can make these two 'data, I think. You may need 'data: 'session for this, but then you're most faithful to the lifetime. The downside of that approach would be that if you need to constrain the lifetime in future, you need to "break" the trait implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried in an earlier experiment. Apparently one or two of the concrete implementations returns data tied to the session. Maybe we can revisit at a later point.

@Swatinem Swatinem merged commit 744e4ad into master Oct 13, 2020
@Swatinem Swatinem deleted the fix/session-lifetimes-2 branch October 13, 2020 10:31
jan-auer added a commit that referenced this pull request Oct 19, 2020
* master:
  ref: Add File/FunctionIterator and lifetimes to DebugSession (#279)
  fix: Implement new clippy advice (#280)
  fix: Add a SymbolIterator and Lifetimes to ObjectLike trait (#277)
jan-auer added a commit that referenced this pull request Nov 17, 2020
* master: (22 commits)
  fix(debuginfo): Update dmsort to 1.0.1 to avoid panic due to UB (#287)
  ci: Use GHA instead of zeus (#286)
  ref: Introduce explicit NameMangling and better DemangleOptions (#275)
  meta: Bump all semver-major dependencies (#283)
  feat(demangle): Update swift demangler to 5.3 (#282)
  ref: Add File/FunctionIterator and lifetimes to DebugSession (#279)
  fix: Implement new clippy advice (#280)
  fix: Add a SymbolIterator and Lifetimes to ObjectLike trait (#277)
  ci: Switch to GitHub Actions (#273)
  ref: Introduce feature flags for demangling languages (#274)
  ref(common): Change InstructionInfo setters to Option (#272)
  ref: Remove all deprecated items (#271)
  ref: Replace failure with std::error::Error (#264)
  ref: Remove deprecated proguard support (#267)
  build: Reorganize the workspace (#266)
  build(unreal): Rename with-serde to serde (#265)
  fix(debuginfo): Detect mangled anonymous namespaces in PDB inlinees (#261)
  release: 7.5.0
  meta: Update changelog for 7.5.0
  feat: Unsafe transmute for PDB symbols (#258)
  ...
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