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.