do not emit mangling for inferred 'return' attribute #9476
Conversation
WalterBright
added
the
Review:Vision
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. 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#9476" |
| scopeinferred = (1L << 49), // 'scope' has been inferred and should not be part of mangling | ||
| future = (1L << 50), // introducing new base class function | ||
| local = (1L << 51), // do not forward (see dmd.dsymbol.ForwardingScopeDsymbol). | ||
| returninferred = (1L << 52), // 'return' has been inferred and should not be part of mangling |
There was a problem hiding this comment.
Wouldn't it be better to have just one STCinferred?
There was a problem hiding this comment.
Either return or scope or both can be present or inferred.
There was a problem hiding this comment.
I'd not have expected inference to happen if either is explicitly given, but Ok.
Some of the bit flags may require a little pruning as there aren't many available left now.
WalterBright
force-pushed
the
returninferred
branch
from
7a35d19 to
cfa89e5
Compare
| va.storage_class |= STC.scope_ | STC.scopeinferred; | ||
| va.storage_class |= v.storage_class & STC.return_; | ||
| if (v.storage_class & STC.return_) | ||
| va.storage_class |= STC.return_ | STC.returninferred; |
There was a problem hiding this comment.
Bad indentation.
Doesn't this need a check for pre-existing return annotation?
if (!(va.storage_class & STC.return_))
va.storage_class |= STC.return_ | STC.returninferred;
If scope and return can be inferred independently, the if-condition !va.isScope() seems to disallow that.
| static assert(typeof(foo).mangleof == "FNaNbNiNfPvZi"); | ||
|
|
||
| auto bar(void* p) { return p; } | ||
| static assert(typeof(bar).mangleof == "FNaNbNiNfPvZQd"); |
There was a problem hiding this comment.
Please also add tests that show explicit annotations make it to the mangling.
There was a problem hiding this comment.
testscope2.d only runs with -dip25, but some of the code only affects -dip1000 AFAICT.
There was a problem hiding this comment.
BTW: you can also use core.demangle with CTFE for more readable asserts.
|
Wow! I gave compiling Phobos with -preview=dip1000 another shot with this. |
6 participants
The idea is to get code binaries with and without -dip1000 to play together. This is already done for
scope.