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

😍 Add module std #98

Merged
merged 1 commit into from
May 23, 2023
Merged

😍 Add module std #98

merged 1 commit into from
May 23, 2023

Conversation

aaronmondal
Copy link
Contributor

@aaronmondal aaronmondal commented Apr 20, 2023

See https://reviews.llvm.org/D144994 for progress the std module patch.

Copy link

@mordante mordante left a comment

Choose a reason for hiding this comment

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

Very cool to see the modules patch can be used in Bazel!

"locale",
"map",
"mdspan",
# "memory", # Bugged?

Choose a reason for hiding this comment

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

FYI I've checked my libc++ patch and the export of std::atomic is indeed incorrect. We have not implemented atomiic smart pointers yet, that will export std::atomic.

I had already fixed this locally, but I want to do other fixes before uploading a new revision in Phabricator.

@aaronmondal
Copy link
Contributor Author

aaronmondal commented Apr 24, 2023

All preparations are now in rules_ll main. We could merge this pretty much at any point now. Technically we could add this without the patch fully working since it doesn't break existing code that doesn't import std;. I'd be fine with that, but some symbols not working might be confusing to users.

It's hard to predict how long it'll take until upstream can merge this, as it needs to actually work to be committed into llvm main and the remaining issues seem to be the kinds of issues that one does not simply fix in a few minutes 😅

We should also keep in mind that modules don't work with remote execution ATM. This currently doesn't affect users that don't use modules, but if we build modules as part of the default toolchains it will become noticeable to everyone. We already have safeguards against this though, and remote execution is disabled for modules anyways.

@SpamDoodler @jaroeichler WDYT? Do you want to play around with this as "highly experimental" feature in rules_ll main or would you prefer to wait until things are more stable?

IMO, since the patch almost fully works, a compelling option would be to wait for @mordante's next revision mentioned above and then add this to rules_ll main (and probably bump the llvm commit to include all separate fixes). This way, users can preview it and "help" find bugs 🤣 We could make preview release and immediately create another release as soon as a fully working version is in llvm main.

@aaronmondal
Copy link
Contributor Author

Pseudo-rebased the patch against the new llvm version. Modified the patch so that it only contains bugfixes and the cppm files. Apart from a buggy include this everything compiles now.

@aaronmondal aaronmondal marked this pull request as ready for review May 23, 2023 17:55
It's finally here. Dayyyyyuuuummm I'm excited~
@aaronmondal aaronmondal merged commit 285e06f into eomii:main May 23, 2023
3 checks passed
@mordante
Copy link

Nice to see this landed, I'm really curious to see how this works in practice.

@aaronmondal
Copy link
Contributor Author

aaronmondal commented May 23, 2023

@mordante Thank you so much for your work on this, and also huge thanks to @ChuanqiXu9 who made it possible to build these targets with fully explicit dependencies ❤️

aec847b updates some examples to use the new std module.

I'll cut a new release soon to make playing around with this easier for users.

On the "regular" C++ side things work pretty much flawlessly so far (apart from some minor inconvenience warnings llvm/llvm-project#62844). Heterogeneous toolchains probably won't be able to use the std module for a while, but they can't use regular modules either 😅

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