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

cgen: mark &Array.sorted_with_compare as used #21909

Closed

Conversation

felipensp
Copy link
Member

Fix #21907

@felipensp felipensp marked this pull request as ready for review July 22, 2024 14:10
@spytheman
Copy link
Member

It should not be whitelisted imho.

In the lsv project, the call to .sort_with_compare(), is not implicit, and done on the whim of the cgen stage (and thus unpredictable from the markused stage).

The call is present in the user program, and reachable through main -> lsv -> sort:

sorted << groups[key].sorted_with_compare(cmp)

(that is from lsv/lsv/sort.v:68)

v.markused should be improved instead, to handle that case/ast node, and mark it as used without a special case in the whitelist.

@spytheman
Copy link
Member

spytheman commented Jul 23, 2024

I have a patch, that does it. I'll make a PR after the local tests pass:
image

@spytheman
Copy link
Member

I've pushed #21914 .

@felipensp
Copy link
Member Author

It should not be whitelisted imho.

In the lsv project, the call to .sort_with_compare(), is not implicit, and done on the whim of the cgen stage (and thus unpredictable from the markused stage).

The call is present in the user program, and reachable through main -> lsv -> sort:

sorted << groups[key].sorted_with_compare(cmp)

(that is from lsv/lsv/sort.v:68)

v.markused should be improved instead, to handle that case/ast node, and mark it as used without a special case in the whitelist.

Yeah, good catch. I recently got a weird case where v_fixed_index got removed with -skip-unused too.

@felipensp felipensp closed this Jul 23, 2024
@felipensp felipensp deleted the fix_unused_sorted_with_compare branch July 23, 2024 11:24
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.

C Error building lsv with -skip-unused
2 participants