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 llvm_struct_type, llvm_packed_struct_type, and llvm_alias #1088

Merged
merged 3 commits into from
Feb 23, 2021
Merged

Conversation

RyanGlScott
Copy link
Contributor

@RyanGlScott RyanGlScott commented Feb 22, 2021

This PR contains two commits that fix #1085. The first commit, which introduces llvm_struct_type and llvm_packed_struct_type, is straightforward. The second commit, which introduces llvm_alias and makes llvm_struct a synonym for llvm_alias, is slightly more involved, as it replaces all uses of llvm_struct with llvm_alias. llvm_struct is not deprecated in favor of llvm_alias yet, but we may wish to do so in the future.

Copy link
Contributor

@brianhuffman brianhuffman left a comment

Choose a reason for hiding this comment

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

This all looks good to me.

Just to be clear: This PR leaves the lifecycle status of llvm_struct as Current; it has not yet been officially changed to Deprecated, which would restrict the use of llvm_struct without the enable_deprecated command. I think this is the right choice at the current time; we can officially mark the command as Deprecated after the next release version of saw.

@RyanGlScott
Copy link
Contributor Author

RyanGlScott commented Feb 22, 2021

Oh, I wasn't aware that there was a separate Deprecated constructor! I just looked at the crucible_* functions for inspiration, and since they all use Current, I went with that.

I worry now that I might be conveying mixed messages, since I explicitly use the word "deprecated" in the changelog. What is a better way to describe the new status of llvm_struct?

This introduces the `llvm_alias` function and makes `llvm_struct` a synonym for
`llvm_alias`. For now, `llvm_struct` continues to be available without a
deprecation notice. In the future, we may want to deprecate `llvm_struct` in
favor of `llvm_alias`, as having two separate functions named `llvm_struct` and
`llvm_struct_type` could invite confusion. See the discussion in #1085.
@RyanGlScott RyanGlScott changed the title Add llvm_struct_type/llvm_packed_struct_type, deprecate llvm_struct for llvm_alias Add llvm_struct_type, llvm_packed_struct_type, and llvm_alias Feb 22, 2021
@RyanGlScott
Copy link
Contributor Author

What is a better way to describe the new status of llvm_struct?

For inspiration, I decided to see how the changelog handled the renaming from crucible_* to llvm_*. There, the changelog manages to avoid mentioning the word "deprecate" by simply stating that the crucible_* functions will continue to be available for the time being. That's a nice rhetorical trick, so I have pushed an update which adopts similar wording for llvm_struct/llvm_alias.

I think this is ready to go now, so I'll optimistically apply the ready-to-merge label.

@RyanGlScott RyanGlScott added the PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run label Feb 22, 2021
@mergify mergify bot merged commit d620cca into master Feb 23, 2021
@mergify mergify bot deleted the T1085 branch February 23, 2021 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add saw-script llvm_struct_type function
2 participants