Revised CL for customisable logging (replacing printfs).

Revised CL for customisable logging (replacing printfs).
Committed: https://code.google.com/p/dart/source/detail?r=15096

[gram, 2012-11-14 18:29:05]

Changes from before:

- the wrappers are added to a new class in log*
- only the printfs in the VM host (runtime/bin/*) are affected

[Ivan Posva, 2012-11-15 07:43:16]

I would like to know Siva's opinion as well, but this is certainly better in the
separation of VM and binary code.

-Ivan

https://codereview.chromium.org/11312242/diff/1/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/directory_win.cc#...
runtime/bin/directory_win.cc:8: #include "bin/directory.h"
Please do not change the include order. Within directory.cc files the include of
directory.h should come before all other includes.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (left):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc#oldcode5
runtime/bin/file_win.cc:5: #include "bin/file.h"
Where did the include of file.h go? And how can this compile given the many
references to the File class?

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log.h
File runtime/bin/log.h (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log.h#newcode8
runtime/bin/log.h:8: #include "bin/builtin.h"
Why do you need to include builtin.h here? There are no references to the class
Builtin inside this file.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc
File runtime/bin/log_android.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:10: #include "bin/log.h"
Should be the first include.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:12: // TODO(gram): For OS::Print and OS::PrintErr, we
should be buffering
Comment is out of date.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:26: void Log::PrintErr(const char* format, ...) {
Please try to maintain the order of the functions as they were defined in the
header.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_linux.cc
File runtime/bin/log_linux.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_linux.cc#newc...
runtime/bin/log_linux.cc:9: #include "bin/log.h"
ditto

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_linux.cc#newc...
runtime/bin/log_linux.cc:18: void Log::VFPrint(FILE* stream, const char* format,
va_list args) {
ditto

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc
File runtime/bin/log_macos.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:5: #include <crt_externs.h>
What is this needed for?

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:6: #include <signal.h>
What for?

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:8: #include <unistd.h>
As far as I can tell you should only need:
#include <stdio.h>
#include <stdarg.h>

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:10: #include "bin/log.h"
ditto

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:19: void Log::VFPrint(FILE* stream, const char* format,
va_list args) {
ditto

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_win.cc
File runtime/bin/log_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_win.cc#newcode5
runtime/bin/log_win.cc:5: #include "bin/log.h"
How is vfprintf included here?

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc#newcode628
runtime/bin/main.cc:628: char buf[1024];
This will make lots of users very unhappy. As the error messages will be cutoff.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_android.cc
File runtime/bin/platform_android.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_android....
runtime/bin/platform_android.cc:77: 
?

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_linux.cc
File runtime/bin/platform_linux.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_linux.cc...
runtime/bin/platform_linux.cc:81: 
Please revert pure white space changes.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/utils_win.cc
File runtime/bin/utils_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/utils_win.cc#newc...
runtime/bin/utils_win.cc:8: #include "bin/platform.h"
Where does the need arise for platform.h?

[siva, 2012-11-15 18:46:48]

I am fine with the introduction of the Log class to abstract
out the print functionality.

The change looks good once the comments are addressed
and the issue of truncated error messages is fixed.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/dbg_connection_an...
File runtime/bin/dbg_connection_android.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/dbg_connection_an...
runtime/bin/dbg_connection_android.cc:10: #include "bin/log.h"
Please sort alphabetically (should go after fdutils.h)

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc#newco...
runtime/bin/file_win.cc:11: #include "bin/platform.h"
Why does this need to be included here now, it was not
there in the previous revision?

https://codereview.chromium.org/11312242/diff/1/runtime/bin/gen_snapshot.cc
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/gen_snapshot.cc#n...
runtime/bin/gen_snapshot.cc:17: #include "bin/log.h"
Please add a blank line after this (would like to separate
bin/* and platform/*)

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log.h
File runtime/bin/log.h (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log.h#newcode15
runtime/bin/log.h:15: static void VFPrint(FILE* stream, const char* format,
va_list args);
Does VFPrint need to be a public function?

I did not see any calls to it from any of the files.

Also you need:
  DISALLOW_ALLOCATION();
  DISALLOW_IMPLICIT_CONSTRUCTORS(Log);
constructs here.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_win.cc
File runtime/bin/log_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_win.cc#newcode14
runtime/bin/log_win.cc:14: void Log::VFPrint(FILE* stream, const char* format,
va_list args) {
Ditto comment regarding order.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc#newcode628
runtime/bin/main.cc:628: char buf[1024];
On 2012/11/15 07:43:17, Ivan Posva wrote:
> This will make lots of users very unhappy. As the error messages will be
cutoff.


I agree, in fact we had a bug filed against us for this when I think we used to
initially cap it off at 1024 and people did not like it especially because some
of the script names are long after canonicalization.

[Ivan Posva, 2012-11-15 18:59:44]

After thinking about this a bit further, I do like the fact that printing has
been abstracted out. Making it only available for Android would be a wasted
effort.

-Ivan

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc
File runtime/bin/log_android.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:21: void Log::VFPrint(FILE* stream, const char*
format, va_list args) {
Actually thinking about it, this looks like a bad abstraction to me. Instead of
having VFPrint take a FILE* it should just indicate whether you want to print on
out or err, by for example defining a named enum.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc#newcode631
runtime/bin/main.cc:631: vsnprintf(buf, sizeof(buf), format, arguments);
Why not use Log::VFPrint here?

[gram, 2012-11-15 21:16:50]

PTAL, thanks!

https://codereview.chromium.org/11312242/diff/1/runtime/bin/dbg_connection_an...
File runtime/bin/dbg_connection_android.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/dbg_connection_an...
runtime/bin/dbg_connection_android.cc:10: #include "bin/log.h"
On 2012/11/15 18:46:48, siva wrote:
> Please sort alphabetically (should go after fdutils.h)

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/directory_win.cc
File runtime/bin/directory_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/directory_win.cc#...
runtime/bin/directory_win.cc:8: #include "bin/directory.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Please do not change the include order. Within directory.cc files the include
of
> directory.h should come before all other includes.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (left):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc#oldcode5
runtime/bin/file_win.cc:5: #include "bin/file.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Where did the include of file.h go? And how can this compile given the many
> references to the File class?

Not sure what happened here; it is restored now.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/file_win.cc#newco...
runtime/bin/file_win.cc:11: #include "bin/platform.h"
On 2012/11/15 18:46:48, siva wrote:
> Why does this need to be included here now, it was not
> there in the previous revision?

Removed

https://codereview.chromium.org/11312242/diff/1/runtime/bin/gen_snapshot.cc
File runtime/bin/gen_snapshot.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/gen_snapshot.cc#n...
runtime/bin/gen_snapshot.cc:17: #include "bin/log.h"
On 2012/11/15 18:46:48, siva wrote:
> Please add a blank line after this (would like to separate
> bin/* and platform/*)

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log.h
File runtime/bin/log.h (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log.h#newcode8
runtime/bin/log.h:8: #include "bin/builtin.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Why do you need to include builtin.h here? There are no references to the
class
> Builtin inside this file.

Removed.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log.h#newcode15
runtime/bin/log.h:15: static void VFPrint(FILE* stream, const char* format,
va_list args);
On 2012/11/15 18:46:48, siva wrote:
> Does VFPrint need to be a public function?
> 
> I did not see any calls to it from any of the files.
> 
> Also you need:
>   DISALLOW_ALLOCATION();
>   DISALLOW_IMPLICIT_CONSTRUCTORS(Log);
> constructs here.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc
File runtime/bin/log_android.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:10: #include "bin/log.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Should be the first include.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:12: // TODO(gram): For OS::Print and OS::PrintErr, we
should be buffering
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Comment is out of date.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:21: void Log::VFPrint(FILE* stream, const char*
format, va_list args) {
On 2012/11/15 18:59:44, Ivan Posva wrote:
> Actually thinking about it, this looks like a bad abstraction to me. Instead
of
> having VFPrint take a FILE* it should just indicate whether you want to print
on
> out or err, by for example defining a named enum.

I've removed VFPrint

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_android.cc#ne...
runtime/bin/log_android.cc:26: void Log::PrintErr(const char* format, ...) {
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Please try to maintain the order of the functions as they were defined in the
> header.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_linux.cc
File runtime/bin/log_linux.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_linux.cc#newc...
runtime/bin/log_linux.cc:9: #include "bin/log.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_linux.cc#newc...
runtime/bin/log_linux.cc:18: void Log::VFPrint(FILE* stream, const char* format,
va_list args) {
On 2012/11/15 07:43:17, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc
File runtime/bin/log_macos.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:5: #include <crt_externs.h>
On 2012/11/15 07:43:17, Ivan Posva wrote:
> What is this needed for?

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:6: #include <signal.h>
On 2012/11/15 07:43:17, Ivan Posva wrote:
> What for?

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:8: #include <unistd.h>
On 2012/11/15 07:43:17, Ivan Posva wrote:
> As far as I can tell you should only need:
> #include <stdio.h>
> #include <stdarg.h>

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:10: #include "bin/log.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_macos.cc#newc...
runtime/bin/log_macos.cc:19: void Log::VFPrint(FILE* stream, const char* format,
va_list args) {
On 2012/11/15 07:43:17, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_win.cc
File runtime/bin/log_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_win.cc#newcode5
runtime/bin/log_win.cc:5: #include "bin/log.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> How is vfprintf included here?

Fixed

https://codereview.chromium.org/11312242/diff/1/runtime/bin/log_win.cc#newcode14
runtime/bin/log_win.cc:14: void Log::VFPrint(FILE* stream, const char* format,
va_list args) {
On 2012/11/15 18:46:48, siva wrote:
> Ditto comment regarding order.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc#newcode628
runtime/bin/main.cc:628: char buf[1024];
On 2012/11/15 07:43:17, Ivan Posva wrote:
> This will make lots of users very unhappy. As the error messages will be
cutoff.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc#newcode628
runtime/bin/main.cc:628: char buf[1024];
On 2012/11/15 18:46:48, siva wrote:
> On 2012/11/15 07:43:17, Ivan Posva wrote:
> > This will make lots of users very unhappy. As the error messages will be
> cutoff.
> 
> 
> I agree, in fact we had a bug filed against us for this when I think we used
to
> initially cap it off at 1024 and people did not like it especially because
some
> of the script names are long after canonicalization.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/main.cc#newcode631
runtime/bin/main.cc:631: vsnprintf(buf, sizeof(buf), format, arguments);
On 2012/11/15 18:59:44, Ivan Posva wrote:
> Why not use Log::VFPrint here?

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_android.cc
File runtime/bin/platform_android.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_android....
runtime/bin/platform_android.cc:77: 
On 2012/11/15 07:43:17, Ivan Posva wrote:
> ?

Reverted

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_linux.cc
File runtime/bin/platform_linux.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/platform_linux.cc...
runtime/bin/platform_linux.cc:81: 
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Please revert pure white space changes.

Done.

https://codereview.chromium.org/11312242/diff/1/runtime/bin/utils_win.cc
File runtime/bin/utils_win.cc (right):

https://codereview.chromium.org/11312242/diff/1/runtime/bin/utils_win.cc#newc...
runtime/bin/utils_win.cc:8: #include "bin/platform.h"
On 2012/11/15 07:43:17, Ivan Posva wrote:
> Where does the need arise for platform.h?

Removed

[Ivan Posva, 2012-11-17 01:13:42]

LGTM with nitpicking comments. Thanks for the cleanups.

-Ivan

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/direct...
File runtime/bin/directory_win.cc (left):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/direct...
runtime/bin/directory_win.cc:6: 
Please leave the empty line.

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/platfo...
File runtime/bin/platform_win.cc (right):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/platfo...
runtime/bin/platform_win.cc:5: #include "bin/log.h"
Please move this under "bin/platform.h".

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/socket...
File runtime/bin/socket_android.cc (right):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/socket...
runtime/bin/socket_android.cc:13: #include "bin/socket.h"
While you are in this code, can you please move this to be the first include?
Thanks!

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/utils_...
File runtime/bin/utils_win.cc (right):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/utils_...
runtime/bin/utils_win.cc:8: #include "bin/utils.h"
ditto

[gram, 2012-11-19 17:38:56]

Done, thanks! This will be very helpful for us.

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/direct...
File runtime/bin/directory_win.cc (left):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/direct...
runtime/bin/directory_win.cc:6: 
On 2012/11/17 01:13:42, Ivan Posva wrote:
> Please leave the empty line.

Done.

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/platfo...
File runtime/bin/platform_win.cc (right):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/platfo...
runtime/bin/platform_win.cc:5: #include "bin/log.h"
On 2012/11/17 01:13:42, Ivan Posva wrote:
> Please move this under "bin/platform.h".

Done.

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/socket...
File runtime/bin/socket_android.cc (right):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/socket...
runtime/bin/socket_android.cc:13: #include "bin/socket.h"
On 2012/11/17 01:13:42, Ivan Posva wrote:
> While you are in this code, can you please move this to be the first include?
> Thanks!

Done.

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/utils_...
File runtime/bin/utils_win.cc (right):

https://chromiumcodereview.appspot.com/11312242/diff/27001/runtime/bin/utils_...
runtime/bin/utils_win.cc:8: #include "bin/utils.h"
On 2012/11/17 01:13:42, Ivan Posva wrote:
> ditto

Done.