Simplify representation of measure names #2464
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| getLHNameResolved n@LHNUnresolved{} = error $ "getLHNameResolved: unresolved name: " ++ show n | ||
| getLHNameResolved n@LHNUnresolved{} = error $ "getLHNameResolved: unresolved name: " ++ showLHNameDebug n | ||
|
|
||
| showLHNameDebug :: LHName -> String |
There was a problem hiding this comment.
This should be the Show instance really, and what is now the instance should be a regular function that explains what it is for.
There was a problem hiding this comment.
I tried that, and found that it influenced error messages intended for users. I'll address this in a follow up PR.
sjoerdvisscher
approved these changes
Dec 12, 2024
| sp1 | ||
| return (sp2, logicNameEnv0) | ||
| else | ||
| return (error "invalid spec", error "invalid logic environment") |
There was a problem hiding this comment.
Add a module name or function name to the error?
There was a problem hiding this comment.
I'm counting on the error trace to show me the location. But I added the function name anyway in 919776d.
facundominguez
force-pushed
the
fd/lhname-measure-names3
branch
from
c8f9a2a to
919776d
Compare
|
Thanks for the review @sjoerdvisscher! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the name field of measures to use LHName instead of Symbol. This makes possible to store more information in the name field, instead of paring the
Measurerecords with LHName in specs.I considered generalizing the type of the field instead of hard-coding it to LHName, but the new type parameter would have propagated to specs and it was a lot of changes and an extra parameter for no clear benefit.
Another commit provides a show function for LHNames that prints the structure of the name. And yet another commit fixes a problem where name resolution would panic if early stages produced an error and we allowed the resolution to proceed anyway, now we interrupt name resolution early if there are errors.