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.

Sign up for GitHub

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

Jump to bottom

[stdlib] Refactor external_call() and inline _LITRefPackHelper into VariadicPack #3632

Closed
wants to merge 29 commits into from
Closed

[stdlib] Refactor external_call() and inline _LITRefPackHelper into VariadicPack #3632

wants to merge 29 commits into from

Conversation

martinvuyk
Copy link
Contributor

@martinvuyk martinvuyk commented Oct 10, 2024 •
edited
Loading

Refactor external_call() and inline most of _LITRefPackHelper into VariadicPack, leave _LITRefPackHelper itself intact since I found no way to inline the AddressSpace parameter into VariadicPack.

@martinvuyk martinvuyk marked this pull request as ready for review October 10, 2024 12:59
@martinvuyk martinvuyk requested a review from a team as a code owner October 10, 2024 12:59
@martinvuyk
fix details
5ebfeff 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
Merge branch 'add-externalcall-variadicpack-overload' of github.com:m…
048985e 
…artinvuyk/mojo into add-externalcall-variadicpack-overload
@martinvuyk
fix details
d37c1d1 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix details
c1c37b5 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix details
bfbe7e6 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix details
47ff4ff 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix details
44bcb5b 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk martinvuyk mentioned this pull request Oct 10, 2024
Open
1 task
soraros
soraros reviewed Oct 10, 2024
View reviewed changes
stdlib/src/sys/ffi.mojo Outdated Show resolved Hide resolved
@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Oct 11, 2024
martinvuyk and others added 10 commits October 14, 2024 11:02
@martinvuyk
Merge remote-tracking branch 'upstream/nightly' into add-externalcall…
3a43fad 
…-variadicpack-overload
@martinvuyk
fix detail
675eefe 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
Merge branch 'nightly' into add-externalcall-variadicpack-overload
c014f1b
@martinvuyk
Merge remote-tracking branch 'upstream/nightly' into add-externalcall…
d9e879f 
…-variadicpack-overload
@martinvuyk
Merge branch 'add-externalcall-variadicpack-overload' of github.com:m…
6e44e10 
…artinvuyk/mojo into add-externalcall-variadicpack-overload
@martinvuyk
fix detail
bd5d75a 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
fix detail
c2269eb 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
Merge remote-tracking branch 'upstream/nightly' into add-externalcall…
d92fa04 
…-variadicpack-overload
@martinvuyk
fix details
636a466 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk
return _litrefpackhelper
cc56be4 
Signed-off-by: martinvuyk <[email protected]>
@martinvuyk martinvuyk requested a review from JoeLoser November 5, 2024 00:57
@JoeLoser
Copy link
Collaborator

JoeLoser commented Nov 5, 2024 •
edited
Loading

CC: @ConnorGray as you have an internal PR up for consolidating _LITRefPackHelper and VariadicPack from other work you were doing.

@ConnorGray
Copy link
Collaborator

!sync

@martinvuyk
Copy link
Contributor Author

@ConnorGray fixed all conflicts, this became mostly a cleanup PR

@ConnorGray
Copy link
Collaborator

ConnorGray commented Nov 7, 2024 •
edited
Loading

Hi Martin, I appreciate you taking the time to fix up the conflicts. However, as Joe mentioned, coincidentally I was finishing work in this same area recently, and ended up performing a similar combination of _LITRefPackHelper and VariadicPack (with the addition that, with some advise from Chris L., we realized we could remove the address space parameter entirely for the time being, allowing a full removal of _LITRefPackHelper).

I'm sorry for stepping on your toes a bit here, that's largely my fault for not being as aware of your prior work in this area as I should have been 😕

With that in mind, this PR still has some nice doc comment additions and improvements, so I'm going to proceed with fixing up the conflicts and making sure that work is still incorporated.

Thank you for all the work you've been doing cleaning up some of these warts in the standard library, it's not always the flashiest work, but very important for having a good foundation and keeping the stdlib healthy. My apologies again Martin 😌

@ConnorGray
Copy link
Collaborator

!sync

@JoeLoser
Copy link
Collaborator

JoeLoser commented Nov 22, 2024 •
edited
Loading

@martinvuyk do you mind rebasing this on top of @ConnorGray's recent changes in this area/the latest nightly? We'd love to incorporate the doc improvements here. Thanks!

@martinvuyk
Copy link
Contributor Author

@JoeLoser done

@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added the merged-internally Indicates that this pull request has been merged internally label Dec 19, 2024
@modularbot
Copy link
Collaborator

Landed in f075bf2! Thank you for your contribution 🎉

@modularbot modularbot added the merged-externally Merged externally in public mojo repo label Dec 21, 2024
modularbot added a commit that referenced this pull request Dec 21, 2024
@modularbot @martinvuyk
[External] [stdlib] Doc improvements and cleanups around low-level `V…
f075bf2 
…ariadicPack` use (#48996)

[External] [stdlib] Doc improvements and cleanups around low-level
VariadicPack use #48996

This includes changes from #3632, minus parts which ended
up
conflicting with internal changes doing the same refactoring.
Context:
#3632 (comment)

Original commit description:

[External] [stdlib] Refactor `external_call()` and inline
`_LITRefPackHelper` into `VariadicPack`

Refactor `external_call()` and inline most of `_LITRefPackHelper` into
`VariadicPack`, leave `_LITRefPackHelper` itself intact since I found no
way to inline the `AddressSpace` parameter into `VariadicPack`.

ORIGINAL_AUTHOR=martinvuyk
<[email protected]>
PUBLIC_PR_LINK=#3632

Co-authored-by: martinvuyk <[email protected]>
Closes #3632
MODULAR_ORIG_COMMIT_REV_ID: 8a1c1a5a92c6910039158108ab47046583ae9586
@modularbot modularbot closed this Dec 21, 2024
msaelices pushed a commit to msaelices/mojo that referenced this pull request Dec 22, 2024
@modularbot @martinvuyk @msaelices
[External] [stdlib] Doc improvements and cleanups around low-level `V…
b845157 
…ariadicPack` use (#48996)

[External] [stdlib] Doc improvements and cleanups around low-level
VariadicPack use #48996

This includes changes from modularml#3632, minus parts which ended
up
conflicting with internal changes doing the same refactoring.
Context:
modularml#3632 (comment)

Original commit description:

[External] [stdlib] Refactor `external_call()` and inline
`_LITRefPackHelper` into `VariadicPack`

Refactor `external_call()` and inline most of `_LITRefPackHelper` into
`VariadicPack`, leave `_LITRefPackHelper` itself intact since I found no
way to inline the `AddressSpace` parameter into `VariadicPack`.

ORIGINAL_AUTHOR=martinvuyk
<[email protected]>
PUBLIC_PR_LINK=modularml#3632

Co-authored-by: martinvuyk <[email protected]>
Closes modularml#3632
MODULAR_ORIG_COMMIT_REV_ID: 8a1c1a5a92c6910039158108ab47046583ae9586
@martinvuyk martinvuyk deleted the add-externalcall-variadicpack-overload branch December 22, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soraros soraros soraros left review comments

@JoeLoser JoeLoser Awaiting requested review from JoeLoser

Assignees

@ConnorGray ConnorGray

@JoeLoser JoeLoser

Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martinvuyk @JoeLoser @ConnorGray @modularbot @soraros