Fix 21544 - -checkaction=context formats enum members as their base type #3336
Conversation
MoonlightSentinel
requested review from
MartinNowak and
schveiguy
as code owners
|
Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + druntime#3336" |
src/core/internal/dassert.d
Outdated
| } | ||
|
|
||
| // Format invalid enum values as their base type | ||
| return "cast(" ~ V.stringof ~ ") " ~ miniFormat(*(cast(BaseType*) &v)); |
There was a problem hiding this comment.
Doesn't this allocate ? Also miniFormat!BaseType(v) should work, no ?
There was a problem hiding this comment.
Doesn't this allocate ?
Yes, but string concatenation is used in several places. But I could also re-use combine here...
Also
miniFormat!BaseType(v)should work, no ?
No because miniFormat's accepts it's parameter by ref
There was a problem hiding this comment.
Yes, but string concatenation is used in several places.
If we are to make this the default, assert(a != b) should not allocate.
Note that it currently does anyway, because of throw new AssertError, but it really shouldn't, because that makes it unusable in destructors.
There was a problem hiding this comment.
Guess this could use combine (or some other utility method) s.t. we can change the allocation strategy in the future.
But that will always be a problem for a user-defined toString()s that are not @nogc (unless those are skipped while running destructors)
There was a problem hiding this comment.
Assert errors shouldn't really be caught. Can we just malloc it?
Yes, a bit of a sloppy solution. But likely the program is about to end.
There was a problem hiding this comment.
Another solution -- set up a "for asserts" allocator that uses malloc, and then frees everything just before the program exits.
There was a problem hiding this comment.
But that will always be a problem for a user-defined
toString()s that are not@nogc(unless those are skipped while running destructors)
that's going to be a really tough problem to solve.
There was a problem hiding this comment.
Can we just malloc it?
hah, just looked at what combine does, and it does just that ;) That's the solution right there.
There was a problem hiding this comment.
Done. Maybe miniFormat should be reworked to use an OutputRange-like buffer s.t. it doesn't need to reallocate to build the final string.
MoonlightSentinel
force-pushed
the
checkaction-enum
branch
from
223a4e0
to
2b5c54d
Compare
Generate code that detects the correct enum member (or defaults to the base type in case of an invalid enum value).
MoonlightSentinel
force-pushed
the
checkaction-enum
branch
from
2b5c54d
to
02dd9d9
Compare
|
|
||
| // Invalid enum value is printed as their implicit base type (int) | ||
| e = cast(E) 3; | ||
| assert(miniFormat(e) == "cast(E) 3"); |
There was a problem hiding this comment.
combine inserts a space before and after the token (here "")
Generate code that detects the correct enum member (or defaults to the base type in case of an invalid enum value).