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

transpile: Rewrite fn compare_src_locs implementation to have a total order #1128

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Kriskras99
Copy link

@Kriskras99 Kriskras99 commented Sep 18, 2024

This implementation is simplified compared to the previous one. It is also almost twice as slow in the exhaustive test (15 vs 25 seconds).
However, in real sort usage the impact should be significantly less.

This implementation is simplified compared to the previous one.
It is also almost twice as slow in the exhaustive test (15 vs 25 seconds) in
immunant#1126 (comment)
However, in real sort usage the impact should be significantly less.

Fixes immunant#1126
@kkysen kkysen self-requested a review September 18, 2024 22:20
@kkysen kkysen changed the title Rewrite compare_src_locs implementation to have a total order transpile: Rewrite fn compare_src_locs implementation to have a total order Sep 18, 2024
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.

Thanks! This looks a lot better, and the better comments help a lot as well.

Is there a test we can add to make sure this is working as intended (correctly sorted, no panics)?

c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
@Kriskras99
Copy link
Author

I've added a test for the total order, based on a reduced test case from the issue (apparently creduce can reduce any text file).
I've also included your code suggestions.

c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
@Kriskras99
Copy link
Author

Not sure what my IDE did to the imports but I'll fix it, I'll also fix your other suggestions. But it will have to wait until after the weekend

@kkysen
Copy link
Contributor

kkysen commented Sep 20, 2024

Not sure what my IDE did to the imports but I'll fix it, I'll also fix your other suggestions. But it will have to wait until after the weekend

No problem, no rush.

@Kriskras99 Kriskras99 force-pushed the sorting_fix branch 2 times, most recently from b562165 to 12f7571 Compare September 23, 2024 15:12
@Kriskras99
Copy link
Author

@kkysen I've included your suggestions. Is there anything else that needs to change?

@kkysen kkysen self-requested a review September 30, 2024 19:45
Also included some code suggestions by Khyber.

Co-authored-by: Khyber Sen <[email protected]>
@Kriskras99
Copy link
Author

Added your last suggestion

@kkysen
Copy link
Contributor

kkysen commented Oct 3, 2024

CI seems to be failing, only on Darwin, for some reason:

CI

/Users/runner/work/1/s/tests/statics/src/thread_locals.c:4:17: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
static __thread gsti = 37;
~~~~~~~~~~~~~~~ ^
int
/Users/runner/work/1/s/tests/statics/src/thread_locals.c:5:17: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
extern __thread geti;
~~~~~~~~~~~~~~~ ^
int
/Users/runner/work/1/s/tests/statics/src/thread_locals.c:12:21: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
    static __thread fsti = 59;
    ~~~~~~~~~~~~~~~ ^
    int
/Users/runner/work/1/s/tests/statics/src/thread_locals.c:13:21: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
    extern __thread feti;
    ~~~~~~~~~~~~~~~ ^
    int

This doesn't seem to be related to the changes, here, though, so I'm not sure what's happening.

@Kriskras99
Copy link
Author

Maybe the darwin image got updated or something? I really don't see how this change can cause this

@Kriskras99
Copy link
Author

Could you try triggering the CI on the last commit in master? See if it has the same failure

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.

SrcLoc sorting is non-transitive and a false total order and equality (panics in 1.81)
2 participants