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

[macro] apply @:native for extern enum #11636

Merged
merged 7 commits into from
Apr 18, 2024
Merged

[macro] apply @:native for extern enum #11636

merged 7 commits into from
Apr 18, 2024

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Apr 12, 2024

Closes #11631

See also 944d4fd / #11481

@Simn
Copy link
Member

Simn commented Apr 12, 2024

Making this conditional seems strange if the real filters don't do the same. What's the rationale here?

@kLabz
Copy link
Contributor Author

kLabz commented Apr 12, 2024

I'm actually having doubts too.

Rationale is that eval target does apply @:native, which is used for mbedtls extern enums.

I want the same to happen with macro context, but it's a bit more involved than I first thought because macro context does have both externs for current targets and eval externs...

Edit:
Given that such externs would only work for std anyway (or maybe plugins? not sure that's still used tbh), we could use some trick there to identify "eval externs", but would be nice to do so in a not too hacky way

@kLabz
Copy link
Contributor Author

kLabz commented Apr 12, 2024

Well.. technically "target externs" should not be loaded in macro context, but I can still see some problematic edge cases.

I don't see any with only doing this for extern enum, though.

@Simn
Copy link
Member

Simn commented Apr 12, 2024

So what exactly breaks if we just apply this to everything?

@kLabz
Copy link
Contributor Author

kLabz commented Apr 12, 2024

#11481 does

@kLabz kLabz marked this pull request as draft April 12, 2024 07:01
@Simn
Copy link
Member

Simn commented Apr 12, 2024

Ah, I completely forgot about that. In that case I think this change is fine, though it makes me wonder if a variant of #11481 with enum abstracts could exist.

@kLabz
Copy link
Contributor Author

kLabz commented Apr 12, 2024

Turning into a draft while adding a test because it's not working properly for some reason atm 🤔

Edit: right, it's enum abstract, not enum...

@kLabz kLabz marked this pull request as ready for review April 12, 2024 07:46
@kLabz kLabz changed the title [eval] apply @:native for externs [eval] apply @:native for extern enum Apr 12, 2024
@kLabz kLabz changed the title [eval] apply @:native for extern enum [macro] apply @:native for extern enum Apr 12, 2024
@skial skial mentioned this pull request Apr 16, 2024
1 task
@kLabz kLabz merged commit ed138ae into development Apr 18, 2024
102 checks passed
@kLabz kLabz deleted the fix/eval-native branch May 28, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants