-
-
Notifications
You must be signed in to change notification settings - Fork 671
add errorsink.d, an abstract interface to error messages #14906
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7561,7 +7561,7 @@ extern (C++) class TemplateInstance : ScopeDsymbol | |
| //printf("\t staticIfDg on '%s %s' in '%s'\n", s.kind(), s.toChars(), this.toChars()); | ||
| if (!s.isStaticIfDeclaration()) | ||
| { | ||
| //s.dsymbolSemantic(sc2); | ||
| //s.dsymbolSemantic(sc2); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated, own commit
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That came about because of my checkin process converts all tabs to spaces, and apparently there were tabs on that line. Being a bit of trivia, there's no issue here. |
||
| done = true; | ||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import core.stdc.stdarg; | |
| import core.stdc.stdio; | ||
| import core.stdc.stdlib; | ||
| import core.stdc.string; | ||
| import dmd.errorsink; | ||
| import dmd.globals; | ||
| import dmd.location; | ||
| import dmd.common.outbuffer; | ||
|
|
@@ -24,6 +25,57 @@ import dmd.console; | |
|
|
||
| nothrow: | ||
|
|
||
| /*************************** | ||
| * Error message sink for D compiler. | ||
| */ | ||
| class ErrorSinkCompiler : ErrorSink | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's the right abstraction either. What about messages issued by
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it not the right abstraction? What has
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. impl should really be using |
||
| { | ||
| nothrow: | ||
| extern (C++): | ||
| override: | ||
|
|
||
| void error(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| va_list ap; | ||
| va_start(ap, format); | ||
| verror(loc, format, ap); | ||
| va_end(ap); | ||
| } | ||
|
Comment on lines
+37
to
+43
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dangerously close to hijacking gcc error diagnostics here. Or am I expected to just fork out the implementation to the C++ side of the compiler? |
||
|
|
||
| void errorSupplemental(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| va_list ap; | ||
| va_start(ap, format); | ||
| verrorSupplemental(loc, format, ap); | ||
| va_end(ap); | ||
| } | ||
|
|
||
| void warning(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| va_list ap; | ||
| va_start(ap, format); | ||
| vwarning(loc, format, ap); | ||
| va_end(ap); | ||
| } | ||
|
|
||
| void deprecation(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| va_list ap; | ||
| va_start(ap, format); | ||
| vdeprecation(loc, format, ap); | ||
| va_end(ap); | ||
| } | ||
|
|
||
| void deprecationSupplemental(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| va_list ap; | ||
| va_start(ap, format); | ||
| vdeprecationSupplemental(loc, format, ap); | ||
| va_end(ap); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Color highlighting to classify messages | ||
| */ | ||
|
|
@@ -768,7 +820,7 @@ private void colorHighlightCode(ref OutBuffer buf) | |
| ++nested; | ||
|
|
||
| auto gaggedErrorsSave = global.startGagging(); | ||
| scope Lexer lex = new Lexer(null, cast(char*)buf[].ptr, 0, buf.length - 1, 0, 1, global.vendor, global.versionNumber()); | ||
| scope Lexer lex = new Lexer(null, cast(char*)buf[].ptr, 0, buf.length - 1, 0, 1, global.errorSink, global.vendor, global.versionNumber()); | ||
| OutBuffer res; | ||
| const(char)* lastp = cast(char*)buf[].ptr; | ||
| //printf("colorHighlightCode('%.*s')\n", cast(int)(buf.length - 1), buf[].ptr); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| /** | ||
| * Provides an abstraction for what to do with error messages. | ||
| * | ||
| * Copyright: Copyright (C) 2023 by The D Language Foundation, All Rights Reserved | ||
| * Authors: $(LINK2 https://www.digitalmars.com, Walter Bright) | ||
| * License: $(LINK2 https://www.boost.org/LICENSE_1_0.txt, Boost License 1.0) | ||
| * Source: $(LINK2 https://github.com/dlang/dmd/blob/master/src/dmd/errorsink.d, _errorsink.d) | ||
| * Documentation: https://dlang.org/phobos/dmd_errorsink.html | ||
| * Coverage: https://codecov.io/gh/dlang/dmd/src/master/src/dmd/errorsink.d | ||
| */ | ||
|
|
||
| module dmd.errorsink; | ||
|
|
||
| import dmd.location; | ||
|
|
||
| /*************************************** | ||
| * Where error/warning/deprecation messages go. | ||
| */ | ||
| abstract class ErrorSink | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reuse existing wheels or explain why they are insufficient. Such deficiencies might include Note also that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This line illustrates the insufficiency: This is the wrong abstraction. The user of Lexer should be able to send the errors anywhere, baking into the API "stderr" or not is insufficent. It's other insufficiency is it is centered around a pointer to a function which overrides the default behavior. This is a primitive form of OOP that fell out of favor in the 1980s. The abstract class, for which the user adds derived classes with specific behaviors, is the more modern approach. The abstract class designer does not have to anticipate what the implementation classes will be doing.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to touch dmd.frontend in this PR. As I mentioned to @ibuclaw it needs a significant overhaul.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then I consider this PR to require approval from @jacob-carlborg before merging.
I do not trust you will ever get around to this. And this adds yet more files without packaging things properly and I trust even less that you will ever do that.
So can
Where? why? (Hint, no it doesn't). |
||
| { | ||
| nothrow: | ||
| extern (C++): | ||
|
|
||
| void error(const ref Loc loc, const(char)* format, ...); | ||
|
|
||
| void errorSupplemental(const ref Loc loc, const(char)* format, ...); | ||
|
|
||
| void warning(const ref Loc loc, const(char)* format, ...); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only added the ones used. |
||
|
|
||
| void deprecation(const ref Loc loc, const(char)* format, ...); | ||
|
|
||
| void deprecationSupplemental(const ref Loc loc, const(char)* format, ...); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only added the ones used. Others can be added as necessary. |
||
|
|
||
| /***************************************** | ||
| * Just ignores the messages. | ||
| */ | ||
| class ErrorSinkNull : ErrorSink | ||
| { | ||
| nothrow: | ||
| extern (C++): | ||
| override: | ||
|
|
||
| void error(const ref Loc loc, const(char)* format, ...) { } | ||
|
|
||
| void errorSupplemental(const ref Loc loc, const(char)* format, ...) { } | ||
|
|
||
| void warning(const ref Loc loc, const(char)* format, ...) { } | ||
|
|
||
| void deprecation(const ref Loc loc, const(char)* format, ...) { } | ||
|
|
||
| void deprecationSupplemental(const ref Loc loc, const(char)* format, ...) { } | ||
| } | ||
|
|
||
| /***************************************** | ||
| * Simplest implementation, just sends messages to stderr. | ||
| */ | ||
| class ErrorSinkStderr : ErrorSink | ||
| { | ||
| import core.stdc.stdio; | ||
| import core.stdc.stdarg; | ||
|
|
||
| nothrow: | ||
| extern (C++): | ||
| override: | ||
|
|
||
| void error(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| fputs("Error: ", stderr); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't |
||
| const p = loc.toChars(); | ||
| if (*p) | ||
| { | ||
| fprintf(stderr, "%s: ", p); | ||
| //mem.xfree(cast(void*)p); // loc should provide the free() | ||
| } | ||
|
|
||
| va_list ap; | ||
| va_start(ap, format); | ||
| vfprintf(stderr, format, ap); | ||
| fputc('\n', stderr); | ||
| va_end(ap); | ||
| } | ||
|
|
||
| void errorSupplemental(const ref Loc loc, const(char)* format, ...) { } | ||
|
|
||
| void warning(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| fputs("Warning: ", stderr); | ||
| const p = loc.toChars(); | ||
| if (*p) | ||
| { | ||
| fprintf(stderr, "%s: ", p); | ||
| //mem.xfree(cast(void*)p); // loc should provide the free() | ||
| } | ||
|
|
||
| va_list ap; | ||
| va_start(ap, format); | ||
| vfprintf(stderr, format, ap); | ||
| fputc('\n', stderr); | ||
| va_end(ap); | ||
| } | ||
|
|
||
| void deprecation(const ref Loc loc, const(char)* format, ...) | ||
| { | ||
| fputs("Deprecation: ", stderr); | ||
| const p = loc.toChars(); | ||
| if (*p) | ||
| { | ||
| fprintf(stderr, "%s: ", p); | ||
| //mem.xfree(cast(void*)p); // loc should provide the free() | ||
| } | ||
|
|
||
| va_list ap; | ||
| va_start(ap, format); | ||
| vfprintf(stderr, format, ap); | ||
| fputc('\n', stderr); | ||
| va_end(ap); | ||
| } | ||
|
|
||
| void deprecationSupplemental(const ref Loc loc, const(char)* format, ...) { } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change, should be its own commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another case of tabs being converted to spaces by my checkin procedure. Not worth another PR.