Skip to content

statement_toIR(): simplify conversion of label statements to goto#10323

Merged
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:extraLabel
Aug 29, 2019
Merged

statement_toIR(): simplify conversion of label statements to goto#10323
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:extraLabel

Conversation

@WalterBright
Copy link
Member

by not needing another hash table

@WalterBright WalterBright added the Severity:Refactoring No semantic changes to code label Aug 20, 2019
@WalterBright WalterBright requested a review from ibuclaw as a code owner August 20, 2019 06:09
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your 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 locally

If 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#10323"

@WalterBright WalterBright force-pushed the extraLabel branch 6 times, most recently from 7b96f3f to d1341ae Compare August 20, 2019 07:13
@WalterBright
Copy link
Member Author

@TurkeyMan @CyberShadow

The buildkite/dmd error:

../../crypto/vibe/crypto/cryptorand.d(173,6): Error: cannot modify `const` expression `p`

The code https://github.com/vibe-d/vibe.d/blob/master/crypto/vibe/crypto/cryptorand.d

auto p = atomicLoad(*cast(const shared GET_RANDOM*) &hasGetRandom);
if (p == GET_RANDOM.UNINITIALIZED)
{
	p = initHasGetRandom() ? GET_RANDOM.AVAILABLE   <= error on this line
					: GET_RANDOM.NOT_AVAILABLE;

Perhaps it has something to do with this? dlang/druntime#2739

@WalterBright
Copy link
Member Author

Looks like the buildkite/dmd error is unrelated to this PR.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 20, 2019

Same as #10322, I'd like a small pause for thought before going ahead. As I've said in the other, this reverts #4854, and goes against the reasoning for using a hash table in the first place - to remove dmd backend-only fields from common frontend types.

If the reasoning for this is the same as in #10322, then perhaps it would be better to jump straight to something that uses the frontend for data analysis, rather than a halfway solution that has no effect for either gdc/ldc, other than slightly increasing memory usage.

@CyberShadow
Copy link
Member

Looks like the buildkite/dmd error is unrelated to this PR.

It was the breaking change in dlang/druntime#2739. @thewilsonator force-merged it.

@WalterBright
Copy link
Member Author

then perhaps it would be better to jump straight to something that uses the frontend for data analysis

Because it took several tries for me to get this correct (making stupid mistakes). Getting it right here, where there is plenty of code to exercise all the cases, is a heluva lot easier than looking for subtle bugs in DFA, which is notoriously hard to debug.

other than slightly increasing memory usage

It's actually less memory usage, because the hash table with the same number of values (and hence quadruple the storage because of the key, hash, and link) is no longer needed.

@dlang-bot dlang-bot merged commit 0e6a07e into dlang:master Aug 29, 2019
@WalterBright WalterBright deleted the extraLabel branch August 29, 2019 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merge:auto-merge Severity:Refactoring No semantic changes to code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants