[crossfire] code cleanup commit.

[Mark Wedel]

Nicolas Weeger (Laposte) wrote:

>> Which are harmless, but I think we should do a real logging system so that
>> isn't needed - instead, pass in the type of log message (object, map,
>> player, spell) to the log function, and then be able to specify what log
>> messages we want via command like, like 'crossfire -debugmsg +map,+player'
>> - that is what a lot of other programs do, and to me makes a lot more
>> sense.
> 
> Would require a rewrite of log stuff, and many call. I'd say we should do it 
> at once though, it will avoid having deprecated LOG and new LOG2 around :)

  My thought to make this easier is to make a bitmasks (since one log message 
could be about several thing anyways), and then just or that with the current 
passed log level (llevDebug, llevError, etc).

  In this way, it can be done piecemeal.  If no functionality is passed (eg, 
just a llevDebug, because the code is old), it would be treated as either it 
matches everything, or it defaults into some generic case.

  In this way, these can be cleaned up on a case by case basis. There are 
apparantly 1012 LOG calls within the code, so cleaning them all up would be a 
big process.

  There are currently 4 log levels - error, info, debug, and monster.

  Monster is used in just a few place.
  info is used in about 30 places.  I'm not quite sure how info and debug differ 
right now.

  I'd suggest 3 notification levels:

Error: Something is clearly wrong, and this typically means that operation can 
not complete (eg, function returns when detecting this instead of doing what it 
was requested).  This would be the default log level.  All errors generated are 
things that should be fixed - under normal operation, crossfire should not 
generate things at the error level.

Warning: something in the data looks suspect, but that doesn't prevent the 
operation from completing.  An example of this right now is the loading of 
objects - it prints a message about anything being put in a key-value list - 
that is generally fine, but could be cases of a typo and maybe something to pay 
attention to (eg, someone does max_grce instead of max_grace).  Running at the 
warning level would show more messages, but these may or may not be messages.

  Debug: Purely for debugging purposes - these are only here to be useful when 
trying to track down a problem, and would not have any value to be used during 
normal operation.

  Then on top of this, the different categories would be added.  Those 
categories would need to be discussed - one message could belong to several 
categories, so would be good to sort that out before this got too far along.

> 
>>   A fair number of other #if 0 are disabling small portions of code to turn
>> off some undesirable behaviour.  I'm thinking that these should either be
>> just pulled out, or if they might be useful, should be enabled either via
>> real #ifdefs or settings.
> 
> Agreed on that. If something is disabled, either it's broken and should go, or 
> should be compilable through settings.
> Maybe though as many things as possible should be settings, not #define. 
> Simply because Crossfire is not too easy to compile under Windows, so I'd 
> rather have people use settings than say « please recompile with 
> --enable-obscure-feature » :)

  Yes - there really shouldn't be any code that is selected by compile time 
options (or very few).  Aside from difficulty on windows for that, the problem 
is that if those are normally turned off, the code can be broken (compile wise) 
for quite a while before anyone notices.

  However, even with run time defaults, that is somewhat the cases - sure, the 
code compiles, but if it is never executed, it can still be broken.  The new 
test framework can probably help in this.  But if there are options that no one 
ever uses, better to remove that code and not have to worry about that potential 
support problem.  I'd argue that anything that is an #if 0 right now falls into 
the 'never used category'.