Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954653002:
Add proposal for new mixin declaration syntax. (Closed)
|
Created:
3 years, 6 months ago by Lasse Reichstein Nielsen Modified:
3 years, 5 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd proposal for new mixin declaration syntax.
R=floitsch@google.com
Committed: https://github.com/dart-lang/sdk/commit/aeacff4f2bf7df1a88950d8714db4a3430958780
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address comments. #Messages
Total messages: 11 (3 generated)
lrn@google.com changed reviewers: + floitsch@google.com
Markdownified by docs, please check if there are silly mistakes I have missed..
LGTM. https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... File docs/language/informal/mixin-declaration.md (right): https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:30: * Nobody understands how the super-feature actually works (http://dartbug.com/29758, http://dartbug.com/25765, missing closing ")" https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:38: *mixinDeclaration* :*metadata*? 'mixin' *identifier* *typeParameters*? <br> space on both sides of ":" ? https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:72: Mixin application syntax is unchanged. A mixin application `S with M` introduces a *class* with superclass `S`, implementing `M` and with copies of all the non-static members of `M`. As usual code in the copies of members retain the static scope of their original declaration. As usual, https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:95: ``` This didn't work. You need to unintend the ```. https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:153: class Foo extends S implements I { // Used as mixin *and* class Nit :) Finish with "."
brianwilkerson@google.com changed reviewers: + brianwilkerson@google.com
DBC https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... File docs/language/informal/mixin-declaration.md (right): https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:39: ('super' *types*)? ('implements' *types*)? '{' <em>mixinMember</em>* '}' Did you mean "requires" rather than "super" here? How about on lines 61 and 62? (There might be other places as well.)
https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... File docs/language/informal/mixin-declaration.md (right): https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:39: ('super' *types*)? ('implements' *types*)? '{' <em>mixinMember</em>* '}' On 2017/06/23 14:37:03, Brian Wilkerson wrote: > Did you mean "requires" rather than "super" here? How about on lines 61 and 62? > (There might be other places as well.) Good catch!
https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... File docs/language/informal/mixin-declaration.md (right): https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:30: * Nobody understands how the super-feature actually works (http://dartbug.com/29758, http://dartbug.com/25765, On 2017/06/23 13:33:35, floitsch wrote: > missing closing ")" Done. https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:38: *mixinDeclaration* :*metadata*? 'mixin' *identifier* *typeParameters*? <br> On 2017/06/23 13:33:36, floitsch wrote: > space on both sides of ":" ? Done. https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:39: ('super' *types*)? ('implements' *types*)? '{' <em>mixinMember</em>* '}' I did, thanks :). https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:39: ('super' *types*)? ('implements' *types*)? '{' <em>mixinMember</em>* '}' On 2017/06/23 19:49:25, floitsch wrote: > On 2017/06/23 14:37:03, Brian Wilkerson wrote: > > Did you mean "requires" rather than "super" here? How about on lines 61 and > 62? > > (There might be other places as well.) > > Good catch! Done. https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:72: Mixin application syntax is unchanged. A mixin application `S with M` introduces a *class* with superclass `S`, implementing `M` and with copies of all the non-static members of `M`. As usual code in the copies of members retain the static scope of their original declaration. On 2017/06/23 13:33:35, floitsch wrote: > As usual, Done. https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:95: ``` On 2017/06/23 13:33:36, floitsch wrote: > This didn't work. You need to unintend the ```. Done. https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:153: class Foo extends S implements I { // Used as mixin *and* class On 2017/06/23 13:33:36, floitsch wrote: > Nit :) Finish with "." Done.
Description was changed from ========== Add proposal for new mixin declaration syntax. ========== to ========== Add proposal for new mixin declaration syntax. R=floitsch@google.com Committed: https://github.com/dart-lang/sdk/commit/aeacff4f2bf7df1a88950d8714db4a3430958780 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as aeacff4f2bf7df1a88950d8714db4a3430958780 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... File docs/language/informal/mixin-declaration.md (right): https://codereview.chromium.org/2954653002/diff/1/docs/language/informal/mixi... docs/language/informal/mixin-declaration.md:95: ``` On 2017/06/26 09:08:29, Lasse Reichstein Nielsen wrote: > On 2017/06/23 13:33:36, floitsch wrote: > > This didn't work. You need to unintend the ```. > > Done. No you didn't. I fixed it on master. Please check if other changes didn't make it into the committed version.
Message was sent while issue was closed.
Doh. I guess I somehow managed to undo that change again after doing it. The rest looks fine. Thanks. |
