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

Remove apply-append-for-loop-to-for-loop lint #435

Open
pavpanchekha opened this issue Dec 18, 2024 · 2 comments
Open

Remove apply-append-for-loop-to-for-loop lint #435

pavpanchekha opened this issue Dec 18, 2024 · 2 comments
Labels
existing lint Issues or pull requests relating to existing lints

Comments

@pavpanchekha
Copy link

The apply-append-for-loop-to-for-loop lint seems to, in some cases, result in much uglier code. For example, in Herbie, it rewrites:

,@(apply append
    (for/list ([rows (append mainline-infos other-infos)])
      (print-rows rows #:name (dict-ref (first rows) 'branch)))

into

,@(for*/list ([rows (append mainline-infos other-infos)]
              [v (in-list (print-rows rows #:name (dict-ref (first rows) 'branch)))])
    v)

This seems much worse! The actual body of the loop is now hidden inside a loop bound, and there's a dummy variable introduced (v) with no real purpose. It also moves the code way to the right, which makes it uglier too. I suggest removing or scoping down this lint.

@pavpanchekha pavpanchekha added the new lint Issues suggesting new lints or pull requests implementing new lints label Dec 18, 2024
@jackfirth
Copy link
Owner

I added this one back when I was trying to get a bunch of gnarly list-processing code to fuse into nice loops, so the real value wasn't this specific refactoring - it was combining it with other for-based refactorings. But on its own I agree that it's actually detrimental. I'll remove this one.

Someday I'd like to see the for/append-list macro suggested in racket/racket#2483 implemented. That's nicer than either existing solution, and would let Resyntax rewrite the above code to this:

,@(for/append-list ([rows (append mainline-infos other-infos)])
    (print-rows rows #:name (dict-ref (first rows) 'branch)))

@jackfirth jackfirth added existing lint Issues or pull requests relating to existing lints and removed new lint Issues suggesting new lints or pull requests implementing new lints labels Dec 18, 2024
@pavpanchekha
Copy link
Author

Yeah that would be nice. In Herbie we sometimes use a reap macro for this, and in another project I had a for/reap that made stuff like this nice. Whatever, anyway, I filed the issue, removing the lint seems good, or maybe it can be used only if it combines with another lint later or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
existing lint Issues or pull requests relating to existing lints
Projects
None yet
Development

No branches or pull requests

2 participants