Introduce CodeBuffer as StringBuffer replacement in compiler.

Introduce CodeBuffer as StringBuffer replacement in compiler.

CodeBuffer stores source location markers along with emitted JavaScript code.
When CodeBuffer content is added to another CodeBuffer, the markers are added as well.

CodeBuffers will be used to propagate source mappings from inside ssa codegen to emitter,
see https://chromiumcodereview.appspot.com/10695174/ for details.

R=floitsch@google.com,kasperl@google.com

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

[floitsch, 2012-07-12 16:29:09]

LGTM.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/code_buffer.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:6: StringBuffer _buffer;
Any reason those fields are marked as private?
Unless you think somebody could mistakenly use a field or a method we generally
keep members public.

This is, of course, not necessarily true, if the CodeBuffers are exposed through
the public API.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:22: void add(Object object) {
Use "var" if you want to show that this function accepts more than one type, but
needs to dispatch.
Also add comment /** */ explaining what it takes.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:33: int offsetDelta =
_buffer.length + firstMapping.offsetDelta -
minor nit. we generally prefer to indent as follows (if it fits):
int offsetDelta =
    _buffer.length + firstMapping.offsetDelta - _lastBufferOffset;

Maybe it's just me, but to me reading it as firstMapping.offsetDelta +
(_buffer.length - _lastBufferOffset) reads more naturally.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:36: firstMapping.token,
indentation.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:43: _buffer.add(other);
I prefer _buffer.add(other.toString()) but not a strong preference. Just shows
that you want to copy over the string now.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:62: void visitSourceLocations(void
visitor(Element element,
This is not a visitor, rather call it "forEach".
void forEachSourceLocation(void f(Element element ....)

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:73: class _SourceLocation {
ditto. why make this class private?

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:704: buffer.add(new
String.fromCharCodes([code]));
I would prefer a buffer.addCharCode (which itself just forwards the call).

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:421: setterBuffer.add('function() {\n 
return this.$fieldName;\n }');
'function(x) {\n  this.$fieldName = x;\n }';

Wasn't there a test that broke?

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/scanner/token.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/scanner/token.dart:209: void printOn(sb) {
Couldn't you make the CodeBuffer implement StringBuffer?

[podivilov, 2012-07-13 12:49:57]

Thanks!

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/code_buffer.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:6: StringBuffer _buffer;
On 2012/07/12 16:29:09, floitsch wrote:
> Any reason those fields are marked as private?
> Unless you think somebody could mistakenly use a field or a method we
generally
> keep members public.
> 
> This is, of course, not necessarily true, if the CodeBuffers are exposed
through
> the public API.

No, they aren't exposed. Made the fields public.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:22: void add(Object object) {
On 2012/07/12 16:29:09, floitsch wrote:
> Use "var" if you want to show that this function accepts more than one type,
but
> needs to dispatch.
> Also add comment /** */ explaining what it takes.

Done.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:33: int offsetDelta =
_buffer.length + firstMapping.offsetDelta -
On 2012/07/12 16:29:09, floitsch wrote:
> minor nit. we generally prefer to indent as follows (if it fits):
> int offsetDelta =
>     _buffer.length + firstMapping.offsetDelta - _lastBufferOffset;
> 
> Maybe it's just me, but to me reading it as firstMapping.offsetDelta +
> (_buffer.length - _lastBufferOffset) reads more naturally.

Done.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:36: firstMapping.token,
On 2012/07/12 16:29:09, floitsch wrote:
> indentation.

Done.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:43: _buffer.add(other);
On 2012/07/12 16:29:09, floitsch wrote:
> I prefer _buffer.add(other.toString()) but not a strong preference. Just shows
> that you want to copy over the string now.

Done.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:62: void visitSourceLocations(void
visitor(Element element,
On 2012/07/12 16:29:09, floitsch wrote:
> This is not a visitor, rather call it "forEach".
> void forEachSourceLocation(void f(Element element ....)

Done.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:73: class _SourceLocation {
On 2012/07/12 16:29:09, floitsch wrote:
> ditto. why make this class private?

Done.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/compile_time_constants.dart:704: buffer.add(new
String.fromCharCodes([code]));
On 2012/07/12 16:29:09, floitsch wrote:
> I would prefer a buffer.addCharCode (which itself just forwards the call).

Done.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/emitter.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/emitter.dart:421: setterBuffer.add('function() {\n 
return this.$fieldName;\n }');
On 2012/07/12 16:29:09, floitsch wrote:
> 'function(x) {\n  this.$fieldName = x;\n }';
> 
> Wasn't there a test that broke?

No, there wasn't. I'll try to find out what happened, and add the test if
needed.

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/scanner/token.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2001/lib/compiler/implem...
lib/compiler/implementation/scanner/token.dart:209: void printOn(sb) {
On 2012/07/12 16:29:09, floitsch wrote:
> Couldn't you make the CodeBuffer implement StringBuffer?

Done.

[floitsch, 2012-07-16 09:55:43]

https://chromiumcodereview.appspot.com/10696194/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/code_buffer.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2002/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:5: class CodeBuffer implements
StringBuffer {
Please implement the 'addAll' method, too.

[sra1, 2012-07-16 20:00:49]

https://chromiumcodereview.appspot.com/10696194/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/code_buffer.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2002/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:14: int get length() {
nit: int get length() => buffer.length;

https://chromiumcodereview.appspot.com/10696194/diff/2002/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:81: Element element;
final?

[ahe, 2012-08-02 19:31:54]

DBC

https://chromiumcodereview.appspot.com/10696194/diff/2002/lib/compiler/implem...
File lib/compiler/implementation/code_buffer.dart (right):

https://chromiumcodereview.appspot.com/10696194/diff/2002/lib/compiler/implem...
lib/compiler/implementation/code_buffer.dart:65: void setSourceLocation(Element
element, Token token) {
I mentioned in a comment in another CL that it looks like bad offsets are
sneaking through. Here is an assertion that will catch that:

assert(() {
  for (Token current = element.position; current !== null; current =
current.next) {
    if (token === current) return true;
  }
  return false;
});