-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix(sem): fix crash routine pragma redefinition #1445
fix(sem): fix crash routine pragma redefinition #1445
Conversation
error reporting This existed because of unhelpful assertions in legacy reports.
fix code that was clowning instead of actually using string formatting still not qutie right as there is _some_ flexibility with pragmas on definitions
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.
Looks good to me. I left one minor suggestion regarding the test file placement.
(I believe you also want to update the PR title?)
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.
Given that the test is for an error message, I think it would make sense to move it to the errmsgs
category, but that's only a soft suggestion.
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 debated that, I think error messages should be part of the feature/functionality they're defining in the error case, where errmsgs
is for stuff we can't otherwise categorize, but maybe that's not as wise an idea as I think it is.
Thoughts?
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.
Hm, from the top of my head, I'd consider there being an error part of the feature, while a test for the exact message(s) feels like something separate from it.
Following that line of though, in this case, completing a forward-declaration with a header using different pragmas being an error would belong under lang_callable
, while a test for the message itself would belong under errmsgs
.
Given that there's only a single message format here, there's no point in having two separate tests, so I think the current test location is correct. For ease of later test navigation/lookup, what you could do is add a error_message
label to the test, but that's just another (very) soft suggestion.
Thanks
Done |
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
/merge |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below:
|
Summary
The compiler no longer crashes when encountering a pragma redefinition
error on a routine. This was due to unnecessary asserts in error message
code
Details
the symbol for the definition, as they will be the same
%
)instead of concatenation
Fixes #1444