Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10917285:
Stub implementation of patch invariants for the patch refactoring. (Closed)
|
Created:
8 years, 3 months ago by Johnni Winther Modified:
8 years, 3 months ago CC:
reviews_dartlang.org, floitsch, karlklose, Lasse Reichstein Nielsen, kasperl Visibility:
Public. |
DescriptionStub implementation of patch invariants for the patch refactoring.
This is the first part of the patch refactoring which implements
a stub version of the invariants needed for the new patch implementation.
See the library comment in lib/compiler/patch_parser.dart for a detailed
description of the new patch system, the terminology and the invariants.
Committed: https://code.google.com/p/dart/source/detail?r=12687
Patch Set 1 #Patch Set 2 : Leftovers from rebase. #
Total comments: 90
Patch Set 3 : Updated cf. comments #
Total comments: 61
Patch Set 4 : Updated cf. comments #Messages
Total messages: 13 (0 generated)
Initial comments https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:998: type = compiler.computeFunctionType(declaration, Why this change? https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1173: bool get isObject => supertype === null; This is fragile: the class may not have been resolved yet. It'd be great if you could avoid having this getter. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/enqueue.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:106: print('$element: isMember=${element.isMember()} isInstanceMember=${element.isInstanceMember()}'); Remove debugging code. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/patch_parser.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:23: * The fields [Element.isPatched] and [Element.isPatch] can be used to determine Should isPatched be isExternal? https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:71: * With patches we have two views on elements: As the 'declaration' which As -> as https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:81: * conveniently available through the [Element.declaration:] and Remove ':' https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:19: TreeElementMapping([Element element]) Could you not make it optional? Also you can write: Element this.currentElement https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:27: if (node is FunctionExpression && node.modifiers != null) { Put this 'if' in the assert. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:30: if (!element.isErroneous() && currentElement != null && element.isPatch) { ditto. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/builder.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:156: HGraph build(WorkItem work) { I'd really prefer if this guy did not have to care about implementation/declaration. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:900: if (function.modifiers !== null) { Put the if in the assert https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2090: push(new HStatic(element.declaration)); Why this change? https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2778: if (tryInlineMethod(element.implementation, selector, node.arguments)) return; line too long https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2784: element.implementation, inputs); ditto https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/universe/universe.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/universe/universe.dart:303: assert(element.isImplementation); No sure you need this restriction. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/universe/universe.dart:360: assert(element.isImplementation); ditto https://codereview.chromium.org/10917285/diff/3001/tests/html/html.status File tests/html/html.status (right): https://codereview.chromium.org/10917285/diff/3001/tests/html/html.status#new... tests/html/html.status:35: websql_test: Pass, Fail # Possibly Issue 4517 This is not related to this CL right? Maybe make it a different CL.
This is very nice. As far as I'm concerned, there is one issue that is making me reluctant to "accept" this change: the use of asserts. Let's discuss alternatives. There are now so many asserts that I feel we need to throw an object with more context. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/compile_time_constants.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/compile_time_constants.dart:535: * Invariant: [target] must be the implementation element. the -> an. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/compile_time_constants.dart:568: // The constructor must be the implementation to ensure that field the -> an. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/compile_time_constants.dart:570: constructor = constructor.implementation; Is this temporary? https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/compile_time_constants.dart:632: * Invariant: [constructor] must be the implementation element. This is not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/compiler.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/compiler.dart:43: * Invariant: [element] must be the declaration element. This is not a documentation comment. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/elements/elements.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:174: //---------------------------------------------------------------------------- What does this comment apply to? I think this is documentation for several methods, but that doesn't work. You have to repeat the comment. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/enqueue.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:12: : codegen = new Enqueuer('codegen enqueuer', compiler, Nice. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:90: * Invariant: [element] must be the declaration element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:94: return universe.generatedCode[element].toString(); Why are you calling toString here? https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:98: * Invariant: [element] must be the declaration element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:316: * Invariant: [element] must be the declaration element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/enqueue.dart:378: String toString() => name; I prefer this: 'Enqueuer($name)' This makes debugging by print easier, as getting the class name is hard. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/js_backend/backend.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/js_backend/backend.dart:625: * Invariant: [element] must be the declaration element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/patch_parser.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:5: /** Add something like: TOOO(johnniwinther): The terminology and invariants described below will not hold until CL 10905305 has been committed. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:6: * This library contains the infrastructure to parse and integrate patches. patches -> patch file. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:41: * Ghost: A class or function (or even field) which is declared in a I don't like this terminology. I have heard Anders, Mads, etc use the term "injected". https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:53: * class PatchedClass { // The origin class. The -> An. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:56: * external void patchedMethod(); // The origin method. The -> An. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:63: * patch class PatchedClass { // The patch class. The -> An. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:65: * patch void patchedMethod() {} // The patch method. The -> An. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/patch_parser.dart:99: * For [ClassElement] and [FunctionElement] we mostly use declarations. I don't see it this way. Yes, we mostly store declarations in TreeElements, but it is the implementation that we resolve, type check, infer types for, build SSA graph for, etc. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/builder.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:156: HGraph build(WorkItem work) { On 2012/09/17 12:46:24, ngeoffray wrote: > I'd really prefer if this guy did not have to care about > implementation/declaration. Agreed, ideally, the only change to this method should be line 158, something like: Element element = work.element.implementation; https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:345: * Invariant: [function] must be the implementation element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:894: * Invariant: [functionElement] must be the implementation element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:969: * Invariant: [function] must be the implementation element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:1021: * Invariant: [element] must be the implementation element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:1068: * Invariant: [constructor] and [constructors] must all be implementation Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:1165: if (!enclosingClass.isObject) { I'm not sure about the implementation of this. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:1361: * Invariant: [functionElement] must be the implementation element. Not documentation. https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/codegen.dart (right): https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:16: js.Fun buildJavaScriptFunction(FunctionElement element, Yay! Fun with js! https://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:76: Map<Element, String> parameterNames = getParameterNames(work); Eliminate getParameterNames? https://codereview.chromium.org/10917285/diff/3001/tests/html/html.status File tests/html/html.status (right): https://codereview.chromium.org/10917285/diff/3001/tests/html/html.status#new... tests/html/html.status:35: websql_test: Pass, Fail # Possibly Issue 4517 On 2012/09/17 12:46:24, ngeoffray wrote: > This is not related to this CL right? Maybe make it a different CL. For sure!
PTAL http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/compile_time_constants.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/compile_time_constants.dart:535: * Invariant: [target] must be the implementation element. On 2012/09/18 11:25:54, ahe wrote: > the -> an. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/compile_time_constants.dart:568: // The constructor must be the implementation to ensure that field On 2012/09/18 11:25:54, ahe wrote: > the -> an. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/compile_time_constants.dart:570: constructor = constructor.implementation; On 2012/09/18 11:25:54, ahe wrote: > Is this temporary? No. That is why I put in the comment. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/compile_time_constants.dart:632: * Invariant: [constructor] must be the implementation element. On 2012/09/18 11:25:54, ahe wrote: > This is not documentation. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/compiler.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/compiler.dart:43: * Invariant: [element] must be the declaration element. On 2012/09/18 11:25:54, ahe wrote: > This is not a documentation comment. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/elements/elements.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/elements/elements.dart:174: //---------------------------------------------------------------------------- On 2012/09/18 11:25:54, ahe wrote: > What does this comment apply to? I think this is documentation for several > methods, but that doesn't work. You have to repeat the comment. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/elements/elements.dart:998: type = compiler.computeFunctionType(declaration, On 2012/09/17 12:46:24, ngeoffray wrote: > Why this change? To enforce the invariant that DartType.element is always a declaration. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/elements/elements.dart:1173: bool get isObject => supertype === null; On 2012/09/17 12:46:24, ngeoffray wrote: > This is fragile: the class may not have been resolved yet. It'd be great if you > could avoid having this getter. Changed to a less fragile method call. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/enqueue.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/enqueue.dart:90: * Invariant: [element] must be the declaration element. On 2012/09/18 11:25:54, ahe wrote: > Not documentation. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/enqueue.dart:94: return universe.generatedCode[element].toString(); On 2012/09/18 11:25:54, ahe wrote: > Why are you calling toString here? I have no idea. It was there before this CL. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/enqueue.dart:98: * Invariant: [element] must be the declaration element. On 2012/09/18 11:25:54, ahe wrote: > Not documentation. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/enqueue.dart:106: print('$element: isMember=${element.isMember()} isInstanceMember=${element.isInstanceMember()}'); On 2012/09/17 12:46:24, ngeoffray wrote: > Remove debugging code. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/enqueue.dart:316: * Invariant: [element] must be the declaration element. On 2012/09/18 11:25:54, ahe wrote: > Not documentation. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/enqueue.dart:378: String toString() => name; On 2012/09/18 11:25:54, ahe wrote: > I prefer this: 'Enqueuer($name)' > > This makes debugging by print easier, as getting the class name is hard. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/js_backend/backend.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/js_backend/backend.dart:625: * Invariant: [element] must be the declaration element. On 2012/09/18 11:25:54, ahe wrote: > Not documentation. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/patch_parser.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:5: /** On 2012/09/18 11:25:54, ahe wrote: > Add something like: > > TOOO(johnniwinther): The terminology and invariants described below will not > hold until CL 10905305 has been committed. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:6: * This library contains the infrastructure to parse and integrate patches. On 2012/09/18 11:25:54, ahe wrote: > patches -> patch file. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:23: * The fields [Element.isPatched] and [Element.isPatch] can be used to determine On 2012/09/17 12:46:24, ngeoffray wrote: > Should isPatched be isExternal? That would only be fitting for functions. Class and libraries are never themselves external even when they are patched. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:41: * Ghost: A class or function (or even field) which is declared in a On 2012/09/18 11:25:54, ahe wrote: > I don't like this terminology. I have heard Anders, Mads, etc use the term > "injected". I'll change to injected. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:53: * class PatchedClass { // The origin class. On 2012/09/18 11:25:54, ahe wrote: > The -> An. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:56: * external void patchedMethod(); // The origin method. On 2012/09/18 11:25:54, ahe wrote: > The -> An. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:63: * patch class PatchedClass { // The patch class. On 2012/09/18 11:25:54, ahe wrote: > The -> An. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:65: * patch void patchedMethod() {} // The patch method. On 2012/09/18 11:25:54, ahe wrote: > The -> An. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:71: * With patches we have two views on elements: As the 'declaration' which On 2012/09/17 12:46:24, ngeoffray wrote: > As -> as Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:81: * conveniently available through the [Element.declaration:] and On 2012/09/17 12:46:24, ngeoffray wrote: > Remove ':' Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/patch_parser.dart:99: * For [ClassElement] and [FunctionElement] we mostly use declarations. On 2012/09/18 11:25:54, ahe wrote: > I don't see it this way. Yes, we mostly store declarations in TreeElements, but > it is the implementation that we resolve, type check, infer types for, build SSA > graph for, etc. Updated http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/resolver.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/resolver.dart:19: TreeElementMapping([Element element]) On 2012/09/17 12:46:24, ngeoffray wrote: > Could you not make it optional? Also you can write: > Element this.currentElement Several call-sites uses optionality. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/resolver.dart:27: if (node is FunctionExpression && node.modifiers != null) { On 2012/09/17 12:46:24, ngeoffray wrote: > Put this 'if' in the assert. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/resolver.dart:30: if (!element.isErroneous() && currentElement != null && element.isPatch) { On 2012/09/17 12:46:24, ngeoffray wrote: > ditto. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/ssa/builder.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:156: HGraph build(WorkItem work) { On 2012/09/18 11:25:54, ahe wrote: > On 2012/09/17 12:46:24, ngeoffray wrote: > > I'd really prefer if this guy did not have to care about > > implementation/declaration. > > Agreed, ideally, the only change to this method should be line 158, something > like: > > Element element = work.element.implementation; Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:345: * Invariant: [function] must be the implementation element. On 2012/09/18 11:25:54, ahe wrote: > Not documentation. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:894: * Invariant: [functionElement] must be the implementation element. On 2012/09/18 11:25:54, ahe wrote: > Not documentation. Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:900: if (function.modifiers !== null) { On 2012/09/17 12:46:24, ngeoffray wrote: > Put the if in the assert Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:1165: if (!enclosingClass.isObject) { On 2012/09/18 11:25:54, ahe wrote: > I'm not sure about the implementation of this. Changed. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:2090: push(new HStatic(element.declaration)); On 2012/09/17 12:46:24, ngeoffray wrote: > Why this change? For some reason putting the invariant on the element of HStatic to be a declaration gave the cleanest cut. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:2778: if (tryInlineMethod(element.implementation, selector, node.arguments)) return; On 2012/09/17 12:46:24, ngeoffray wrote: > line too long Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/builder.dart:2784: element.implementation, inputs); On 2012/09/17 12:46:24, ngeoffray wrote: > ditto Done. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/ssa/codegen.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/ssa/codegen.dart:76: Map<Element, String> parameterNames = getParameterNames(work); On 2012/09/18 11:25:54, ahe wrote: > Eliminate getParameterNames? It's used elsewhere. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... File lib/compiler/implementation/universe/universe.dart (right): http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/universe/universe.dart:303: assert(element.isImplementation); On 2012/09/17 12:46:24, ngeoffray wrote: > No sure you need this restriction. The only call-site enforces the invariant so I'll keep it here. http://codereview.chromium.org/10917285/diff/3001/lib/compiler/implementation... lib/compiler/implementation/universe/universe.dart:360: assert(element.isImplementation); On 2012/09/17 12:46:24, ngeoffray wrote: > ditto The call-sites enforces the invariant so I'll keep it here. http://codereview.chromium.org/10917285/diff/3001/tests/html/html.status File tests/html/html.status (right): http://codereview.chromium.org/10917285/diff/3001/tests/html/html.status#newc... tests/html/html.status:35: websql_test: Pass, Fail # Possibly Issue 4517 On 2012/09/17 12:46:24, ngeoffray wrote: > This is not related to this CL right? Maybe make it a different CL. I will do that.
LGTM. I'm not sure if element should be spannable. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/compile_time_constants.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/compile_time_constants.dart:547: assert(invariant(target, target.isImplementation)); First argument should be node. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/compiler.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/compiler.dart:976: bool invariant(Spanable spanable, bool condition, {String message: null}) => Please don't use => shorthand when it doesn't fit on one line. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/elements/elements.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:110: class Element implements Hashable, Spanable { It's "spanned" and "spanning", so shouldn't it be spannable? http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:174: //---------------------------------------------------------------------------- Can you remove this comment now? http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:180: * Is [:true:] iff this element has a corresponding patch. Don't use "iff" or "if and only if" in documentation. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1044: Node parseNode(DiagnosticListener listener) { I think this method is fishy. It concerns me greatly that you don't call parseNode anymore. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1215: * Return [:true:] if this element is the [:Object:] class for the [compiler]. This is not using "iff" and is perfectly clear. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:26: assert(invariant(node, () { How about allowing function arguments in invariant. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); Why is this necessary? http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1641: world.registerStaticUse(constructor.declaration); Why? http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:2732: Element localLookup(SourceString name) => null; I don't understand this change. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/builder.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:363: assert(invariant(function, function.isImplementation)); First argument should be node. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/typechecker.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/typechecker.dart:167: assert(element == null || invariant(element, element.isDeclaration)); ===
LGTM too http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/compiler.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/compiler.dart:44: * Invariant: [element] must be an declaration element. an -> a http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/builder.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:170: Element element = work.element.implementation; I think in this method you could just use element (knowing it's a declaration). The buildMethod/buildLanzyInitializer/buildConstructor would then use the implementation. This is to avoid changing line 188 and 211. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:971: TreeElements treeElements = Please add a comment here on why you need to use declaration. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:975: compiler.enqueuer.codegen.addToWorkList(bodyElement.declaration, We just constructed bodyElement. Could we avoid having both a declaration and an implementation for it? http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2122: push(new HStatic(element.declaration)); Please add TODO to try to get rid of this. Maybe also file a bug to reference it here. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2131: push(new HStatic(element.declaration)); Ditto. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2587: = function.implementation.computeSignature(compiler); How can a declaration and an implementation have a different sisnature? http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2654: HInstruction target = new HStatic(element.declaration); Add TODO http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2662: element.implementation, ditto. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2801: HInstruction target = new HStatic(constructor.declaration); ditto http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2806: constructor.implementation, ditto http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2851: if (tryInlineMethod(element.implementation, selector, node.arguments)) { I would fetch the implementation in tryInlineMethod instead. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2859: element.implementation, TODO http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/codegen.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:23: Element sourceElement = element.patch == null ? element : element.patch; Can you remove this todo and use element.implementation instead? http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:77: work.element.computeSignature(compiler).forEachParameter((element) { Please use parameterNames and fetch the declaration from the elements. Otherwise the fact that you want declarations is too hidden behind the scenes.
http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/compile_time_constants.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/compile_time_constants.dart:547: assert(invariant(target, target.isImplementation)); On 2012/09/20 11:12:07, ahe wrote: > First argument should be node. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/compiler.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/compiler.dart:44: * Invariant: [element] must be an declaration element. On 2012/09/20 11:38:37, ngeoffray wrote: > an -> a Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/compiler.dart:976: bool invariant(Spanable spanable, bool condition, {String message: null}) => On 2012/09/20 11:12:07, ahe wrote: > Please don't use => shorthand when it doesn't fit on one line. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/elements/elements.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:110: class Element implements Hashable, Spanable { On 2012/09/20 11:12:07, ahe wrote: > It's "spanned" and "spanning", so shouldn't it be spannable? Probably :) http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:174: //---------------------------------------------------------------------------- On 2012/09/20 11:12:07, ahe wrote: > Can you remove this comment now? Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:180: * Is [:true:] iff this element has a corresponding patch. On 2012/09/20 11:12:07, ahe wrote: > Don't use "iff" or "if and only if" in documentation. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/elements/elements.dart:1044: Node parseNode(DiagnosticListener listener) { On 2012/09/20 11:12:07, ahe wrote: > I think this method is fishy. It concerns me greatly that you don't call > parseNode anymore. Bad merge. These should not have been removed until a later CL. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:26: assert(invariant(node, () { On 2012/09/20 11:12:07, ahe wrote: > How about allowing function arguments in invariant. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); On 2012/09/20 11:12:07, ahe wrote: > Why is this necessary? [target] might be the implementation element and only declaration elements may be registered. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1641: world.registerStaticUse(constructor.declaration); On 2012/09/20 11:12:07, ahe wrote: > Why? Same reason as before: [constructor] might be an implementation element. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:2732: Element localLookup(SourceString name) => null; On 2012/09/20 11:12:07, ahe wrote: > I don't understand this change. [lookup] is defined in [Scope] to call [:parent.lookup(name):] if [localLookup] returns [:null:]. The change makes [lexicalLookup] work. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/builder.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:170: Element element = work.element.implementation; On 2012/09/20 11:38:37, ngeoffray wrote: > I think in this method you could just use element (knowing it's a declaration). > The buildMethod/buildLanzyInitializer/buildConstructor would then use the > implementation. > > This is to avoid changing line 188 and 211. But then we would have to change line 182, 184, and 198 which all require implementation elements :( http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:363: assert(invariant(function, function.isImplementation)); On 2012/09/20 11:12:07, ahe wrote: > First argument should be node. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:971: TreeElements treeElements = On 2012/09/20 11:38:37, ngeoffray wrote: > Please add a comment here on why you need to use declaration. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:975: compiler.enqueuer.codegen.addToWorkList(bodyElement.declaration, On 2012/09/20 11:38:37, ngeoffray wrote: > We just constructed bodyElement. Could we avoid having both a declaration and an > implementation for it? If [constructor] is a patch we will need both. This is though only generated in a later CL. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2122: push(new HStatic(element.declaration)); On 2012/09/20 11:38:37, ngeoffray wrote: > Please add TODO to try to get rid of this. Maybe also file a bug to reference it > here. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2131: push(new HStatic(element.declaration)); On 2012/09/20 11:38:37, ngeoffray wrote: > Ditto. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2587: = function.implementation.computeSignature(compiler); On 2012/09/20 11:38:37, ngeoffray wrote: > How can a declaration and an implementation have a different sisnature? The signatures have different elements for the parameters. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2654: HInstruction target = new HStatic(element.declaration); On 2012/09/20 11:38:37, ngeoffray wrote: > Add TODO Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2662: element.implementation, On 2012/09/20 11:38:37, ngeoffray wrote: > ditto. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2801: HInstruction target = new HStatic(constructor.declaration); On 2012/09/20 11:38:37, ngeoffray wrote: > ditto Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2806: constructor.implementation, On 2012/09/20 11:38:37, ngeoffray wrote: > ditto Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2851: if (tryInlineMethod(element.implementation, selector, node.arguments)) { On 2012/09/20 11:38:37, ngeoffray wrote: > I would fetch the implementation in tryInlineMethod instead. Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/builder.dart:2859: element.implementation, On 2012/09/20 11:38:37, ngeoffray wrote: > TODO Done. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/ssa/codegen.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:23: Element sourceElement = element.patch == null ? element : element.patch; On 2012/09/20 11:38:37, ngeoffray wrote: > Can you remove this todo and use element.implementation instead? In a later CL. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/ssa/codegen.dart:77: work.element.computeSignature(compiler).forEachParameter((element) { On 2012/09/20 11:38:37, ngeoffray wrote: > Please use parameterNames and fetch the declaration from the elements. Otherwise > the fact that you want declarations is too hidden behind the scenes. I can't. There is no link between a parameter element of the implementation and the corresponding parameter element of the declaration. I didn't want to make [VariableElement] patchable. Added a comment instead. http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/typechecker.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/typechecker.dart:167: assert(element == null || invariant(element, element.isDeclaration)); On 2012/09/20 11:12:07, ahe wrote: > === Done.
http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); On 2012/09/21 09:18:25, Johnni Winther wrote: > On 2012/09/20 11:12:07, ahe wrote: > > Why is this necessary? > > [target] might be the implementation element and only declaration elements may > be registered. Why might it be the implementation element?
http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); On 2012/09/21 09:26:42, ahe wrote: > On 2012/09/21 09:18:25, Johnni Winther wrote: > > On 2012/09/20 11:12:07, ahe wrote: > > > Why is this necessary? > > > > [target] might be the implementation element and only declaration elements may > > be registered. > > Why might it be the implementation element? If we are in a patch library, target might resolve to a patch element.
http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... File lib/compiler/implementation/resolver.dart (right): http://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementatio... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); On 2012/09/21 10:08:13, Johnni Winther wrote: > On 2012/09/21 09:26:42, ahe wrote: > > On 2012/09/21 09:18:25, Johnni Winther wrote: > > > On 2012/09/20 11:12:07, ahe wrote: > > > > Why is this necessary? > > > > > > [target] might be the implementation element and only declaration elements > may > > > be registered. > > > > Why might it be the implementation element? > > If we are in a patch library, target might resolve to a patch element. Why?
https://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); On 2012/09/21 12:11:31, ahe wrote: > On 2012/09/21 10:08:13, Johnni Winther wrote: > > On 2012/09/21 09:26:42, ahe wrote: > > > On 2012/09/21 09:18:25, Johnni Winther wrote: > > > > On 2012/09/20 11:12:07, ahe wrote: > > > > > Why is this necessary? > > > > > > > > [target] might be the implementation element and only declaration elements > > may > > > > be registered. > > > > > > Why might it be the implementation element? > > > > If we are in a patch library, target might resolve to a patch element. > > Why? Because the patch scope (the scope created within a patch) returns the patch elements of the same patch library on lookup.
https://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); On 2012/09/21 12:20:14, Johnni Winther wrote: > On 2012/09/21 12:11:31, ahe wrote: > > On 2012/09/21 10:08:13, Johnni Winther wrote: > > > On 2012/09/21 09:26:42, ahe wrote: > > > > On 2012/09/21 09:18:25, Johnni Winther wrote: > > > > > On 2012/09/20 11:12:07, ahe wrote: > > > > > > Why is this necessary? > > > > > > > > > > [target] might be the implementation element and only declaration > elements > > > may > > > > > be registered. > > > > > > > > Why might it be the implementation element? > > > > > > If we are in a patch library, target might resolve to a patch element. > > > > Why? > > Because the patch scope (the scope created within a patch) returns the patch > elements of the same patch library on lookup. Why?
https://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementati... File lib/compiler/implementation/resolver.dart (right): https://codereview.chromium.org/10917285/diff/12001/lib/compiler/implementati... lib/compiler/implementation/resolver.dart:1564: if (!target.isClass()) world.registerStaticUse(target.declaration); On 2012/09/21 12:21:22, ahe wrote: > On 2012/09/21 12:20:14, Johnni Winther wrote: > > On 2012/09/21 12:11:31, ahe wrote: > > > On 2012/09/21 10:08:13, Johnni Winther wrote: > > > > On 2012/09/21 09:26:42, ahe wrote: > > > > > On 2012/09/21 09:18:25, Johnni Winther wrote: > > > > > > On 2012/09/20 11:12:07, ahe wrote: > > > > > > > Why is this necessary? > > > > > > > > > > > > [target] might be the implementation element and only declaration > > elements > > > > may > > > > > > be registered. > > > > > > > > > > Why might it be the implementation element? > > > > > > > > If we are in a patch library, target might resolve to a patch element. > > > > > > Why? > > > > Because the patch scope (the scope created within a patch) returns the patch > > elements of the same patch library on lookup. > > Why? We discussed this offline. Seems that our invariants aren't as tight as they should be, yet. Johnni will add a TODO explaining what direction we will take in the next CL. |
