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

Revert "RFC: prettier IR-show for line number and inlining information" #29118

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

StefanKarpinski
Copy link
Member

Reverts #28390

@vtjnash
Copy link
Member

vtjnash commented Sep 10, 2018

Why?

@mbauman
Copy link
Member

mbauman commented Sep 10, 2018

In #28390 in response to your merge:

I extremely much prefer the old printing. Please bring that back with an option or something. I can't use this at all.

Yeah, I'm sorry, but this printing just looks messy. It's great if I care about the line information, but near useless if I want to actually look at the code:

I agree. I loved the previous @code_typed output. I had already found the line info comments in LLVM IR and native ASM to be far too verbose to be helpful… and we've talked about it on Slack. Do you really find the pushback here surprising?

@StefanKarpinski
Copy link
Member Author

Because you merged it without getting consensus or addressing the many objections to the change. That's not acceptable and immediate reversion is the appropriate response.

@KristofferC
Copy link
Member

There seem to be two main purposes to use these macros. One is where you want to get an overview of what the code actually end up doing. And another when you want to pinpoint exactly where in the source code a specific instruction came from. I typically use it for the first purpose where these verbose line annotations is mostly noise. So some option flag to toggle between these two modes might be suitable.

And as a reference, when looking at code_warntype, I typically paste it into an editor and use some regex filtering to remove all line annotations.

@jrevels
Copy link
Member

jrevels commented Sep 10, 2018

I can understand why others feel it's too verbose by default, but I find the verbose pretty-printing really useful for my purposes (which are probably similar to Jameson's).

Is it possible that instead of reverting this entirely, we can hide the additional verbosity behind a flag or something? It makes debugging things via looking at the IR way easier IMO.

@KristofferC
Copy link
Member

That is the wrong order. The correct order is to revert this and then the author makes a new PR that adds that verbosity option.

@vtjnash
Copy link
Member

vtjnash commented Sep 10, 2018

The verbosity option is already present (Base.IRShow.lineinfo_disabled), it just needs a name. There's actually several configuration options that got added here, so that people could experiment further—I just opted not to expose any of them initially (they're listed as hard-coded constants right now in the source code, until we got a feel for which would be most useful).

@KristofferC KristofferC merged commit 6caabc9 into master Sep 10, 2018
@mbauman mbauman deleted the revert-28390-jn/irshow3 branch September 10, 2018 22:10
@StefanKarpinski
Copy link
Member Author

All the code introspection tools need to be switched back to reasonably concise, readable output by default and then you can add back the option to ask for more verbose output.

@StefanKarpinski
Copy link
Member Author

We're also not going to reward the bad behavior of merging a controversial PR unilaterally without addressing the objections people have with it by not reverting it and doing the work to fix it. Someone who finds this extremely verbose, noisy output useful can make a PR that is actually acceptable.

@StefanKarpinski
Copy link
Member Author

The verbosity option is already present (Base.IRShow.lineinfo_disabled), it just needs a name.

This is not a sufficient interface to control this. Excessive output should also not be the default.

@vtjnash
Copy link
Member

vtjnash commented Sep 11, 2018

All the code introspection tools need to be switched back to reasonably concise, readable output by default

OK, so what you're saying is that we should go back to the v0.6 level of information in printing?

@StefanKarpinski
Copy link
Member Author

Yes, with options for more advanced output.

vtjnash added a commit to vtjnash/julia that referenced this pull request Sep 28, 2018
vtjnash added a commit to vtjnash/julia that referenced this pull request Oct 1, 2018
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.

5 participants