Skip to content

Conversation

@ahomescu
Copy link
Contributor

Change the behavior of remove_unnecessary_refs to not remove dereferences of pointers because those are generally required.

@ahomescu ahomescu force-pushed the ahomescu/fix_reorganize_collisions branch from c5185d5 to f8b7c3b Compare November 21, 2025 22:13
@ahomescu ahomescu force-pushed the ahomescu/fix_remove_refs branch from c1a9e4e to 3b74526 Compare November 21, 2025 22:13
Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

Code changes look reasonable, but is there an example to look at to better understand the motivation for this change? This refactoring pass runs in CI for the testsuite--are there cases that were broken there and now fixed?

@kkysen kkysen changed the title c2rust-refactor: Fix remove_unnecessary_refs to ignore pointers refactor: Fix remove_unnecessary_refs to ignore pointers Nov 21, 2025
Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

LGTM, too.

@ahomescu ahomescu force-pushed the ahomescu/fix_remove_refs branch from 3b74526 to 542d402 Compare November 21, 2025 23:58
@ahomescu ahomescu force-pushed the ahomescu/fix_reorganize_collisions branch from f8b7c3b to 0bba09a Compare November 21, 2025 23:58
Change the behavior of remove_unnecessary_refs to not remove
dereferences of pointers because those are generally required.
@ahomescu
Copy link
Contributor Author

Code changes look reasonable, but is there an example to look at to better understand the motivation for this change? This refactoring pass runs in CI for the testsuite--are there cases that were broken there and now fixed?

That's right, we have remove_unnecesary_refs enabled in CI now and there are a bunch of places where it failed, e.g.

error[E0308]: mismatched types
   --> src/xzlib.rs:263:17
    |
263 |         *have = (have).wrapping_add(ret as ::core::ffi::c_uint);
    |         -----   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u32`, found `*mut u32`
    |         |
    |         expected due to the type of this binding
    |
    = note:     expected type `u32`
            found raw pointer `*mut u32`

error[E0599]: no method named `expect` found for raw pointer `*mut Option<unsafe extern "C" fn(*mut c_void, *const i8, ...)>` in the current scope
     --> src/xpath.rs:18203:18
      |
18202 | /             (crate::src::globals::__xmlGenericError())
18203 | |                 .expect(
      | |                 -^^^^^^ method not found in `*mut Option<unsafe extern "C" fn(*mut c_void, *const i8, ...)>`
      | |_________________|

and more.

I think that &*x is always fine to elide when x is a reference, but never when it's a pointer (correct me if I missed a case).

I had a separate patch for the test suite to disable that transform, but might as well fix it instead.

@ahomescu ahomescu force-pushed the ahomescu/fix_remove_refs branch from 542d402 to d5de663 Compare November 22, 2025 00:24
Base automatically changed from ahomescu/fix_reorganize_collisions to master November 22, 2025 00:28
@ahomescu ahomescu merged commit 9c4460c into master Nov 24, 2025
5 checks passed
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.

4 participants