Library privacy.

Library privacy.

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

[ngeoffray, 2012-03-13 12:47:11]

There are a few things where I'm not sure what you're doing.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dart
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:60: return
'_${getName(lib)}${name.slowToString()}\$${selector.argumentCount}$buffer';
line too long

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:131: name = 'lib';
Very surprised by this, it should not happen.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:194: case ElementKind.LIBRARY:
ditto.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:902: if (body.name.isPrivate()) {
Why do you need to do this and avoid it in codegen?

[floitsch, 2012-03-13 13:28:56]

LGTM.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dart
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:123: name = getterName(lib, element.name);
I agree that this makes the code maybe even simpler, but we don't need to care
for the library for static elements.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:129: name =
'$name\$${functionElement.parameterCount(compiler)}';
Actually we don't need to care for the parameterCount either.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:902: if (body.name.isPrivate()) {
I'm don't think this is necessary at all.
Constructor bodies are statically resolved.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/codeg...
File dart/frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/codeg...
dart/frog/leg/ssa/codegen.dart:529: if (node.name.isPrivate()) {
static elements don't need to be mangled wrt privacy.

[kasperl, 2012-03-13 13:43:51]

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dart
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:43: if (name.isPrivate()) {
How about refactoring this pattern?

[ahe, 2012-03-13 15:48:58]

Hi Kasper, Nicolas, and Florian,

Thank you for your suggestions. Florian has a good point about not needing to
mangle static methods, and this should help me clean up the constructor mess.

Stay tuned for the next revision.

Cheers,
Peter

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dart
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:123: name = getterName(lib, element.name);
On 2012/03/13 13:28:56, floitsch wrote:
> I agree that this makes the code maybe even simpler, but we don't need to care
> for the library for static elements.

Good point, I didn't think about that.

Actually, the code may be simpler if static methods and constructors aren't
prefixed. I didn't like what I had to do for constructors.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:129: name =
'$name\$${functionElement.parameterCount(compiler)}';
On 2012/03/13 13:28:56, floitsch wrote:
> Actually we don't need to care for the parameterCount either.

Another good point I didn't think about.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:131: name = 'lib';
On 2012/03/13 12:47:11, ngeoffray wrote:
> Very surprised by this, it should not happen.

It is because I started using getName on libraries to get a unique prefix. See
line 44.

[ngeoffray, 2012-03-17 20:59:09]

Hi Peter, any updates on this CL? It is blocking swarm :)

[ahe, 2012-03-17 21:01:15]

On 2012/03/17 20:59:09, ngeoffray wrote:
> Hi Peter, any updates on this CL? It is blocking swarm :)

I guess I could submit it as is and work out the static resolution later.

[ahe, 2012-03-17 21:01:15]

On 2012/03/17 20:59:09, ngeoffray wrote:
> Hi Peter, any updates on this CL? It is blocking swarm :)

I guess I could submit it as is and work out the static resolution later.

[ngeoffray, 2012-03-17 21:06:14]

On 2012/03/17 21:01:15, ahe wrote:
> On 2012/03/17 20:59:09, ngeoffray wrote:
> > Hi Peter, any updates on this CL? It is blocking swarm :)
> 
> I guess I could submit it as is and work out the static resolution later.

As you wish. I was waiting for your new patchset before reviewing. It's not
urgent, but if it could go higher on your priority list, it'd be nice :)

[ahe, 2012-03-17 22:19:44]

PTAL

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dart
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:43: if (name.isPrivate()) {
On 2012/03/13 13:43:51, kasperl wrote:
> How about refactoring this pattern?

Done.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/namer.dar...
dart/frog/leg/namer.dart:60: return
'_${getName(lib)}${name.slowToString()}\$${selector.argumentCount}$buffer';
On 2012/03/13 12:47:11, ngeoffray wrote:
> line too long

Done.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:902: if (body.name.isPrivate()) {
On 2012/03/13 13:28:56, floitsch wrote:
> I'm don't think this is necessary at all.
> Constructor bodies are statically resolved.

I'm not sure how to invoke a statically resolved method on this. I have changed
how these names are used and generated.

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/codeg...
File dart/frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/1/dart/frog/leg/ssa/codeg...
dart/frog/leg/ssa/codegen.dart:529: if (node.name.isPrivate()) {
On 2012/03/13 13:28:56, floitsch wrote:
> static elements don't need to be mangled wrt privacy.

Done.

[ngeoffray, 2012-03-18 11:49:59]

LGTM!

Nice that you actually don't have to do anything for statically resolved
methods, due to not exporting private members.

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/emitt...
File dart/frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/emitt...
dart/frog/leg/emitter.dart:574: compiler.universe.libraries.getValues();
missing 2 spaces.

[floitsch, 2012-03-18 18:29:30]

LGTM.

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/namer...
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/namer...
dart/frog/leg/namer.dart:43: return '_${getName(lib)}${name.slowToString()}';
Unless I'm wrong this means that a user can "fake" private access by quessing
the library-name.
Add a TODO?.

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/namer...
dart/frog/leg/namer.dart:50: // TODO(floitsch): mangle, while preserving
uniqueness.
I think you can remove the TODO here.

[ahe, 2012-03-18 19:17:05]

Let me know what you think.

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/namer...
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/namer...
dart/frog/leg/namer.dart:43: return '_${getName(lib)}${name.slowToString()}';
On 2012/03/18 18:29:30, floitsch wrote:
> Unless I'm wrong this means that a user can "fake" private access by quessing
> the library-name.
> Add a TODO?.

I think I covered that by prefixing with _

[ahe, 2012-03-18 21:12:04]

Thank you.

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/namer...
File dart/frog/leg/namer.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/namer...
dart/frog/leg/namer.dart:50: // TODO(floitsch): mangle, while preserving
uniqueness.
On 2012/03/18 18:29:30, floitsch wrote:
> I think you can remove the TODO here.

Done.

[ahe, 2012-03-18 21:15:55]

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/emitt...
File dart/frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9689045/diff/13001/dart/frog/leg/emitt...
dart/frog/leg/emitter.dart:574: compiler.universe.libraries.getValues();
On 2012/03/18 11:49:59, ngeoffray wrote:
> missing 2 spaces.

Done.