Add codegen support in smoke

Add codegen support in smoke

R=jmesserly@google.com

Committed: https://code.google.com/p/dart/source/detail?r=34087

[Siggi Cherem (dart-lang), 2014-03-11 20:00:37]

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/test/static_te...
File pkg/smoke/test/static_test.dart (right):

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/test/static_te...
pkg/smoke/test/static_test.dart:64: #noSuchMethod: const
Declaration(#noSuchMethod, Function, kind: METHOD),
(it didn't seem worth it to try to wrap these long lines in the code generator.)

[Jennifer Messerly, 2014-03-17 22:38:47]

https://codereview.chromium.org/194813007/diff/40001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/194813007/diff/40001/pkg/pkg.status#newcode218
pkg/pkg.status:218: smoke/test/codegen/recorder_test: Skip # Uses dart:io.
smoke/test/codegen/* ?

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
File pkg/smoke/lib/codegen/generator.dart (right):

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:11: library smoke.codegen.generator;
is this all intended as public api? SmokeCodeGenerator kind of looks that way
but I'm not sure about the AST-ish types

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:31: List<String> _getters = [];
final here & below?

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:37: Map<TypeIdentifier, TypeIdentifier>
_parents =
does it work to just say "final _parents" here instead of repeating the type
twice?

same for _declarations

maybe we need to file some bugs, but I thought for "final" it was supposed to do
the right thing.

(alternative if you care more about tooling support, assuming that it is an
issue, keeping the type annotation and the runtime type "new SplayTreeMap()"
would work too.)

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:135: String generateTopLevelDeclarations()
{
hmmm, I wonder if these should be methods that accept a StringBuffer. that's
usually a more performant pattern when composing a bunch of them

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:244: String name;
final for these fields?

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:290: List<String> get librariesUsed;
doc comment?

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:298: /// Create a string expression of the
form `"string"`.
maybe explain that it will be normalized

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:300: var value = string.replaceAll(r'\',
r'\\').replaceAll('\'', '\\\'');
for second replace, maybe write it as:

.replaceAll("'", r"\'");

IOW, double quotes on outside then you don't need to escape single quote inside

https://codereview.chromium.org/194813007/diff/100001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/194813007/diff/100001/pkg/pkg.status#newcode289
pkg/pkg.status:289: polymer/example/canonicalization/test/deploy_test: Fail, OK
# tests deploy only behavior
polymer/example/canonicalization/*?

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/lib/codegen/g...
File pkg/smoke/lib/codegen/generator.dart (right):

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/lib/codegen/g...
pkg/smoke/lib/codegen/generator.dart:13: // We use SplayTreeSet/Map to keep
generated code sorted.
maybe move this comment down to where they're used?

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/test/codegen/...
File pkg/smoke/test/codegen/generator_test.dart (right):

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/test/codegen/...
pkg/smoke/test/codegen/generator_test.dart:130: generator.addGetter('a');
hmmm. I wonder if this should be an error?

[Siggi Cherem (dart-lang), 2014-03-18 01:52:29]

thanks John, PTAL

https://codereview.chromium.org/194813007/diff/40001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/194813007/diff/40001/pkg/pkg.status#newcode218
pkg/pkg.status:218: smoke/test/codegen/recorder_test: Skip # Uses dart:io.
On 2014/03/17 22:38:48, John Messerly wrote:
> smoke/test/codegen/* ?

there is also smoke/test/codegen/generator_test which can technically run
anywhere :).. not that we need to, though.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
File pkg/smoke/lib/codegen/generator.dart (right):

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:11: library smoke.codegen.generator;
On 2014/03/17 22:38:48, John Messerly wrote:
> is this all intended as public api? SmokeCodeGenerator kind of looks that way
> but I'm not sure about the AST-ish types

Yes.

SmokeCodeGenerator will be heavily used extenrally. The main thing about the AST
is that it's needed for the 'addDeclaration', which is also public. I expect
it's mainly going to be used by recorder.dart, but it needs to be exposed
anyways.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:31: List<String> _getters = [];
On 2014/03/17 22:38:48, John Messerly wrote:
> final here & below?

Done.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:37: Map<TypeIdentifier, TypeIdentifier>
_parents =
On 2014/03/17 22:38:48, John Messerly wrote:
> does it work to just say "final _parents" here instead of repeating the type
> twice?
> 
> same for _declarations
> 
> maybe we need to file some bugs, but I thought for "final" it was supposed to
do
> the right thing.
> 
> (alternative if you care more about tooling support, assuming that it is an
> issue, keeping the type annotation and the runtime type "new SplayTreeMap()"
> would work too.)

Done.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:135: String generateTopLevelDeclarations()
{
On 2014/03/17 22:38:48, John Messerly wrote:
> hmmm, I wonder if these should be methods that accept a StringBuffer. that's
> usually a more performant pattern when composing a bunch of them

good idea, done.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:244: String name;
On 2014/03/17 22:38:48, John Messerly wrote:
> final for these fields?

Done.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:290: List<String> get librariesUsed;
On 2014/03/17 22:38:48, John Messerly wrote:
> doc comment?

Done.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:298: /// Create a string expression of the
form `"string"`.
On 2014/03/17 22:38:48, John Messerly wrote:
> maybe explain that it will be normalized

Done.

https://codereview.chromium.org/194813007/diff/40001/pkg/smoke/lib/codegen/ge...
pkg/smoke/lib/codegen/generator.dart:300: var value = string.replaceAll(r'\',
r'\\').replaceAll('\'', '\\\'');
On 2014/03/17 22:38:48, John Messerly wrote:
> for second replace, maybe write it as:
> 
> .replaceAll("'", r"\'");
> 
> IOW, double quotes on outside then you don't need to escape single quote
inside

Done.

https://codereview.chromium.org/194813007/diff/100001/pkg/pkg.status
File pkg/pkg.status (right):

https://codereview.chromium.org/194813007/diff/100001/pkg/pkg.status#newcode289
pkg/pkg.status:289: polymer/example/canonicalization/test/deploy_test: Fail, OK
# tests deploy only behavior
On 2014/03/17 22:38:48, John Messerly wrote:
> polymer/example/canonicalization/*?

seems like the diff was just because of a merge I did between patch#1 and
patch#2, but I'm not modifying anything here.

In this case though, there are more tests under that folder that are not
supposed to be marked (like polymer/example/canonicalization/test/dev_test)

I haven't tried, but does * work in the middle of a token (not sure how powerful
this file format regexps are), but if so, we could do:
polymer/example/canonicalization/test/deploy*

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/lib/codegen/g...
File pkg/smoke/lib/codegen/generator.dart (right):

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/lib/codegen/g...
pkg/smoke/lib/codegen/generator.dart:13: // We use SplayTreeSet/Map to keep
generated code sorted.
On 2014/03/17 22:38:48, John Messerly wrote:
> maybe move this comment down to where they're used?

Done. I moved it down to the first occurrence (there are 5 of them, but all next
to each other)

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/test/codegen/...
File pkg/smoke/test/codegen/generator_test.dart (right):

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/test/codegen/...
pkg/smoke/test/codegen/generator_test.dart:130: generator.addGetter('a');
On 2014/03/17 22:38:48, John Messerly wrote:
> hmmm. I wonder if this should be an error?

good question. I had it as an error initially, but then I realized it was better
to allow it. The way this is used with the recorder is that we'll be executing
static queries and request data from those queries to be appended to the
generator. Some queries might have common symbols and getters, even when they
are referring to different members. For example, I could request to append
getters for all fields in class A, and all fields in class B:

  class A { int x; }
  class B { int x; }

After running into these issues, I changed this code to allow it. That's why I
added this test so make sure that even if you add it more than once, the output
only reflects it once.

[Jennifer Messerly, 2014-03-19 00:15:47]

LGTM

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/test/codegen/...
File pkg/smoke/test/codegen/generator_test.dart (right):

https://codereview.chromium.org/194813007/diff/100001/pkg/smoke/test/codegen/...
pkg/smoke/test/codegen/generator_test.dart:130: generator.addGetter('a');
On 2014/03/18 01:52:29, Siggi Cherem (dart-lang) wrote:
> On 2014/03/17 22:38:48, John Messerly wrote:
> > hmmm. I wonder if this should be an error?
> 
> good question. I had it as an error initially, but then I realized it was
better
> to allow it. The way this is used with the recorder is that we'll be executing
> static queries and request data from those queries to be appended to the
> generator. Some queries might have common symbols and getters, even when they
> are referring to different members. For example, I could request to append
> getters for all fields in class A, and all fields in class B:
> 
>   class A { int x; }
>   class B { int x; }
> 
> After running into these issues, I changed this code to allow it. That's why I
> added this test so make sure that even if you add it more than once, the
output
> only reflects it once.

makes sense :)

[Siggi Cherem (dart-lang), 2014-03-19 00:21:31]

Committed patchset #4 manually as r34087 (presubmit successful).