Uses SNPRINT macro where possible. Otherwise uses #define for format.

Uses SNPRINT macro where possible. Otherwise uses #define for format.

I was able to convert 49 SNPrint(NULL, 0, ...) patterns to use the
macro. I had to use a #define for the format string 5 times due to
uses that didn't fit the macro.

2 occurences of SNPrint(NULL, 0, ...) had >=2 possibilities for the
format string based on runtime values, and I didn't try to convert
them.

Fixed ~10 format strings.

BUG=
R=iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/16393ee9c6cfcda036ce32f4fb1c029f3cadc906

[zra, 2015-09-08 21:09:24]

zra@google.com changed reviewers:
+ iposva@google.com

[zra, 2015-09-08 21:09:24]

https://codereview.chromium.org/1331623002/diff/1/runtime/vm/assembler.cc
File runtime/vm/assembler.cc (left):

https://codereview.chromium.org/1331623002/diff/1/runtime/vm/assembler.cc#old...
runtime/vm/assembler.cc:184: // Shared macros are implemented here.
These weren't used, and have a memory leak.

[Ivan Posva, 2015-09-09 16:30:36]

LGTMwC and the Unimplemented (and friends) added back as discussed. We should
deal with the "existing" leak separately.

-Ivan

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10665: "Namespace for library '%s'", lib.ToCString());
I like this formatting better than having the format string trailing on the
first line. What do you think?

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:15934: }
How about:

char* chars = NULL;
if (...) {
  // First part.
} else if (...) {
  // Second part.
} else {
  // Third part.
}
return chars;

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:16125: return chars;
ditto.

[siva, 2015-09-09 16:46:21]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2015-09-09 16:46:21]

DBC

Instead of having a macro SNPRINT why not change the signature of OS::SNPrint to

SNPrint(Zone* zone, char* str, const char* format, ...);

it is implemented to figure out the length and alloc the buffer, print and free
the buffer.
if (zone == NULL) then malloc is used and a free is done otherwise the
allocation is done in the passed in zone.

[siva, 2015-09-09 16:50:16]

Sorry I meant

OS::SNPrint(Zone* zone, const char* format, ...);

[zra, 2015-09-09 18:40:13]

Added back assembler functions and added a call OS::SNCreate to do what Siva
suggested.

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10665: "Namespace for library '%s'", lib.ToCString());
On 2015/09/09 16:30:36, Ivan Posva wrote:
> I like this formatting better than having the format string trailing on the
> first line. What do you think?

Done.

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:15934: }
On 2015/09/09 16:30:36, Ivan Posva wrote:
> How about:
> 
> char* chars = NULL;
> if (...) {
>   // First part.
> } else if (...) {
>   // Second part.
> } else {
>   // Third part.
> }
> return chars;

Done.

https://codereview.chromium.org/1331623002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:16125: return chars;
On 2015/09/09 16:30:36, Ivan Posva wrote:
> ditto.

Done.

[Ivan Posva, 2015-09-10 17:59:07]

LGTM

I do like the fact that we were able to get rid of the MACRO.

-Ivan

[Cutch, 2015-09-10 18:01:04]

johnmccutchan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2015-09-10 18:01:04]

DBC-

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.cc
File runtime/vm/os_android.cc (right):

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.c...
runtime/vm/os_android.cc:354: buffer = zone->Alloc<char>(len + 1);
Note that Zone already has the following utility functions:

char* PrintToString(const char* format, ...) PRINTF_ATTRIBUTE(2, 3);
char* VPrint(const char* format, va_list args);

[Ivan Posva, 2015-09-10 18:08:11]

-Ivan

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.cc
File runtime/vm/os_android.cc (right):

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.c...
runtime/vm/os_android.cc:342: char* OS::SNCreate(Zone* zone, const char* format,
...) {
P.S. Please drop the N as we are not passing a length here. I missed that in the
original review, sorry.

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.c...
runtime/vm/os_android.cc:354: buffer = zone->Alloc<char>(len + 1);
On 2015/09/10 18:01:04, Cutch wrote:
> Note that Zone already has the following utility functions:
> 
> char* PrintToString(const char* format, ...) PRINTF_ATTRIBUTE(2, 3);
> char* VPrint(const char* format, va_list args);

Thanks for pointing that out. I find that API a bit confusing since it does not
print the Zone, but allocates from the Zone. We should probably consolidate
these uses with this API here.

[zra, 2015-09-10 21:58:38]

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.cc
File runtime/vm/os_android.cc (right):

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.c...
runtime/vm/os_android.cc:342: char* OS::SNCreate(Zone* zone, const char* format,
...) {
On 2015/09/10 18:08:11, Ivan Posva wrote:
> P.S. Please drop the N as we are not passing a length here. I missed that in
the
> original review, sorry.

Done.

https://codereview.chromium.org/1331623002/diff/60001/runtime/vm/os_android.c...
runtime/vm/os_android.cc:354: buffer = zone->Alloc<char>(len + 1);
On 2015/09/10 18:08:11, Ivan Posva wrote:
> On 2015/09/10 18:01:04, Cutch wrote:
> > Note that Zone already has the following utility functions:
> > 
> > char* PrintToString(const char* format, ...) PRINTF_ATTRIBUTE(2, 3);
> > char* VPrint(const char* format, va_list args);
> 
> Thanks for pointing that out. I find that API a bit confusing since it does
not
> print the Zone, but allocates from the Zone. We should probably consolidate
> these uses with this API here.

Changed as discussed this morning.

[Ivan Posva, 2015-09-10 22:49:03]

Still LGTM.

-Ivan

[zra, 2015-09-11 07:18:20]

Committed patchset #7 (id:120001) manually as
16393ee9c6cfcda036ce32f4fb1c029f3cadc906 (presubmit successful).