Re: [MERGE] SourceLayout: logs take 0

On 03/19/2009 06:48 PM, Amos Jeffries wrote:
> Shuffle the logs output code into liblogs.la
>
> Shuffles the log output files into src/logs.

Are there any build targets that use COMMON_LIBS but do not use
liblogs.la? If not, you can move loblogs into COMMON_LIBS.

> Also C++ converts logfile* and customlog* functions into methods of
> classes LogFile and CustomLog.

> LogFile objects have a 1:1 relationship with disk files

Except for syslog-based logging?

Do you think it would be better to handle syslog in a child class of
LogFile or Log? There seems to be a lot of "exceptions" and testing for
syslog in the current general LogFile code...

> and control all
> actions associated with that file. Log files are created by passing the
> details to a LogFile constructor and when constructor has completed the
> LogFile object will be immediately writable as output logs. Files are
> closed by deleting the LogFile handler object.

The LogFile class is missing a brief description of its primary purpose,
I think. All newly added classes should have that right above the class
declaration.

The LogFile class is missing a copy ctor and an assignment operator. If
you do not need them, declare but do not define them.

Please add preliminary member initialization to LogFile constructor.
There may be no bugs now, but it is difficult to say whether everything
is initialized in that long ctor with many ifs and #ifdefs...

Non-static LogFile method names should start with a lower-case letter,
right? Any reason to keep them out-of-style with other new classes?

> CustomLog objects are generated from configuration info and describe the
> log file and its display format.

The CustomLog class is missing a brief description of its primary
purpose, I think. All newly added classes should have that. It would be
nice if that description mentions the relationship between CustomLog and
the default log. That part confuses developers like me. If CustomLog
handles all types of logs, including the default one, then why is it
called custom?

CustomLog class is missing a copy ctor and an assignment operator. If
you do not need them, declare but do not define them.

Do we need class LogConfig? One would expect to find log configuration
there, but the class does not have any data members, holds just one
[static] method. Yet, we have a global object of LogConfig type.
Something is odd here. This should be a namespace or just a global
function, I guess.

> NP: Simple open+write testing has been done to verify cache.log output.
> However bugs in HEAD prevent proper testing at this time.

Does "make distcheck" still work if translations are disabled?

Thanks,

Alex.
Received on Fri Mar 20 2009 - 21:20:44 MDT