-
Notifications
You must be signed in to change notification settings - Fork 20
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
macaw-x86-syntax: Syntactic sugar for macaw-x86-symbolic CFGs #422
Conversation
3229df0
to
284e1d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! I'm looking forward to being able to use this.
Now that we have macaw-x86-syntax
, it would also be nice to have macaw-*-syntax
packages for the other ISAs that macaw
supports (AArch32, PowerPC, and RISC-V). Could you file an issue about this as a reminder to do this later?
let r = MXS.x86RegAssignment Ctx.! i | ||
rName = MS.crucGenArchRegName MXS.x86_64MacawSymbolicFns r | ||
in show rName | ||
Nothing -> error "impossible" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using panic
here instead of error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that it makes a big difference here - The main distinction between panic
and error
is the message that gets printed out, especially the confirmation that it is a programmer error (and not, say, a user error). But since this code is only part of the test suite, it's only targeted at Macaw developers, and it's clear from that context that it constitutes a programmer error. In any case, it's real easy to change so I'll go ahead and change it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the Panic
modules in the Macaw repo are exposed, and Lang.Cruicble.Panic
feels misleading... I might just leave this as error
if that's alright with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usual convention is to create a PanicComponent
instance for each package so that the panic
messages can trace back to the specific package that threw the panic (see, for instance, Data.Macaw.X86.Symbolic.Panic
).
That being said, I'm fine with error
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the suggestion that test-suites should have their own PanicComponent
? Again, it wouldn't be too much effort to add one, but I'm still having a hard time seeing the advantages of adding this over just using error
. Could you describe the benefits to doing this? I'm happy to implement this (for all the test-suites in the repo) if we can agree it has benefits.
Comparing a few possible approaches:
- We could try to make this function total (e.g., by introducing a
Maybe
). I would argue there's not much point - it's in a test-suite, so doesn't need to be very robust, and it returningNothing
would indicate a bug in the test harness (not in an individual test). Thus, I think partiality is both easier and clearer. - We could have an incomplete pattern match - I think this is basically the same as using
undefined
, and would throw some kind of generic exception? - We could abort/exit the process - not sure how this would interact with Tasty (e.g., would it be considered a test failure? would other tests run after it fails?)
- We could use
error
, which raises theErrorCall
exception - We could define a custom exception type and throw that, like
Tasty.HUnit
does - We could use
panic
, which itself defines a custom exception type. It's relatively undocumented, but my impression is that this exception is meant primarily to ask users to file bug reports. This doesn't seem super relevant to me, as users generally don't run the test suite.
All in all, I don't see many advantages nor disadvantages of any of these approaches over one another, and using error
is really easy :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the suggestion that test-suites should have their own PanicComponent?
Yes. The only real advantage is that you'd get the nice formatting (along with information about where to submit a bug report) that the panic
library provides. That being said, that's not super important, since we're in a test suite (and most test suite users are developers).
So, again, I'm fine with error
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sorry if I went overboard there 😄 I just wanted to make sure I wasn't missing something important!
Perhaps we just keep #349 open, and add a comment saying that this has been done for x86_64 but the other architectures need it too? |
That works for me. |
Towards #349