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

Remove unused method. #962

Closed
wants to merge 1 commit into from
Closed

Conversation

ltratt
Copy link
Contributor

@ltratt ltratt commented Feb 8, 2024

Newer Rust nightlies spot that this is unused, causing our .buildbot.sh to fail.

This is responsible for the recent failure of #959 to pass CI.

Newer Rust nightlies spot that this is unused, causing our
`.buildbot.sh` to fail.
@vext01
Copy link
Contributor

vext01 commented Feb 8, 2024

We use this method for debugging, so I don't want to remove it. Can we mark it used somehow?

@ltratt
Copy link
Contributor Author

ltratt commented Feb 8, 2024

Given that eprintln!("{}", x.to_str()) is equivalent, this feels like a really low value method to leave lurking as dead code...

@ltratt
Copy link
Contributor Author

ltratt commented Feb 8, 2024

[TBH, I'm not even sure why we have to_str instead of just impl Display or Debug, but that's a slightly separate issue.]

@vext01
Copy link
Contributor

vext01 commented Feb 8, 2024

TBH, I'm not even sure why we have to_str instead of just impl Display or Debug

Because you require the module in order to print the instructions sensibly and those traits don't provision for passing in user data.

Given that eprintln!("{}", x.to_str()) is equivalent...

I'm going to push back.

When I'm debugging something difficult, the harder it is for me to do print debugging, the less productive I am.

Typing (and remembering that this is how you have to do it):

eprintln!("{}", x.to_str(m))

insead of:

x.dump(m)

Can really distract from the (usually difficult) problem I'm trying to solve. And this frustrates me.

I know you don't attach much weight to LLVM design decisions, but they do the same and (IMHO) got it right. In LLVM it's:

X.dump();

instead of:

X.print(errs());

Please allow us this :)

@ltratt
Copy link
Contributor Author

ltratt commented Feb 8, 2024

Please can we unblock CI first then have this (old) argument in a later PR?

@ltratt
Copy link
Contributor Author

ltratt commented Feb 8, 2024

@vext01 is going to take a different approach to this. Closing.

@ltratt ltratt closed this Feb 8, 2024
@ltratt ltratt deleted the remove_unused_method branch April 4, 2024 11:03
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.

2 participants