Per isolate Log with block support

Per isolate Log with block support

BUG=
R=srdjan@google.com

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

[Cutch, 2015-02-09 16:58:31]

johnmccutchan@google.com changed reviewers:
+ asiva@google.com, iposva@google.com, koda@google.com, srdjan@google.com

[koda, 2015-02-09 17:57:29]

First set of comments.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.cc#newcode50
runtime/vm/isolate.cc:50: DEFINE_FLAG(charp, isolate_log_filter, NULL, "Log only
named isolate.");
This is actually a substring filter, so say that. Also note default omission of
service isolate.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.h#newcode171
runtime/vm/isolate.h:171: class Log* Log() const;
Why not "log" (and avoid the "class")? The collision with the class name is a
bit confusing.

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

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode64
runtime/vm/log.h:64: LogBlock(Isolate* isolate, Log* log)
Please add a LogBlock(Thread*) constructor (you might need to define it in the
.cc).

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode67
runtime/vm/log.h:67: log_->EnableManualFlush();
Shouldn't 'log' usually be the same as isolate->Log()?

https://codereview.chromium.org/913503004/diff/1/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/913503004/diff/1/runtime/vm/thread.h#newcode34
runtime/vm/thread.h:34: // The log for with this thread.
"for with" -> "for"

[srdjan, 2015-02-09 18:01:03]

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.h#newcode699
runtime/vm/isolate.h:699: bool service_isolate_;
I think 'is_service_isolate_' is more descriptive.

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

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode25
runtime/vm/log.h:25: ~Log();
virtual ~Log();

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode31
runtime/vm/log.h:31: void Append(const char* str);
Why both Print and Append? Append => Print("%s", str) ?

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode35
runtime/vm/log.h:35: void Flush(const intptr_t cursor = 0);
Do we currently need the cursor functionality?

[Cutch, 2015-02-09 18:19:39]

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.cc#newcode50
runtime/vm/isolate.cc:50: DEFINE_FLAG(charp, isolate_log_filter, NULL, "Log only
named isolate.");
On 2015/02/09 17:57:29, koda wrote:
> This is actually a substring filter, so say that. Also note default omission
of
> service isolate.

Done.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.h#newcode171
runtime/vm/isolate.h:171: class Log* Log() const;
On 2015/02/09 17:57:29, koda wrote:
> Why not "log" (and avoid the "class")? The collision with the class name is a
> bit confusing.

Per our style guide- lower case log() must be a straight return of the field.
Alternatively I could use GetLog.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/isolate.h#newcode699
runtime/vm/isolate.h:699: bool service_isolate_;
On 2015/02/09 18:01:02, srdjan wrote:
> I think 'is_service_isolate_' is more descriptive.

Done.

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

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode25
runtime/vm/log.h:25: ~Log();
On 2015/02/09 18:01:02, srdjan wrote:
> virtual ~Log();

Why? There are no virtual methods.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode31
runtime/vm/log.h:31: void Append(const char* str);
On 2015/02/09 18:01:02, srdjan wrote:
> Why both Print and Append? Append => Print("%s", str) ?

Removed Append.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode35
runtime/vm/log.h:35: void Flush(const intptr_t cursor = 0);
On 2015/02/09 18:01:02, srdjan wrote:
> Do we currently need the cursor functionality?

Yes for LogBlocks.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode64
runtime/vm/log.h:64: LogBlock(Isolate* isolate, Log* log)
On 2015/02/09 17:57:29, koda wrote:
> Please add a LogBlock(Thread*) constructor (you might need to define it in the
> .cc).

Done.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/log.h#newcode67
runtime/vm/log.h:67: log_->EnableManualFlush();
On 2015/02/09 17:57:29, koda wrote:
> Shouldn't 'log' usually be the same as isolate->Log()?

Yes, but, for unit testing it's awkward to need to fetch the isolate's log. I've
added convenience constructors.

https://codereview.chromium.org/913503004/diff/1/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/913503004/diff/1/runtime/vm/thread.h#newcode34
runtime/vm/thread.h:34: // The log for with this thread.
On 2015/02/09 17:57:29, koda wrote:
> "for with" -> "for"

Done.

[srdjan, 2015-02-09 18:34:17]

lgtm

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h
File runtime/vm/log.h (right):

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h#newcode19
runtime/vm/log.h:19: Isolate::Current()->Log()->Print(format, ##__VA_ARGS__)
When reading ISO_Print, I think of a standardized print (ISO = International
Standard Organization, e.g. ISO9001)

Maybe ISL_Print is a better name?

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h#newcode59
runtime/vm/log.h:59: class LogBlock : public StackResource {
Please add a brief comment.

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h#newcode86
runtime/vm/log.h:86: intptr_t cursor_;
const ?

[Cutch, 2015-02-09 18:51:11]

New patchsets have been uploaded after l-g-t-m from srdjan@google.com

[Cutch, 2015-02-09 18:51:20]

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h
File runtime/vm/log.h (right):

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h#newcode19
runtime/vm/log.h:19: Isolate::Current()->Log()->Print(format, ##__VA_ARGS__)
On 2015/02/09 18:34:17, srdjan wrote:
> When reading ISO_Print, I think of a standardized print (ISO = International
> Standard Organization, e.g. ISO9001)
> 
> Maybe ISL_Print is a better name?

Done.

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h#newcode59
runtime/vm/log.h:59: class LogBlock : public StackResource {
On 2015/02/09 18:34:17, srdjan wrote:
> Please add a brief comment.

Done.

https://codereview.chromium.org/913503004/diff/20001/runtime/vm/log.h#newcode86
runtime/vm/log.h:86: intptr_t cursor_;
On 2015/02/09 18:34:17, srdjan wrote:
> const ?

Done.

[Cutch, 2015-02-09 18:54:41]

Committed patchset #3 (id:40001) manually as 43604 (presubmit successful).