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

refactor: use Tuple instead of Struct in saw-remote-api backend #1162

Merged
merged 2 commits into from
Apr 2, 2021

Conversation

pnwamk
Copy link
Contributor

@pnwamk pnwamk commented Mar 26, 2021

Use the more generic "tuple" instead of "struct" notion of value in the server backend (while keeping "struct" in the llvm-interface). See #1031

@pnwamk pnwamk requested a review from atomb March 26, 2021 22:06
@RyanGlScott
Copy link
Contributor

Does this imply that we should rename the data structures corresponding to the struct type (in addition to the struct setup value)?

@pnwamk
Copy link
Contributor Author

pnwamk commented Mar 26, 2021

Does this imply that we should rename the data structures corresponding to the struct type (in addition to the struct setup value)?

I'm re-reading @atomb's original comments on the matter. The impression I got was the things which are not inherently LLVM-related should prefer the more generic "tuple", while things that are LLVM specific should be free to use the terminology/type names/etc from LLVM. To that end, I think it's right to rename the SetupVal to "tuple" because it is a crucible/core concept, whereas the types are LLVM specific, so they should keep "struct" as the name.

@pnwamk pnwamk requested a review from RyanGlScott March 31, 2021 23:24
@pnwamk pnwamk 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 Mar 31, 2021
@@ -44,7 +44,7 @@ instance FromJSON cryptolExpr => FromJSON (CrucibleSetupVal cryptolExpr) where
TagNullValue -> pure NullValue
TagCryptol -> CryptolExpr <$> o .: "expression"
TagArrayValue -> ArrayValue <$> o .: "elements"
TagStructValue -> StructValue <$> o .: "fields"
TagTupleValue -> TupleValue <$> o .: "elements"
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this isn't the officially sanctioned source of documentation anymore, but I'd find it helpful if old-SAW.rst were updated to reflect this change to the SAW remote API, especially in light of #1121.

Copy link
Contributor

@atomb atomb left a comment

Choose a reason for hiding this comment

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

This looks good to me. And, yes, the idea was that language-agnostic stuff would use tuple/record names whereas language-specific stuff would use language-specific names (like struct). At the moment, that distinction is somewhat academic, but once the Java bindings are in place I think it'll be more meaningful.

@mergify mergify bot merged commit 4624229 into master Apr 2, 2021
@mergify mergify bot deleted the rpc-server/refactor-struct-val branch April 2, 2021 19:14
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.

3 participants