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

fix lying comment #285

Merged
merged 1 commit into from
Aug 30, 2023
Merged

fix lying comment #285

merged 1 commit into from
Aug 30, 2023

Conversation

Ptival
Copy link
Contributor

@Ptival Ptival commented Aug 29, 2023

Comment was incorrect, the argument remains the full vector of arguments throughout the recursion.

Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Almost makes you wonder if argsToRegister should be written like this to make it more obvious what it's doing:

argsToRegister :: forall m. Monad m => V.Vector AnnFunArg -> ArgResolver m ()
argsToRegister args = go 0
  where
    go :: Int -> ArgResolver m ()
    go cnt
      | cnt >= V.length args = pure ()
      | otherwise = do
          let arg = args V.! cnt
          let nm = fromMaybe ("arg" ++ show cnt) (funArgName arg)
          resolveArgType nm (funArgType arg)
          go (cnt + 1)

Whether you find this cleaner or not is up to personal taste. Either way, it's good to correct the comments.

@Ptival
Copy link
Contributor Author

Ptival commented Aug 29, 2023

I was considering it, so I'm going to change it.

`argsToRegisters` was unnecessarily exposing an argument counter used
internally for vector indexing, while annotated with an incorrect
comment.  This makes the function simpler from the outside, and fixes
the comment.
@Ptival Ptival merged commit 3f88f60 into main Aug 30, 2023
2 checks passed
@Ptival Ptival deleted the vr/fix-lies branch August 30, 2023 15:46
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.

2 participants