Skip to content

Conversation

@WalterBright
Copy link
Member

Reverts #9484

Once this is reverted, #9350 is next.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9507"

@jacob-carlborg
Copy link
Contributor

Please close, it's fine as is.

@andralex
Copy link
Member

@jacob-carlborg as a matter of course, if the project lead disagrees with a large refactoring, we need to go with that. Sorry.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how due process goes.

@wilzbach
Copy link
Contributor

Walter: have you ever tried using DMD as a library? We need to be able to collect diagnostics in DMD as a library, just printing them out to stdout doesn't cut it (think e.g. language server), so until there's a clear argument on how one can do this once you'll have undo this work, I strongly urge you to reconsider.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Not without a discussion.

@wilzbach
Copy link
Contributor

wilzbach commented Mar 29, 2019

if the project lead disagrees with a large refactoring, we need to go with that. Sorry.

Sure, but why would anyone still bother contributing?
What do you think will happen to core contributors whose work gets reverted/closed without any good argument?
Yup, they get fed up and leave. This has happened many many times in the past.

@thewilsonator
Copy link
Contributor

You should give a good amount of thought to someone saying that who is giving a dconf presentation on "How to Become a D Contributor"...

@dkorpel
Copy link
Contributor

dkorpel commented Mar 29, 2019

I'm confused. Jacob is doing valuable work to help the sorry state of D static analysis tools, so why is there so much resistance instead of support? It even seems to comply with Walter's values:
"Successfully using pure functions is regarded with particular favor." - DMD style
"No I/O In Low Level Functions" (and instead) "Push I/O Up Call Stack" - Avoiding code smells
And it's done in relatively small steps instead of a monster PR. The only objection I see mentioned is:

It's a great deal of code added that appears to do nothing.

Yes, there is some temporary boilerplate because it's a gradual refactor. And even once it's finished, the line count may be increased. That also happens when changing gotos into nested functions, it doesn't imply worse code.

Is there another diagnostic reporter design we can all agree with, or it the current error handling design considered ideal and not to be tampered with?

@WalterBright
Copy link
Member Author

WalterBright commented Mar 30, 2019

there's a clear argument on how one can do this once you'll have undo this work

Further rationale given here: #9350

@WalterBright
Copy link
Member Author

WalterBright commented Mar 30, 2019

"Successfully using pure functions is regarded with particular favor."

Replacing a call to an impure function with a call to an impure interface doesn't make the caller pure.

"Push I/O Up Call Stack"

Replacing a call to a function with one to an interface is not pushing the call up the stack.

@WalterBright
Copy link
Member Author

@wilzbach > there's a clear argument on how one can do this once you'll have undo this work

#9350 (comment)

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Mar 31, 2019

Replacing a call to an impure function with a call to an impure interface doesn't make the caller pure.
Replacing a call to a function with one to an interface is not pushing the call up the stack.

The DiagnosticReporter PRs did not do this. It was the other PR that you closed #9494, that was in the process of accomplishing that for the lexer. Here's the complete PR that accomplishes that: #9480 (no one has made a single comment in that PR). As you can see, the error function of the lexer is now pure: https://github.com/dlang/dmd/pull/9480/files#diff-b24a7d8934fc062aaa3f71e4e5f96081R2297.

@WalterBright
Copy link
Member Author

As you can see, the error function of the lexer is now pure

Yes, and that is good.

@jacob-carlborg
Copy link
Contributor

As you can see, the error function of the lexer is now pure

Yes, and that is good.

@WalterBright But you don't like it anyway? Because you close the PR #9494 that is the first of several in the process of making this possible.

@WalterBright
Copy link
Member Author

But you don't like it anyway?

That's right. It's much too heavy, as "first of several in the process of making this possible" indicates.

@Geod24
Copy link
Member

Geod24 commented Aug 10, 2019

This branch is 5 months old, and almost 1500 commits behind master. If there's still outstanding issues, please submit a new PR for it.

@Geod24 Geod24 closed this Aug 10, 2019
@Geod24 Geod24 deleted the revert-9484-move-diagnostic-reporter branch August 10, 2019 14:49
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.

8 participants