core.demangle: add support for type, identifer and symbol back references #1830
Conversation
|
Notably missing unittests, but I guess these will be added after compiler support is in. |
The demangling is tested with the compiler test suite as I changed some of the mangling tests to just compare the result of demangling. I guess I should some tests here, though, so we'll get some coverage locally before merging. |
rainers
force-pushed
the
mangle_backrefs
branch
3 times, most recently
from
23972c9
to
78d946f
Compare
|
Added some test cases. Trying to demangle all symbols from phobos unittest, I'm hitting the amgibuity on Pascal function types, see dlang/dmd#6702. The current implementation runs into a stack overflow. I should add some sanity checks anyway... |
|
As a test field to work on I have extracted all D symbols from the map file of the phobos unittest. For a build with master, there are 127172 symbols with a maximum length of 416133 characters and an average length of 369 characters. With back references the same symbols have a maximum length of 1114 characters with an average of 117. I updated demangling to properly support back references, but a number of failures for demangling symbols with back references also affected symbols as emitted by master, so I fixed these, too. Results:
I cancelled the master test after about 3 hours when it was about half done (timings taken from that run). For the back reference symbols, results only change a little:
Edit: failed demanglings for symbols with back references reduced from 36 to 21 by update to dmd PR. |
src/core/internal/traits.d
Outdated
| static if (__traits(compiles, __traits(externDmangle, "foo", void function()))) | ||
| { | ||
| alias _wrap(alias T) = T; // fool the parser | ||
| alias externDFunc = _wrap!(__traits(externDmangle, fqn, FT)); |
There was a problem hiding this comment.
We should be fixing mangleFunc!T(fqn) instead.
There was a problem hiding this comment.
Unfortunately the qualified name in mangleFunc is a runtime parameter. No luck using a trait there...
As mangleFunc is a public documented function this quite a bummer. If we want to keep the runtime argument as is I don't see an alternative to actually implementing the mangler in druntime, too. I'm not sure if this is possible with the recently added traits, but would be rather pretty pessimistic with respect to template arguments. As the qualified name didn't support these so far, it might not be a large breaking change to restrict the function type to non-template types, too. |
|
Thanks for your pull request, @rainers! Bugzilla references
|
FYI: this is a newly deployed feature on the dlang-bot and it will only post once for each PR and then update its comment with (hopefully) helpful messages, see e.g.
Bugs or feature requests can be reported dlang-bot's repo. |
rainers
force-pushed
the
mangle_backrefs
branch
2 times, most recently
from
3e13966
to
e7e567c
Compare
|
Planning to throw out symbol back references and just go with type and symbol back references, I've now implemented mangleFunc with the help of the demangler and a little bit of DbI. I had to add some |
rainers
force-pushed
the
mangle_backrefs
branch
2 times, most recently
from
75eba5e
to
a2a30ae
Compare
|
Rebased with symbol back references removed. |
rainers
force-pushed
the
mangle_backrefs
branch
3 times, most recently
from
848ea8b
to
d123511
Compare
|
I've restricted the trusted code to a few small functions, so the demangler can be annotated safe. Apart from adding support for back references this PR now fixes some other issues, too:
|
FYI: you can tag as many issues as you want to the dlang-bot / changelog in one commit. |
…ding, support no full length specified
… function signatures in QualifiedNames, only print return types for the outmost symbol
…make demangler @safe, pure and nothrow
|
@wilzbach @CyberShadow I think its pretty demotivating seeing almost every PR in the overview marked with a red cross, even though these are caused by spurious or unrelated failures of coverage and jenkins builds. Maybe the resulting build state should be evaluated from the required tests only. Is this possible? |
|
@rainers I agree completely, and I've been complaining about this since forever to anyone who can listen, but I'm not sure what I can do at this point. If you have time to spare, tracking down and fixing the causes of the random failures or coverage fluctuations would be much appreciated. @wilzbach What is the significance of "74.92% of diff hit (target 75.38%)"? Is it to say that of the lines added in this PR, only 74% are covered? If so, why the oddly specific 75.38% threshold? |
Unfortunately I don't think it's possible. It would be nice if we could get rid of the codecov statuses and only use their bot's comments or integrate them with dlang-bot's comments, though. |
I fully agree.
Well actually Jenkins is supposed to be required, but we had quite some issues with it over the last few weeks.
Yeah, I took us quite a long time to fix all spots at Phobos.
Yes.
Target is the overall coverage that the project had before. I think the motivation is that you want to avoid decreasing the overall coverage with every PR.
This shouldn't be too difficult, but I am not sure whether people would really care about the bot's comment. CodeCov's solution is to send a new comment for every new coverage event which is quite noisy. Unfortunately the GH status API really sucks and we can't automatically receive the status hook and remove it, but would have to crawl CodeCov for updates. |
It seems that it boils down to one of the following:
IMO the second one has more value, as lack of trust in CI is not a problem we want to have, and seems more important than the artificial coverage metric. I understand that even if we were to set the target to a very low value or even 0, codecov will still continue creating the status updates, and thus it should still be possible to see the difference in coverage at a glance, right? If so, then to me that seems like the preferable option. |
I don't know how that would be possible, the only thing that comes to my mind that we can do for now is #1874 (disable the status notifications).
AFAICT is the target the current project coverage. We could try to manually set the target coverage to |
… times when optimizing
| } | ||
| if (len + val.length > dst.length) | ||
| overflow(); | ||
| size_t v = &val[0] - &dst[0]; |
| char peek( size_t n ) | ||
| { | ||
| if( pos + n < buf.length ) | ||
| return buf[pos + n]; |
There was a problem hiding this comment.
I speculate speed is important here - perhaps you can make this trusted and skip bounds checking.
|
I'll boldly go where only @rainers has gone and merge this. |
Yes, but only if they are based on a master commit that contains these changes (unfortunately
CodeCov doesn't show up anymore, but it's built and analyzed: https://codecov.io/gh/dlang/druntime/commit/d4015f4d403bd22ecee8b81bb61378d33e7df7df
Thanks a lot.
Yes, I didn't anticipate that this would be merged so quickly (it could have been rechanged to |
Thanks, looks good.
Can the link be displayed somewhere? Or will it be necessary to build it manually or copy it from circleCI's output?
Nor did I ;-) |
dlang/dlang-bot#142 (for now we will just hard-code it to success and display it in the status bar). In the future, we might also show the message in the comment, but that's slightly more complicated as we would need to parse the existing comment when try to modify it) |
support for dlang/dmd#5855 and a couple of fixes.