Re: [boost] [Review] The review of the Boost.Logging library starts today, November 7th

From: John Torjo (john.lists_at_[hidden])
Date: 2005-11-14 08:51:27

Hi Andrew,

> 
> Some main thoughts:
> 1) It might be helpful if loggers and managers were "promoted" to 
> objects with an obvious interface that can be easily passed around.

Managers will be promoted to objects.
Perhaps loggers can be promoted too.

> 
> 1.1) I haven't had a problem passing loggers around by use using a 
> logger& or a logger*, but the interface seems to assume that generally 
> the code will be compiled where the log name is known. Most of the code 
> I work on is header or dll code that needs to be passed a logger by the 
> main system.

Not sure I follow...

> 1.2.1) I think the static manager makes the design harder to use and 
> control in a system that isn't a single executeable. This could be 
> solved by either forcing the user to declare the manager, or possibly by 
> leaving it static, but making all the functions take an explicit manager 
> ref or pointer, or making the functions members of the manager.

If you take a look at log_manager.hpp, you'll see that this is how it 
happens now. Or have I misunderstood something?

> 
> 1.2.1.1) The static manager seems particularly problematic for dlls.
> 
> 1.2.1.1.1) Currently, the dll can't call the api functions and act on 
> the application's logs, since the calls will be compiled to act on the 
> dll's static log manager (I think). Although "externally visible" is a 
> design premise of the library, I don't think the dll can even enable or 
> disable application logs it knows the name for because the call will go 
> to its own logs of the same name. Similarly the application can't 
> control the dlls logs for the same reason... I think!... maybe I missed 
> how to do this. If that's the case maybe the doc just needs to be more 
> extensive.
> 

The docs need to be more extensive on this issue ;)

> 1.2.1.1.2) It looks like there could actually be multi-threaded issues 
> writing to logs from a dll, because, if the log thread is used, both the 
> dll and the app will start their own trhead to write to the log. 
> Presumably the log is ultimately a single object, like the screen or a 
> file. I suppose if the locking uses a "named" lock that the OS api can 
> look up this could be solved. However, it looks like on windows at least 
> a Critical Section is used, which I don't think is named, so I don't 
> think the one or more dlls running the thread and the application would 
> actually lock each other. It seems like they could step on each other.

If you specify the appenders/modifiers only in one place (like, in the 
Executable), there are no problems.

> 
> 1.3) I can't seem to pass around log levels. I think this is because 
> they are const ints, not members of an enum. This means the following 
> code won't compile:
> 
> void SomeClass::Dump(boost::logging::level_type eLevel,
> 		     boost::logging::logger& rLog)
> {
> 	BOOST_SCOPEDLOGL(rLog, eLevel)
> 			<< "Some Msg" << m_iSomeMemberVariable << endl;
> }

At this time, indeed you can't.
I'll find a solution.

In particular, log levels can't be members of an enum, since you should 
be able to add your own at any time.

> 
> 2) BOOST_LOG and BOOST_SCOPEDLOG are way too many uppercase characters 
> to type. :)

Well, you're right :) But so far can't think of any names that could be 
shorter...

> -----------------------------------------------------------------
>  >> - What is your evaluation of the implementation?
> 
> Looks fine. No usage headaches. Find a few minor bugs, to be expected. 
> No major ones.
> 
> -----------------------------------------------------------------
>  >> - What is your evaluation of the documentation?
> 
> Good start, but as usual, could use more - John we know you need more 
> work :). In particular, I think some description of the underlying 
> implementation would be helpful. I'd like to see a description of the 
> major objects and their data structures, and their relation to each 
> other. I'd like to see a description of how "add_appender" works. It 
> took me a while to struggle through this in the code.
> 
> In particular, I'd like to see a description of how exactly the 
> BOOST_LOG macro works! This is the most important part of the libary, 
> and there are couple indirections an inversion in the implementation, 
> (which seems fine), but takes a bit to understand.
> 
> John, I can volunteer some hours in next couple weeks to help review or 
> add doc if you like. If that would be helpful, I just need some 
> direction files to review or subject matter. You can e-mail me at "a dot 
> schweitzer dot grps at gmail dot com"

I'll definitely call you on that!

Andrew, was this a formal review? I'm asking, since I did not see the 
"Yes" (accept) or "No" (reject) at the end of it.

Best,
John

-- 
John Torjo,    Contributing editor, C/C++ Users Journal
-- "Win32 GUI Generics" -- generics & GUI do mix, after all
-- http://www.torjo.com/win32gui/surfaces.html - Sky's the limit!
-- http://www.torjo.com/cb/ - Click, Build, Run!