[refactor] remove DiagnosticReporter from core compiler#10711
[refactor] remove DiagnosticReporter from core compiler#10711RazvanN7 merged 1 commit intodlang:masterfrom
Conversation
|
Thanks for your pull request, @rainers! Bugzilla referencesYour 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 locallyIf 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#10711" |
|
|
||
| auto o = p.parseTypeOrAssignExp(TOK.endOfFile); | ||
| if (p.errors) | ||
| if (errors != global.errors) |
There was a problem hiding this comment.
The p.errors checks have been slightly wrong because they ignore warnings emitted as errors. There are only two warnings shown by parsing and lexing, though.
There was a problem hiding this comment.
Do you want to make that change together with refactorings?
There was a problem hiding this comment.
Well, it's more or less a revert of the respective change in the introducing commit. I'm not sure it makes much sense to fix it in the DiagnosticReporter just to rip it out afterwards.
rainers
force-pushed
the
move_diagreport
branch
5 times, most recently
from
8516b06 to
81a76bd
Compare
jacob-carlborg
left a comment
jacob-carlborg
left a comment
There was a problem hiding this comment.
I think this is a step backwards. Now the lexer and parser will depend on more global state.
|
|
||
| auto o = p.parseTypeOrAssignExp(TOK.endOfFile); | ||
| if (p.errors) | ||
| if (errors != global.errors) |
There was a problem hiding this comment.
Do you want to make that change together with refactorings?
rainers
force-pushed
the
move_diagreport
branch
2 times, most recently
from
60f9b7b to
254375b
Compare
rainers
force-pushed
the
move_diagreport
branch
from
254375b to
f409093
Compare
|
I agree that global state should be minimized, but this allows the DiagnosticReporter to be used for semantic analysis aswell. I don't see this realistically happening otherwise by passing around some extra object everywhere. |
|
@jacob-carlborg I understood your comment here (#10072 (comment)) as agreeing on a change like this. |
|
I was thinking a delegate could be passed to the lexer and parser instead of a class. |
WalterBright
added
the
Merge:72h no objection -> merge
|
This caused a regression (seen on forums), creating a reduced test. |
|
Reduced test and bug report created: https://issues.dlang.org/show_bug.cgi?id=20730 |
| o = p.parseTypeOrAssignExp(TOK.endOfFile); | ||
| if (p.errors || | ||
| p.token.value != TOK.endOfFile) | ||
| if (errors != global.errors || p.token.value != TOK.endOfFile) |
There was a problem hiding this comment.
I think the problem is here, this check is done whilst gagging is turned on, unlike in other places where p.errors was replaced with errors != global.errors.
|
Proposed fix in #11023. |
6 participants
and move it into the frontend library files instead.
Now that #10072 is in, we can use it to implement the DiagnosticReporter outside of the core compiler.