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!