Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve order of evaluation spec, add lifetime of temporaries spec #2625

Merged
merged 5 commits into from
Apr 14, 2019

Conversation

andralex
Copy link
Member

This formalizes behavior already existing in the compiler. It simplifies the rvalue ref DIP.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @andralex!

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.

@andralex
Copy link
Member Author

cc @WalterBright @tgehr @TurkeyMan

spec/expression.dd Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor

/dev/shm/dtest/work/repo/dlang.org/web/spec/expression.html:144: [RAW MACRO LEAKAGE] $(SPEC_S Expressions,

@andralex
Copy link
Member Author

@thewilsonator found and fixed one missing paren

@thewilsonator thewilsonator added the 72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 12, 2019
@thewilsonator
Copy link
Contributor

Waiting to see input from Timon. (N.B the 72->h merge does not auto-merge)

spec/expression.dd Outdated Show resolved Hide resolved
spec/expression.dd Outdated Show resolved Hide resolved
Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

Approved with changes noted.

Fixed, thanks
@andralex andralex dismissed WalterBright’s stale review April 13, 2019 11:30

I've addressed the review

int f4() { writeln("f4() called"); return 1; }
int f3(int x) { writeln("f3() called"); return x + 1; }
int f1() { writeln("f1() called"); return 2; }
int f2() { writeln("f2() called"); return 3; }
Copy link
Member

Choose a reason for hiding this comment

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

Please declare them in order. Also, f4 should return 4, not 1. Etc. By scrambling the order and relationships, it leads the reader to look for something tricky going on, when there isn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good point

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member Author

Choose a reason for hiding this comment

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

oh wait

Copy link
Member Author

Choose a reason for hiding this comment

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

re-done

Copy link
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

Ah, lovely!

@jacob-carlborg
Copy link
Contributor

Please make sure the commits are squashed when merging.

@andralex andralex added auto-merge-squash and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Apr 14, 2019
@dlang-bot dlang-bot merged commit 30b1a34 into dlang:master Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants