Skip to content

improve try-finally handling of nofallthru#8171

Merged
WalterBright merged 1 commit intodlang:masterfrom
WalterBright:try-nofallthru
Apr 21, 2018
Merged

improve try-finally handling of nofallthru#8171
WalterBright merged 1 commit intodlang:masterfrom
WalterBright:try-nofallthru

Conversation

@WalterBright
Copy link
Member

If the try-body does not fall-through to the finally-body, we can improve the code generation by marking that block as an exit node (treated as a does-not-get-here node). Saves a few instructions in generated code.

@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#8171"

@WalterBright
Copy link
Member Author

Important to pull this because it makes my keynote presentation look better :-)

@WalterBright
Copy link
Member Author

On windows 32 and 64:

core.exception.AssertError@std\regex\package.d(1237): unittest failure

Doesn't seem to have anything to do with this change?

@braddr
Copy link
Member

braddr commented Apr 19, 2018

Removing auto-merge tag to de-prioritize the build. The windows platforms are failing consistently. There isn't a systemic windows platform failing on any other builds, which points strongly to this pull being broken in some way still.

Statement _body;
Statement finalbody;

bool bodyFallsThru; // true if _body falls through to finally
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is very similar in spirit to #5501, which was promptly shot down back then without offering an alternative solution.

Both add extra fields for metadata that can totally be inferred from the after-semantic AST, but is more convenient to collect during semantic analysis, and that is used by one particular backend implementation.

@ibuclaw, time to get on your soapbox again and settle this debate once and for all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this field does look pretty pointless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should ideally be a region that's reserved for per compiler specific information. For arguments sake, let's give it a void pointer type in the frontend. Here is stored information that can be read or set by the backend before, in-between, or after the semantic passes. In the case of LDC, I'd be surprised if you couldn't add your own PGO pass to achieve the same result - just at the slight cost of compile time.

@WalterBright
Copy link
Member Author

There isn't a systemic windows platform failing on any other builds, which points strongly to this pull being broken in some way still.

There was at the time, but now it is clear that there is something wrong with this PR.

@WalterBright WalterBright merged commit 5cb0541 into dlang:master Apr 21, 2018
@WalterBright WalterBright deleted the try-nofallthru branch April 21, 2018 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants