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
Issue 9451 - Better diagnostic on unimplemented abstract methods. #1620
Conversation
Guys would it be ok if I changed |
The idea is that they line up with normal errors, making it easier to skip them. |
I'm closing temporarily as I want to refactor the code so it's used for both abstract (current pull) and interface functions (#1523). |
Ok I've fixed up Pull 2452 so now both interface and abstract functions use the same diagnostic. |
@@ -666,6 +666,7 @@ struct FuncDeclaration : Declaration | |||
void appendState(Statement *s); | |||
char *mangle(bool isv = false); | |||
const char *toPrettyChars(); | |||
const char *toFullSignature(); // for diagnostics, e.g. '@pure foo(int x, int y)' |
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.
More correct example is better: e.g. 'int foo(int x, int y) pure'
Updated. |
} | ||
|
||
class C : I | ||
{ | ||
void f(int p) { } |
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.
I think the original test case has an intent as follows: "if derived class actually implements abstract method, compiler does not report error for it." So, this looks to me to make original test case a bit weaken.
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.
I can't clearly understand what you mean, do you want me to revert the test-case to what it was before and just change TEST_OUTPUT? Also this test is about interface methods, not abstract methods.
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.
No. This is just a nitpick. What I mean is:
If class C implements f2(int)
, the error message "void f2(int) is not implemented" should not appear, but "void f2(float) const is not implemented" should.
That was well tested in original case, but you removed it.
To realize it, you can add only void f2(int p) {}
in class C, and fix the diagnostic message according to it.
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.
Was this done?
Let me do a quick modification so functions are wrapped inside of quotes. |
Waiting for green. |
It's been green for a while now: http://d.puremagic.com/test-results/pull-history.ghtml?projectid=1&repoid=1&pullid=1620 |
LGTM. |
Issue 9451 - Better diagnostic on unimplemented abstract methods.
http://d.puremagic.com/issues/show_bug.cgi?id=9451