This repository has been archived by the owner on Oct 12, 2022. It is now read-only.
(Re)Introduce Throwable.message #1895
Merged
WalterBright
merged 2 commits into
dlang:master
from
nemanja-boric-sociomantic:futureisnow
Aug 31, 2017
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import core.stdc.stdio; | ||
|
||
// Make sure basic stuff works with future Throwable.message | ||
class NoMessage : Throwable | ||
{ | ||
@nogc @safe pure nothrow this(string msg, Throwable next = null) | ||
{ | ||
super(msg, next); | ||
} | ||
} | ||
|
||
class WithMessage : Throwable | ||
{ | ||
@nogc @safe pure nothrow this(string msg, Throwable next = null) | ||
{ | ||
super(msg, next); | ||
} | ||
|
||
override const(char)[] message() const | ||
{ | ||
return "I have a custom message."; | ||
} | ||
} | ||
|
||
class WithMessageNoOverride : Throwable | ||
{ | ||
@nogc @safe pure nothrow this(string msg, Throwable next = null) | ||
{ | ||
super(msg, next); | ||
} | ||
|
||
const(char)[] message() const | ||
{ | ||
return "I have a custom message and no override."; | ||
} | ||
} | ||
|
||
class WithMessageNoOverrideAndDifferentSignature : Throwable | ||
{ | ||
@nogc @safe pure nothrow this(string msg, Throwable next = null) | ||
{ | ||
super(msg, next); | ||
} | ||
|
||
immutable(char)[] message() | ||
{ | ||
return "I have a custom message and I'm nothing like Throwable.message."; | ||
} | ||
} | ||
|
||
void test(Throwable t) | ||
{ | ||
try | ||
{ | ||
throw t; | ||
} | ||
catch (Throwable e) | ||
{ | ||
fprintf(stderr, "%.*s ", e.message.length, e.message.ptr); | ||
} | ||
} | ||
|
||
void main() | ||
{ | ||
test(new NoMessage("exception")); | ||
test(new WithMessage("exception")); | ||
test(new WithMessageNoOverride("exception")); | ||
test(new WithMessageNoOverrideAndDifferentSignature("exception")); | ||
fprintf(stderr, "\n"); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Need a test case to turn the red to green.
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 see all tests in green, or do you mean the green of a review approval?
Also, there are some test cases already included, is there anything in particular you see missing?
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.
Line 1745 above is red. It isn't executed because all the test cases override
Throwable.message()
. Need a test case that does not override it, and just calls 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.
@leandro-lucarella-sociomantic There is a CodeCov browser extension, which overlays the unittest coverage on top of the diff for pull requests. I think what Walter is saying is that this line doesn't show up as covered by a unittest.
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.
@ZombineDev is right. Here it is: https://github.com/codecov/browser-extension Please install 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.
Thanks, had no idea there's an extension, will do it today!
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've started looking into this and it feels something is off with the code coverage - because this is already covered by: https://github.com/dlang/druntime/pull/1895/files#diff-8289be999021da628af8f313e9992573R4 (I even confirmed this with printouts in the line - it sure gets triggered)
Could it be that source of the problem is that code coverage doesn't collect ones from the test suite? IIRC, we have the similar problem in sociomantic's ocean, about the code coverage overriding each other, I wonder if it same here.
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've added the unittest, let's see if that helps
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.
Ok, green now!
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.
Not sure if there's a bugzilla issue about this (if not there should be), the reason for this initially not appearing covered is that coverage in external test files is essentially ignored. I think it is a build-system level issue, though I haven't looked in detail.