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

refactor(sem): clean up hasUnresolvedParams #957

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

bung87
Copy link
Contributor

@bung87 bung87 commented Oct 13, 2023

Summary

Remove disabled code from the hasUnresolvedParams procedure. The
code has been disabled for 7 years and is unlikely to become used
again.

Details

  • remove the disabled code block
  • remove the unused flags: TExprFlags parameter

@zerbina
Copy link
Collaborator

zerbina commented Oct 14, 2023

I'm not sure I agree with their removals. They do convey additional information, and they also provide a customization point (e.g., for placing an assertion). For example, I think that hasUnresolvedParams(s) (the name is not entirely correct) conveys more information to the reader than a "raw" tfUnresolved in s.flags check.

In any case, a routine having a name longer than its implementation is by itself not a reason for removal.

To be clear, I have no problem with the disabled code being removed; removing the procedures as a whole is what I feel uneasy about.

@bung87
Copy link
Contributor Author

bung87 commented Oct 15, 2023

I agree with your points, but still problem remains template rangeHasUnresolvedStatic give the connection to reader that seems range type has special detection of unresolved, but it doesn't. proc hasUnresolvedParams accept extroTExprFlags that been used in disabled code. which also gives reader unnecessary association

@zerbina
Copy link
Collaborator

zerbina commented Oct 15, 2023

still problem remains template rangeHasUnresolvedStatic give the connection to reader that seems range type has special detection of unresolved

What do you mean? I'm not sure I understand.

proc hasUnresolvedParams accept extroTExprFlags that been used in disabled code. which also gives reader unnecessary association

Yep, the unused flag is confusing -- it should be removed together with the disabled code.

@bung87 bung87 changed the title clean tfUnresolved usages Remove proc hasUnresolvedParams disabled code and obsolete parameter Oct 16, 2023
@bung87
Copy link
Contributor Author

bung87 commented Oct 16, 2023

followed your suggestions, I messed up git history, sorry.

@saem saem added compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Oct 16, 2023
@zerbina zerbina changed the title Remove proc hasUnresolvedParams disabled code and obsolete parameter refactor(sem): clean up hasUnresolvedParams Oct 17, 2023
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.

@bung87 I've reworded the PR message a bit and shortened the title. Once you've taken a look at the updated message and title, please ping me and I'll start the merge.

@bung87
Copy link
Contributor Author

bung87 commented Oct 17, 2023

@zerbina Thank you! nice!

@zerbina
Copy link
Collaborator

zerbina commented Oct 17, 2023

/merge

@github-actions
Copy link

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

@chore-runner chore-runner bot added this pull request to the merge queue Oct 17, 2023
Merged via the queue into nim-works:devel with commit 3d72145 Oct 17, 2023
18 checks passed
@bung87 bung87 deleted the clean-tfUnresolved-usages branch October 17, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants