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

Turn --reorganize-definitions on by default #215

Open
rinon opened this issue Nov 23, 2019 · 7 comments
Open

Turn --reorganize-definitions on by default #215

rinon opened this issue Nov 23, 2019 · 7 comments
Assignees

Comments

@rinon
Copy link
Contributor

rinon commented Nov 23, 2019

I think reorg is now stable enough to be turned on by default. However, I'd like rust-lang/rust#66464 to get fixed before we do. Large codebases (e.g. python2 & nginx) trigger this bug after doing reorg.

@rinon rinon added the blocked Cannot be supported without external support first label Nov 23, 2019
@rinon rinon self-assigned this Nov 23, 2019
@rinon
Copy link
Contributor Author

rinon commented Dec 6, 2019

We can do this after #217 lands.

@thedataking thedataking removed the blocked Cannot be supported without external support first label May 1, 2020
@jrmuizel
Copy link

#217 has landed. Can this happen?

@kkysen kkysen changed the title Turn reorg on by default Turn --reorganize-definitions on by default Jun 29, 2022
@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

rust-lang/rust#66464 has also been fixed by now. @rinon, is this still something we should enable?

@rinon
Copy link
Contributor Author

rinon commented Jun 29, 2022

Unfortunately we can't support reorganize-definitions until we have some sort of refactoring support again. This feature uses a global refactoring pass after transpiling to gather and organize declarations and definitions across modules. We really need a substitute for the refactoring engine (or resurrecting it as is with a newer nightly), as reorg was a pretty critical feature IMO.

@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

Is --reorganize-definitions using c2rust-refactor directly? It's still an option in c2rust transpile --help. Or is it just not very useful without the refactor afterwards?

Also, maybe once MIR is stabilized, we could update the refactoring engine to use that and just leave it, not having to worry about nightly updates anymore:

@rinon
Copy link
Contributor Author

rinon commented Jun 29, 2022

We should remove it from the options... at least for now. It requires c2rust-refactor and will fail to work properly without it.

I'm not sure MIR helps us, because ultimately we need to transform text, rather than an underlying IR, in order to refactor things.

@kkysen
Copy link
Contributor

kkysen commented Jun 29, 2022

Ok, I can remove the option then.

For the refactoring in general, I don't know much about it, but rust-analyzer and Intellij Rust have improved dramatically in the last few years and may be able to do a lot of the simpler refactorings we want, though I'm not sure on the details there. Even clippy; I wonder what it says about redundant casts, for example.

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

No branches or pull requests

4 participants