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

Fixing Inline Elision (Tests) #23

Merged
merged 3 commits into from
Jul 28, 2023
Merged

Fixing Inline Elision (Tests) #23

merged 3 commits into from
Jul 28, 2023

Conversation

JustusAdam
Copy link
Collaborator

@JustusAdam JustusAdam commented Jul 28, 2023

Some of the inline elision tests fail.

test no_elision_without_output ... FAILED
test no_elision_without_input ... FAILED
test basic_elision_mut ... ok
test basic_elision ... ok
test connection_precision_self ... ok
test unelision ... ok
test connection_precision_args ... ok

Even more mysterious: If I disable elision (not pass the --inline-elision argument), only three of them fail.

test no_elision_without_input ... FAILED
test unelision ... ok
test connection_precision_self ... FAILED
test no_elision_without_output ... FAILED
test connection_precision_args ... ok
test basic_elision ... ok
test basic_elision_mut ... ok

Maybe this is mean to happen, but need to investigate. Already found one niche (but very dumb) error in the algebra.


So here is what happened.

  1. My merge broke things a bit and the flag --inline-elision was no longer being heeded. Inline elision would simply always be performed. Worse still it would also forget to check if the function carries a marker in the inliner. This might not actually have manifested negatively but it feels dangerous at least.
  2. The tests were actually wrong. Whether or not inline elision happens, the (elided) function is gone from the graph whether its body is inlined or elided. However the test cases were checking for exactly that, whether the function occurs in the graph. I changed them to just call std::convert::identity (an external function and therefore neither inlined, nor elided) inside and then check if that function is in the graph.

Now the test cases predictably succeed when --inline-elision is active and fail if not.

@JustusAdam JustusAdam changed the title Fixed a very niche error Fixing Inline Elision Tests Jul 28, 2023
@JustusAdam JustusAdam changed the title Fixing Inline Elision Tests Fixing Inline Elision (Tests) Jul 28, 2023
@JustusAdam JustusAdam merged commit 31b5991 into main Jul 28, 2023
4 checks passed
@JustusAdam JustusAdam deleted the fixing-elision-tests branch July 28, 2023 17:57
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.

1 participant