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.

Sign up for GitHub

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

Jump to bottom

clearer resolution of variables + add warning #605

Merged
merged 5 commits into from
Nov 16, 2023
Merged

clearer resolution of variables + add warning #605

merged 5 commits into from
Nov 16, 2023

Conversation

bgregoir
Copy link
Contributor

@bgregoir bgregoir commented Oct 5, 2023

this PR solve #604.

@bgregoir bgregoir requested review from vbgl and eponier October 7, 2023 10:56
@bgregoir
Copy link
Contributor Author

Any comment ?

@vbgl
Copy link
Member

vbgl commented Oct 18, 2023

The refactoring looks OK. I don’t understand the new warnings though. IMHO they do not belong to pre-typing but to your IDE or the so-called linter.

@bgregoir
Copy link
Contributor Author

This is true, but we spend two hours (with Tiago) to debug a program due to this absence of warning.
Another problem, for used declaration, is that we do not keep track of the local variables declaration, this mean that it is hard to write a linter that will warn stuff after pretyping (since the info does not exists anymore).

@bgregoir
Copy link
Contributor Author

Maybe I can remove the warning from that file and store the needed info somewhere else.
Then I can add a linter.ml that will for the moment only print that warning.
But we can extent it to print more.
Should I do that ?

@vbgl
Copy link
Member

vbgl commented Oct 18, 2023

we spend two hours (with Tiago) to debug a program due to this absence of warning.

You could have written: “we had a hard time debugging due to lack of IDE support”.

vbgl
vbgl approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@vbgl vbgl left a comment

Choose a reason for hiding this comment

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

Please add a CHANGELOG entry.

compiler/src/pretyping.ml Outdated Show resolved Hide resolved
bgregoir and others added 3 commits November 15, 2023 08:58
@bgregoir @vbgl
Update compiler/src/pretyping.ml
194249c 
Co-authored-by: Vincent Laporte <vbgl@users.noreply.github.com>
@bgregoir
add entry in the change log
70c4b39
@bgregoir
changelog
0e47fd5
vbgl
vbgl reviewed Nov 16, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@vbgl vbgl merged commit b60a11e into main Nov 16, 2023
1 check was pending
@vbgl vbgl deleted the warn-duplicate branch November 16, 2023 13:51
vbgl pushed a commit that referenced this pull request Apr 3, 2024
@bgregoir @vbgl
clearer resolution of variables + add warning (#605)
ecf8477 
(cherry picked from commit b60a11e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbgl vbgl vbgl approved these changes

@eponier eponier Awaiting requested review from eponier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bgregoir @vbgl