Initial check-in of Diagnostics policy framework.

[Jeffrey Yasskin, 2009-03-17 03:37:35]

You should probably create a mailing list for this project, and announce it on
plumage. I think you'll get several subscribers from there.

http://codereview.appspot.com/27059/diff/1/2
File Makefile.am (right):

http://codereview.appspot.com/27059/diff/1/2#newcode31
Line 31: include/scarcity/diagnostics/Channel.h\
Re source layout: I'm guessing public headers live in include/scarcity to make
installing them easy. You should also dedicate a space up front for private
headers so that it's easy to share private functions between cpp files. Adjacent
to the .cpp files would be fine with me. This is a mistake Python made.

http://codereview.appspot.com/27059/diff/1/4
File include/scarcity/diagnostics/Channel.h (right):

http://codereview.appspot.com/27059/diff/1/4#newcode8
Line 8: #ifndef SCARCITY_DIAGNOSTICS_FILTER_H
Skip this. gcc's preprocessor already has this optimization.

http://codereview.appspot.com/27059/diff/1/4#newcode19
Line 19: ChannelInfo(const char * name) : name_(name) {}
Document that name is presumed to live forever. Presumably, that makes it a
"literal string".

Nitpicky: bleh on putting spaces on both sides of the *. I always put the space
on the right, but I won't insist on my religion.

http://codereview.appspot.com/27059/diff/1/4#newcode31
Line 31: template<class Filter>
Document the concept Filter needs to fulfil.

http://codereview.appspot.com/27059/diff/1/4#newcode32
Line 32: class Channel {
This class appears to exist just to curry two arguments into Filter?

http://codereview.appspot.com/27059/diff/1/4#newcode34
Line 34: Channel(Writer & writer, const char * name = "")
Document the lifetimes.

http://codereview.appspot.com/27059/diff/1/8
File include/scarcity/diagnostics/Writer.h (right):

http://codereview.appspot.com/27059/diff/1/8#newcode11
Line 11: #include <sys/types.h>
I think you want stddef.h here. I don't see any uses of types from sys/types.h
except for size_t.

http://codereview.appspot.com/27059/diff/1/8#newcode39
Line 39: /** Writer that prints to stdout. */
stdout or stderr?

http://codereview.appspot.com/27059/diff/1/8#newcode48
Line 48: void write(const char * msg, size_t length) {}
It'll be interesting to see if gcc can inline that. The call through a virtual
pointer, even if it can see all the types involved, is likely to cause problems.
A way to check is to compile a file with "-dA -S -o the_asm.s" and then look for
something that looks like a virtual call.

http://codereview.appspot.com/27059/diff/1/9
File lib/diagnostics/Filter.cpp (right):

http://codereview.appspot.com/27059/diff/1/9#newcode34
Line 34: writer_.write("\n", 1);
You may need to flush the writer too.

http://codereview.appspot.com/27059/diff/1/10
File lib/diagnostics/FullDiagnostics.cpp (right):

http://codereview.appspot.com/27059/diff/1/10#newcode11
Line 11: Channel<AbortFilter> FullDiagnostics::assertFailed(writer, "assertion
failed");
You may need to make these aggregates. Otherwise, their constructors run in an
undefined order relative to constructors in other files. Those constructors may
allocate memory, and you probably need these loggers alive before they're
needed.

http://codereview.appspot.com/27059/diff/1/11
File lib/diagnostics/NoDiagnostics.cpp (right):

http://codereview.appspot.com/27059/diff/1/11#newcode12
Line 12: Channel<DisabledFilter> NoDiagnostics::assertFailed(writer, "assertion
failed");
I think I might argue for a templated DiagnosticsT type with functions for each
of these log types, rather than having a separate variable for each one. That'd
let you avoid duplicating the field names and would, I think, make it easier to
ensure that you don't need dynamic initialization.

http://codereview.appspot.com/27059/diff/1/12
File lib/diagnostics/StackTrace.cpp (right):

http://codereview.appspot.com/27059/diff/1/12#newcode37
Line 37: char * buffer = (char *)malloc(sz);
Do you really want to use malloc inside a memory allocator's crash handler?

http://codereview.appspot.com/27059/diff/1/13
File lib/diagnostics/Writer.cpp (right):

http://codereview.appspot.com/27059/diff/1/13#newcode27
Line 27: size_t len = sprintf(buffer, "%d", value);
snprintf! (yes, even though you know the size)

http://codereview.appspot.com/27059/diff/1/14
File test/unit/DiagnosticsTest.cpp (right):

http://codereview.appspot.com/27059/diff/1/14#newcode35
Line 35: TD::fatal() << "message";
This isn't really a test of most of the diagnostics library. You'd need death
tests and/or captured output tests to really test things here.

[Talin, 2009-03-17 17:36:48]

Thanks for the review! I have a few questions on some of the items, see below.

http://codereview.appspot.com/27059/diff/1/2
File Makefile.am (right):

http://codereview.appspot.com/27059/diff/1/2#newcode31
Line 31: include/scarcity/diagnostics/Channel.h\
On 2009/03/17 03:37:35, jyasskin wrote:
> Re source layout: I'm guessing public headers live in include/scarcity to make
> installing them easy. You should also dedicate a space up front for private
> headers so that it's easy to share private functions between cpp files.
Adjacent
> to the .cpp files would be fine with me. This is a mistake Python made.

Hmmm. Since most of the library is going to be templates, I'm not sure that any
header files can be truly "private". But we'll see.

I did take your suggestion of using "-inl.h" for the implementation of the
templates.

http://codereview.appspot.com/27059/diff/1/4
File include/scarcity/diagnostics/Channel.h (right):

http://codereview.appspot.com/27059/diff/1/4#newcode8
Line 8: #ifndef SCARCITY_DIAGNOSTICS_FILTER_H
On 2009/03/17 03:37:35, jyasskin wrote:
> Skip this. gcc's preprocessor already has this optimization.

OK. I see that LLVM also doesn't bother with this. BTW I acquired this habit
from many years of working with Visual Studio. (Although I don't know what the
current version does.)

http://codereview.appspot.com/27059/diff/1/4#newcode19
Line 19: ChannelInfo(const char * name) : name_(name) {}
On 2009/03/17 03:37:35, jyasskin wrote:
> Document that name is presumed to live forever. Presumably, that makes it a
> "literal string".

OK

> Nitpicky: bleh on putting spaces on both sides of the *. I always put the
space
> on the right, but I won't insist on my religion.

I see that LLVM puts it on the left.

The argument about the positioning of spaces around the * goes back a long way,
and essentially boils down to an argument about how C++ "ought to be" vs. how it
actually is.  I got into the habit of putting spaces on both sides as a
compromise between the two camps.

http://codereview.appspot.com/27059/diff/1/4#newcode31
Line 31: template<class Filter>
On 2009/03/17 03:37:35, jyasskin wrote:
> Document the concept Filter needs to fulfil.

Done.

http://codereview.appspot.com/27059/diff/1/4#newcode32
Line 32: class Channel {
On 2009/03/17 03:37:35, jyasskin wrote:
> This class appears to exist just to curry two arguments into Filter?

Yeah.

http://codereview.appspot.com/27059/diff/1/4#newcode34
Line 34: Channel(Writer & writer, const char * name = "")
On 2009/03/17 03:37:35, jyasskin wrote:
> Document the lifetimes.

OK.

http://codereview.appspot.com/27059/diff/1/8
File include/scarcity/diagnostics/Writer.h (right):

http://codereview.appspot.com/27059/diff/1/8#newcode11
Line 11: #include <sys/types.h>
On 2009/03/17 03:37:35, jyasskin wrote:
> I think you want stddef.h here. I don't see any uses of types from sys/types.h
> except for size_t.

uintptr_t

http://codereview.appspot.com/27059/diff/1/8#newcode39
Line 39: /** Writer that prints to stdout. */
On 2009/03/17 03:37:35, jyasskin wrote:
> stdout or stderr?

Done, thanks.

http://codereview.appspot.com/27059/diff/1/8#newcode48
Line 48: void write(const char * msg, size_t length) {}
On 2009/03/17 03:37:35, jyasskin wrote:
> It'll be interesting to see if gcc can inline that. The call through a virtual
> pointer, even if it can see all the types involved, is likely to cause
problems.
> A way to check is to compile a file with "-dA -S -o the_asm.s" and then look
for
> something that looks like a virtual call.

Actually, this isn't where the magic switch occurs. That occurs in
DisabledFilter, which overloads the stream operator >> to do nothing.

The only reason for this class to exist is that the NoDiagnostics policy class
uses only DisabledFilter classes, which never write to a Writer. It seemed
misleading to have a StdErrWriter which never got written to.

I'm open to suggestions as to how to make this clearer.

http://codereview.appspot.com/27059/diff/1/9
File lib/diagnostics/Filter.cpp (right):

http://codereview.appspot.com/27059/diff/1/9#newcode34
Line 34: writer_.write("\n", 1);
On 2009/03/17 03:37:35, jyasskin wrote:
> You may need to flush the writer too.

Done.

http://codereview.appspot.com/27059/diff/1/10
File lib/diagnostics/FullDiagnostics.cpp (right):

http://codereview.appspot.com/27059/diff/1/10#newcode11
Line 11: Channel<AbortFilter> FullDiagnostics::assertFailed(writer, "assertion
failed");
On 2009/03/17 03:37:35, jyasskin wrote:
> You may need to make these aggregates. Otherwise, their constructors run in an
> undefined order relative to constructors in other files. Those constructors
may
> allocate memory, and you probably need these loggers alive before they're
> needed.

I'm not sure what you mean by making them aggregates. Do you mean using
initializer-lists instead of constructors?

http://codereview.appspot.com/27059/diff/1/11
File lib/diagnostics/NoDiagnostics.cpp (right):

http://codereview.appspot.com/27059/diff/1/11#newcode12
Line 12: Channel<DisabledFilter> NoDiagnostics::assertFailed(writer, "assertion
failed");
On 2009/03/17 03:37:35, jyasskin wrote:
> I think I might argue for a templated DiagnosticsT type with functions for
each
> of these log types, rather than having a separate variable for each one.
That'd
> let you avoid duplicating the field names and would, I think, make it easier
to
> ensure that you don't need dynamic initialization.

Do you mean something like this?

template<class Filter>  
class DiagnosticsT {
  static Filter & fatal();
  static Filter & fatal(const char * file, size_t line);
  static Filter & fatal(const Location & loc);

  static Filter & error();
  static Filter & error(const char * file, size_t line);
  static Filter & error(const Location & loc);

  static Filter & warn();
  static Filter & warn(const char * file, size_t line);
  static Filter & warn(const Location & loc);

  static Filter & info();
  static Filter & info(const char * file, size_t line);
  static Filter & info(const Location & loc);

  static Filter & debug();
  static Filter & debug(const char * file, size_t line);
  static Filter & debug(const Location & loc);
};

How would you specify different filter types for the different severities?

http://codereview.appspot.com/27059/diff/1/12
File lib/diagnostics/StackTrace.cpp (right):

http://codereview.appspot.com/27059/diff/1/12#newcode37
Line 37: char * buffer = (char *)malloc(sz);
On 2009/03/17 03:37:35, jyasskin wrote:
> Do you really want to use malloc inside a memory allocator's crash handler?

Unfortunately, I am not sure I can avoid it - the specification of
__cxa_demangle is that it can do a realloc() of the buffer that you pass to it,
returning the new buffer pointer.

I suppose you could pre-reserve a buffer of sufficiently large size and hope
that no C++ symbol in your program is large enough to overflow the buffer. We
would need to remember to call the pre-reserve function before we start.

Another issue is whether we intend to redefine 'malloc'. I was assuming that we
wouldn't - my intent is to build something that can co-exist with normal malloc,
so that garbage-collectable objects are allocated on my heap, whereas extension
libraries can continue to use normal malloc.

http://codereview.appspot.com/27059/diff/1/13
File lib/diagnostics/Writer.cpp (right):

http://codereview.appspot.com/27059/diff/1/13#newcode27
Line 27: size_t len = sprintf(buffer, "%d", value);
On 2009/03/17 03:37:35, jyasskin wrote:
> snprintf! (yes, even though you know the size)

Oh, all right. :)

http://codereview.appspot.com/27059/diff/1/14
File test/unit/DiagnosticsTest.cpp (right):

http://codereview.appspot.com/27059/diff/1/14#newcode35
Line 35: TD::fatal() << "message";
On 2009/03/17 03:37:35, jyasskin wrote:
> This isn't really a test of most of the diagnostics library. You'd need death
> tests and/or captured output tests to really test things here.

True, however I was having trouble getting the death tests to work.

[Jeffrey Yasskin, 2009-03-18 15:07:18]

LGTM

http://codereview.appspot.com/27059/diff/1/8
File include/scarcity/diagnostics/Writer.h (right):

http://codereview.appspot.com/27059/diff/1/8#newcode11
Line 11: #include <sys/types.h>
On 2009/03/17 17:36:48, Talin wrote:
> On 2009/03/17 03:37:35, jyasskin wrote:
> > I think you want stddef.h here. I don't see any uses of types from
sys/types.h
> > except for size_t.
> 
> uintptr_t

Oh, but that's properly from <stdint.h>

http://codereview.appspot.com/27059/diff/1/8#newcode48
Line 48: void write(const char * msg, size_t length) {}
On 2009/03/17 17:36:48, Talin wrote:
> On 2009/03/17 03:37:35, jyasskin wrote:
> > It'll be interesting to see if gcc can inline that. The call through a
virtual
> > pointer, even if it can see all the types involved, is likely to cause
> problems.
> > A way to check is to compile a file with "-dA -S -o the_asm.s" and then look
> for
> > something that looks like a virtual call.
> 
> Actually, this isn't where the magic switch occurs. That occurs in
> DisabledFilter, which overloads the stream operator >> to do nothing.

Oh, right. Never mind then.

> The only reason for this class to exist is that the NoDiagnostics policy class
> uses only DisabledFilter classes, which never write to a Writer. It seemed
> misleading to have a StdErrWriter which never got written to.
> 
> I'm open to suggestions as to how to make this clearer.

I think it's probably fine.

http://codereview.appspot.com/27059/diff/1/10
File lib/diagnostics/FullDiagnostics.cpp (right):

http://codereview.appspot.com/27059/diff/1/10#newcode11
Line 11: Channel<AbortFilter> FullDiagnostics::assertFailed(writer, "assertion
failed");
On 2009/03/17 17:36:48, Talin wrote:
> On 2009/03/17 03:37:35, jyasskin wrote:
> > You may need to make these aggregates. Otherwise, their constructors run in
an
> > undefined order relative to constructors in other files. Those constructors
> may
> > allocate memory, and you probably need these loggers alive before they're
> > needed.
> 
> I'm not sure what you mean by making them aggregates. Do you mean using
> initializer-lists instead of constructors?

Yeah, although they're not really initializer-lists yet. The current standard
says,

[dcl.init.aggr] 
8.5.1 Aggregates 
1 An aggregate is an array or a class (clause 9) with no user-declared
constructors (12.1), no private or pro- 
tected non-static data members (clause 11), no base classes (clause 10), and no
virtual functions (10.3). 
2 When an aggregate is initialized the initializer can be an initializer-clause
consisting of a brace-enclosed, 
comma-separated list of initializers for the members of the aggregate, written
in increasing subscript or 
member order.
...
14 When an aggregate with static storage duration is initialized with a
brace-enclosed initializer-list, if all the member initializer expressions are
constant expressions, and the aggregate is a POD type, the initialization 
shall be done during the static phase of initialization (3.6.2); otherwise, it
is unspecified whether the initialization of members with constant expressions
takes place during the static phase or during the dynamic 
phase of initialization.

http://codereview.appspot.com/27059/diff/1/11
File lib/diagnostics/NoDiagnostics.cpp (right):

http://codereview.appspot.com/27059/diff/1/11#newcode12
Line 12: Channel<DisabledFilter> NoDiagnostics::assertFailed(writer, "assertion
failed");
On 2009/03/17 17:36:48, Talin wrote:
> On 2009/03/17 03:37:35, jyasskin wrote:
> > I think I might argue for a templated DiagnosticsT type with functions for
> each
> > of these log types, rather than having a separate variable for each one.
> That'd
> > let you avoid duplicating the field names and would, I think, make it easier
> to
> > ensure that you don't need dynamic initialization.
> 
> Do you mean something like this?
> 
> template<class Filter>  
> class DiagnosticsT {
>   static Filter & fatal();
>   static Filter & fatal(const char * file, size_t line);
>   static Filter & fatal(const Location & loc);

Perhaps
  static Filter & fatal(Location loc = Location());
  ...

and then have the macro expand to:
  Diag::fatal(Location(__FILE__, __LINE__))

>   static Filter & error();
>   static Filter & error(const char * file, size_t line);
>   static Filter & error(const Location & loc);
> 
>   static Filter & warn();
>   static Filter & warn(const char * file, size_t line);
>   static Filter & warn(const Location & loc);
> 
>   static Filter & info();
>   static Filter & info(const char * file, size_t line);
>   static Filter & info(const Location & loc);
> 
>   static Filter & debug();
>   static Filter & debug(const char * file, size_t line);
>   static Filter & debug(const Location & loc);
> };
> 
> How would you specify different filter types for the different severities?

2 template arguments? You only ever have a different filter for the fatal type
anyway, right?

http://codereview.appspot.com/27059/diff/1/12
File lib/diagnostics/StackTrace.cpp (right):

http://codereview.appspot.com/27059/diff/1/12#newcode37
Line 37: char * buffer = (char *)malloc(sz);
On 2009/03/17 17:36:48, Talin wrote:
> On 2009/03/17 03:37:35, jyasskin wrote:
> > Do you really want to use malloc inside a memory allocator's crash handler?
> 
> Unfortunately, I am not sure I can avoid it - the specification of
> __cxa_demangle is that it can do a realloc() of the buffer that you pass to
it,
> returning the new buffer pointer.

Bleh. __cxa_demangle--

> I suppose you could pre-reserve a buffer of sufficiently large size and hope
> that no C++ symbol in your program is large enough to overflow the buffer. We
> would need to remember to call the pre-reserve function before we start.

That's probably worse. You could use mmap directly to allocate some memory, but
I wouldn't want to risk crashing in the trace collector if a symbol were
unusually long. And C++ does generate really long symbol names.

> Another issue is whether we intend to redefine 'malloc'. I was assuming that
we
> wouldn't - my intent is to build something that can co-exist with normal
malloc,
> so that garbage-collectable objects are allocated on my heap, whereas
extension
> libraries can continue to use normal malloc.

Oh, ok.

http://codereview.appspot.com/27059/diff/1/14
File test/unit/DiagnosticsTest.cpp (right):

http://codereview.appspot.com/27059/diff/1/14#newcode35
Line 35: TD::fatal() << "message";
On 2009/03/17 17:36:48, Talin wrote:
> On 2009/03/17 03:37:35, jyasskin wrote:
> > This isn't really a test of most of the diagnostics library. You'd need
death
> > tests and/or captured output tests to really test things here.
> 
> True, however I was having trouble getting the death tests to work.

Yeah, they're trickier.

http://codereview.appspot.com/27059/diff/1005/1017
File lib/diagnostics/Writer.cpp (right):

http://codereview.appspot.com/27059/diff/1005/1017#newcode40
Line 40: fwrite(text, sizeof(char), len, stdout);
stdout or stderr?