Skip to content

Added Transfer Manager 2.0 public API types (Phase 1) - #3843

Closed
narahavi wants to merge 2 commits into
mainfrom
tmNew
Closed

Added Transfer Manager 2.0 public API types (Phase 1)#3843
narahavi wants to merge 2 commits into
mainfrom
tmNew

Conversation

@narahavi

@narahavi narahavi commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Issue #, if available:

Description of changes:

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Header files should be in include/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You should use #pragma once like what you did above

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

// Created by Narahari, Vinay on 6/5/26.
yeahhhh, i dont think we want this comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this an empty class? if we need to add members in the future we should make this into a pimpl

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we should add IsSuccess()/GetError() on the response to let customer know whether their transfer failed or succeeded

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

TM v1 uses a private constructor and static shared_ptr<TransferManager> Create() factory, which enables shared_from_this internally to prevent destruction while async CRT callbacks are in flight. Making this constructor public and deleting moves locks you out of that pattern later without a breaking API change. We should use a factory here

@narahavi narahavi closed this Jun 10, 2026
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.

2 participants