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

Deprecate llvm_struct #2183

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Deprecate llvm_struct #2183

wants to merge 1 commit into from

Conversation

sauclovian-g
Copy link
Contributor

Closes #2159.

@sauclovian-g
Deprecate llvm_struct.
acb5224 
Change references to either llvm_alias (which has the same type and does
the same thing) or llvm_struct_type (which is sometimes what people
referring to llvm_struct are actually trying to do).

Alas, the manual was wrong and cited llvm_struct when it wanted
llvm_struct_type...
@sauclovian-g
Copy link
Contributor Author

So, it seems a bunch of the s2n proofs use llvm_struct. Should we fix them, or put this off? (and if we put it off, what eventually changes? it seems unlikely we'll drop the s2n proofs entirely from the testing)

@RyanGlScott
Copy link
Contributor

While it would be nice to fix the s2n proofs to use llvm_alias instead of llvm_struct, I am rather surprised that it's strictly required here. Isn't the point of this PR to deprecate llvm_struct rather than to remove it outright? If llvm_struct is only deprecated, then why is code suddenly failing to run?

@sauclovian-g
Copy link
Contributor Author

Because when you mark something deprecated, you then have to use enable_deprecated to be allowed to see it. Which seems like the wrong thing, so maybe the first step is to rearrange the framework...

@RyanGlScott
Copy link
Contributor

Because when you mark something deprecated, you then have to use enable_deprecated to be allowed to see it.

Oh wow, I was blissfully ignorant of this. That certainly feels like a strange design—I would argue it makes much more sense for seeing deprecated functions to be opt-out rather than opt-in.

the first step is to rearrange the framework

I agree. Shall we open a separate issue about this? It feels as though we'd want disable_{experimental,deprecated} counterparts to the existing enable_{experimental,deprecated} commands, and then change the deprecated commands to be enabled by default.

@sauclovian-g
Copy link
Contributor Author

It's more complicated than that because I don't think we want the existing not-visible deprecated functions to come back to life.

Anyway I opened #2184 for this.

@sauclovian-g sauclovian-g mentioned this pull request Jan 16, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanGlScott RyanGlScott Awaiting requested review from RyanGlScott

@mtullsen mtullsen Awaiting requested review from mtullsen

@mccleeary-galois mccleeary-galois Awaiting requested review from mccleeary-galois

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate llvm_struct()
2 participants
@sauclovian-g @RyanGlScott