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

semProcAux hasError cover fixupInstantiatedSymbols #938

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

bung87
Copy link
Contributor

@bung87 bung87 commented Oct 3, 2023

Summary

  • semProcAux hasError cover fixupInstantiatedSymbols
  • Instantiated symbol's AST may be the nkError kind that causen[bodyPos] raise FieldDefect

Details

  • fixupInstantiatedSymbols now returns false if instantiated symbol's AST is nkError kind during iteration

Notes for Reviewers

@bung87 bung87 force-pushed the semProcAux-hasError-cover-fixupInstantiatedSymbols branch from b53bfd0 to cf0a45a Compare October 3, 2023 07:06
@bung87 bung87 marked this pull request as ready for review October 3, 2023 11:46
Copy link
Collaborator

@zerbina zerbina left a comment

Choose a reason for hiding this comment

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

This is not correct, sorry.

There being an error in the instantiation of a generic procedure is not an error with the definition. Besides, the way you use hasError here is wrong: AST must only be wrapped in a wrapper error (wrapError) when there's an nkError somewhere within, but this is not the case here.

In general, please document in the PR message what exactly you're fixing (what is the error and why did it occur), and also provide a test where you have made sure that it reproduces the failure, thanks. I don't want to have to investigate these issues myself each time.

@bung87
Copy link
Contributor Author

bung87 commented Oct 3, 2023

by the words "not an error", here should continue iteration without marking anything ?

@zerbina
Copy link
Collaborator

zerbina commented Oct 3, 2023

by the words "not an error", here should continue iteration without marking anything ?

Yes. Instantiations with errors are reported when they're inserted into the module AST.

@bung87
Copy link
Contributor Author

bung87 commented Oct 3, 2023

is that a global error? because here n[bodyPos] may cause FieldDefect

@zerbina
Copy link
Collaborator

zerbina commented Oct 3, 2023

Oh, my bad, I misremembered. instantiateGeneric reports the error (via localReport), appendInstancedGenericRuntimeRoutines doesn't.

@bung87
Copy link
Contributor Author

bung87 commented Oct 3, 2023

okay so here the error check is needed I guess, I also aware even similar kind of errors, they use localReport or globalReport that seems don't have certain rules. that's another thing.

@zerbina
Copy link
Collaborator

zerbina commented Oct 3, 2023

To be clear, my original review comment still applies.

@bung87 bung87 marked this pull request as draft October 4, 2023 03:28
@bung87
Copy link
Contributor Author

bung87 commented Oct 4, 2023

okay, so this is the another case compiler continue compilation cause subsequent errors, the appendInstancedGenericRuntimeRoutines do internalAssert which should cause compiler abort,
whereas structuredReportHook may just return doNothing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants