Hello Patrick,

I'm about to review merge the patches from the moblin/master branch into our 
libsynthesis repo - thanks to your earlier cleanup of the history it's a really 
straightforward task, thanks for that!

The only change that I'm not sure about is:

71aef21545 (TDebugLoggerBase::DebugOpenBlock(): unused va_list)

Doesn't the "static" prefix make that argument a global scope variable, and 
thus make the routine non-reentrant and non-threadsafe?

Just because in another commit, you point out that a va_list can't be re-used, 
which IMHO implies it is changed when it is processed by vsprintf. Of course, 
in this case with the list being empty we can be reasonably safe it is not 
changed or a change would not have any effect, even if multiple threads should 
try it simultaneously. Still, is that clean enough?

Second - I don't really see why declaring it static would mean anything for 
initialisation (apart from the fact that gcc seems not to be able to figure out 
it should complain about it then) - C-ish stuff is not implicitly initialized, 
neither in global scope nor on the stack.

I tried to initialize va by using va_start() on it, however gcc rejects this in 
a function which has no ... in the declaration. We could add a ... just to make 
gcc happy. Not nice either.

So my proposal would be just to call 
DebugOpenBlock(blockname,title,collapsed,fmt,...) with fmt=NULL and no optional 
arguments instead of DebugVOpenBlock:

// open new Block without attribute list
void TDebugLoggerBase::DebugOpenBlock(cAppCharP aBlockName, cAppCharP 
aBlockTitle, bool aCollapsed)
{
  // we need a format and debug not completely off
  if (getMask() && aBlockName) {
    DebugOpenBlock(aBlockName,aBlockTitle,aCollapsed,NULL);
  }
} // TDebugLoggerBase::DebugOpenBlock

This is completely safe and we don't have to mess with the fact that apparently 
there's no proper way to initialize an empty va_list.

Lukas Zeller (l...@synthesis.ch)
- 
Synthesis AG, SyncML Solutions  & Sustainable Software Concepts
i...@synthesis.ch, http://www.synthesis.ch





_______________________________________________
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis

Reply via email to