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

chore: remove callset from Procedure #1470

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Aug 27, 2024

I was able to remove the call set from Procedure and ProcedureContext without breaking anything. It suggests that we no longer need to track that information.

Am I missing something?

Note: You can only look at the first commit; the second one fixes the new nightly clippy across the repo and is unrelated

@plafer plafer added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Aug 27, 2024
@bitwalker
Copy link
Contributor

This was being used at one point to drive the order in which procedures are compiled (by tracing from the root procedures you are interested in, to all of the procedures they called, thus excluding any procedures that are not referenced). However, the ModuleGraph essentially made that irrelevant, but IIRC there were still some things it was used for immediately after that was introduced, but I think we've essentially removed all such usages at this point, at least so far as I can recall.

The one thing it could in theory be useful for, is accessing that type of information after assembly - the ModuleGraph (and in particular, the call graph), are essentially internal to the Assembler, so if you want to do your own static analysis based on the call graph, you have to reconstruct it. I'm not aware of anything that is relying on that at the moment though.

@plafer
Copy link
Contributor Author

plafer commented Aug 27, 2024

I don't think we should keep things around "in case we need them later", so I'm in favor of merging this.

@bitwalker
Copy link
Contributor

I don't think we should keep things around "in case we need them later", so I'm in favor of merging this.

I agree that cruft should be removed in general, but the definition of what constitutes "unused" code isn't always about what we need in these crates. Users of these crates also need things, above and beyond what is needed directly by the assembler, or the VM. The compiler is one such user, obviously the most important one, but there can (and ideally will) be others. We should always be considering that perspective when making changes that remove or reduce the functionality exposed, and ideally we would implement things in such a way that all of the tools are there for downstream users to implement the stuff they need themselves, rather than needing to do it upstream, but that isn't always clear cut.

Typically, if I recognize that something would be useful to the compiler, or to a tool I'd like to build, I'll advocate for those things when it comes up, but obviously I don't review every PR. On numerous occasions I've run into the fact that APIs are not made public that should be, or aren't implemented at all (or are removed), because they weren't needed in this repo directly, so this isn't a theoretical issue.

In any case, I think this is perfectly fine to remove, because it wasn't really the best way to export that information anyway, and it isn't clear how a post-assembly analysis of the call graph would be used in practice. I could see it being useful for implementing tools like cargo-udeps or something along those lines, but without a compelling use case, I agree with your general sentiment that we should remove cruft.

@plafer
Copy link
Contributor Author

plafer commented Aug 27, 2024

Good point. I will merge this, but will also keep this in mind in the future. What probably makes most sense in the short term at least is to ping you whenever we remove something we think isn't useful anymore.

@plafer plafer merged commit 07134e3 into next Aug 27, 2024
9 checks passed
@plafer plafer deleted the plafer-remove-proc-callset branch August 27, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants