-
-
Notifications
You must be signed in to change notification settings - Fork 606
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 8765, 9255 - Show informative assert errors #7575
Conversation
Thanks for your pull request, @wilzbach! Bugzilla references
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#7575" |
@nomad-software it looks like your testing library needs to be updated to not rely on the default assert error message. |
@@ -4391,6 +4401,16 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor | |||
result = exp; | |||
} | |||
|
|||
private Expression ensureValidMsg(Scope* sc, Expression msg) |
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.
Could this be inlined into its one and only caller?
@@ -4333,6 +4333,9 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor | |||
printf("AssertExp::semantic('%s')\n", exp.toChars()); | |||
} | |||
|
|||
// save expression as a a string before any semantic expansion | |||
auto assertExpMsg = exp.msg ? null : exp.toChars(); |
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.
Right, this should be done before semantic, but is using toChars
correct here? What happens with multiline expressions?
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.
Could exp.copy()
be used instead? But ultimately you're going to want to print the expression.
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.
isn't exp.copy()
shallow, so that the copy will still be affected by semantic analysis?
I side with @WalterBright and his comments in the Bugzilla issue. I think if the developer chose to leave off the assert message, then that was their deliberate choice, and I don't think the compiler should override that choice. If we want to print the expression by default, then I recommend introducing an Furthermore, this does have the potential to be a disruptive change for test suites that analyze assert outputs. I vote to close Issue 8765 with won't fix, and instead encourage an alternate solution as I mentioned above. UPDATE: After reading through the comments of the previous PRs that attempted to address this issue, I believe that my opinion above will likely be very unpopular. If the community at large finds this enhancement useful, I will concede. |
#7831 solves a more general problem via ARGS_STRING; assert could either be implemented in terms of ARGS_STRING or reuse code from that PR in functionParameters modulo a small refactoring to extract out this block of code:
|
Fix Issue 9255 - Use file name in assert failure
#8517 supersedes this. |
Rebased #5189 (orginates from: #1203 via #1426)