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

Success story running c2rust on h3 and some feedback #720

Closed
LegNeato opened this issue Nov 8, 2022 · 7 comments
Closed

Success story running c2rust on h3 and some feedback #720

LegNeato opened this issue Nov 8, 2022 · 7 comments

Comments

@LegNeato
Copy link
Contributor

LegNeato commented Nov 8, 2022

(feel free to close, just wanted to give a success story and perhaps file bugs / open PRs for found issues)

I ran c2rust on h3 and it worked pretty well! Thank you! See https://github.com/LegNeato/unsafe-h3lib for the cleaned up output and the git history for what it took to get everything building.

I put some issues encountered in nmandery/h3ron#31 (comment) but the tl;dr is:

  1. Formatting
  2. Replacing f128. I just made these f64, but I should probably change to rust_decimal with the c_repr feature. This is probably the source of the test failures.
  3. Adding the various Cargo.toml files.
  4. Adding the various lib.rs files. This was needed as the app binaires aren't as standalone as they could be.
  5. Silencing warnings, including things like using PI consts and unnecessary mut.
  6. Fixing clippy lints
  7. There was a c2rust bug in some of the tests I needed to work around by copying an informational output string. Oddly, it got it right in other cases so I need to investigate.

Questions/comments about the above:

  1. It looks like the old refactor tool was responsible for runningrustfmt. To me, it seems like this isn't really refactoring and instead is part of the translation. If you agree I can file a task and perhaps put up a PR.
  2. long double varargs won't compile #154. But that doesn't mention rust_decimal. Is rust_decimal with the c_repr feature a potential path forward?
  3. Not sure that c2rust could do anything about this without parsing cmake. Or could it? Does compile_commands.json contain linker invocations?
  4. Not sure that c2rust could do anything about this without parsing cmake. Or could it? Does compile_commands.json contain linker invocations?
  5. Seems like PI and such constants could be replaced? An argument could be made that it is refactoring, but I also think just as strong as an argument could be made that translating a well known constant in C to a rust std one is in the scope of the translator. For the second, clippy somehow knows the muts were redundant but c2rust does not. Is that a bug I should file?
  6. Use std::ptr::null_mut::<T>() instead of 0 as *mut T #202 was the main one. There were also a bunch of redundant returns generated, is that a bug I should file?
  7. I am not sure if this is a bug or if c2rust isn't supposed to understand borrowing. See LegNeato/unsafe-h3lib@386d0f4#r89264842
@thedataking
Copy link
Contributor

@LegNeato Thank you for taking the time to report a success story plus useful feedback. Very encouraging to read!

re 1. please go ahead and file a task/PR.

re 3-4. these items are very similar but seems to miss something. compile_commands.json does not capture linker invocations but at least one tool maintainer is interested in adding them in to support our use case.

re 7. this is a bug; please file an issue so we can get this sorted.

@LegNeato
Copy link
Contributor Author

Great, I will try to get that all done this week.

@eddywestbrook
Copy link

@LegNeato As a point of reference, how long did the translation take? I looked at your commit history, and it looks like all of your commits are on the same day, though I realize that you might have just batched them at the same time...

@LegNeato
Copy link
Contributor Author

LegNeato commented Dec 2, 2022

The commit timestamps are realtime / they were not batched. I wasn't working 100% of the time though...went quick!

@eddywestbrook
Copy link

Whoa, that's pretty impressive! It looks like it all happened in one day!

I noticed one of the files had a lot of lines of code changed, but I guess that was just formatting some large arrays, and maybe you did some regexp find and replace or something like that?

@LegNeato
Copy link
Contributor Author

LegNeato commented Dec 3, 2022

Yep, a lot of find and replace, a run with all clippy autofixes, some manual fixes, and rustfmt.

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

3 participants