[CF-Devel] Bug in stealing code

[crossfire-devel at archives.real-time.com]

Mark Wedel wrote:
>
     
      common/item.c: is the get_weight() function really correct? I
     
     >
     
      note that it does not factor in the number of objects. For
     
     >
     
      example, if there are 30 arrows, get_weight returns the weight
     
     >
     
      of just 1 arrow. I'm not sure if that is intentional, but would
     
     >
     
      perhaps seem confusion. I do see it matches the WEIGHT() macro
     
     >
     
      definition in socket/item.c, so maybe so,
     
     
It is correct because the client needs both the weight of *one*
item and the number of items. It calculates the total weight of
all items itself.

>
     
      but now I'm curious why make that a function, compared to moving
     
     >
     
      the macro to define.h or the like.
     
     
OK, will declare it as a macro in define.h.

The reason for using a function is that I generally tend to avoid
macros: when calling a macro, you have to worry about side effects
(due to possible multiple evaluations of parameters) and also do
not get type-checking by the compiler.


>
     
      server/login.c: I'm not sure if the addition of a
     
     >
     
      esrv_update_item is really necessary, since it immediately
     
     >
     
      follows the esvr_new_player, and weight shouldn't have changed
     
     >
     
      between those two calls.
     
     
After checking the source, esrv_new_player indeed sends the
weight. But it does not set the field last_weight. I'll add this
assignment to last_weight, and remove the call to
esrv_update_item.

The reason for adding the call to esrv_update_item was to have
only one place that sets last_weight. But I realized that
esrv_new_player is called two times while creating a new
character, so setting the field last_weight is indeed a good idea.


>
     
      The use of assert doesn't follow with the current coding style,
     
     >
     
      and it's usage is questionable.
     
     >
     
     
     >
     
      For the few uses, the question is 'if the assert is false, is
     
     >
     
      this a significant error'. If the answer is yes, then having it
     
     >
     
      as an assert may do no good, because the asserts do nothing at
     
     >
     
      all unless NDEBUG is defined, which currently doesn't happen
     
     >
     
      anyplace.
     
     >
     
     
     >
     
      Thus also means that in normal running code, an error isn't even
     
     >
     
      noted at such asserts. So the usefulness in debugging is pretty
     
     >
     
      limited.
     
     
Java exhibits this limited usefulness that you have to explicitly
enable assertions (at runtime). The behavior of C assertions is
exactly the opposite: the C standard says (in section 7.2):

|
     
     If NDEBUG is defined as a macro name at the point in the source
     
     |
     
     file where <assert.h> is included, the assert macro is defined
     
     |
     
     simply as
     
     |
     
     
     |
     
     #define assert(ignore) ((void)0)
     
     
That is, assert() does nothing *only if* NDEBUG ("no debug") is
defined. Otherwise (i.e. if you do not explicitly disable the
assertions by defining NDEBUG), the assertions are active.


>
     
      The current methodoly used in the code is to check those
     
     >
     
      expressions manually (if ... { .. }) and LOG() in all such
     
     >
     
      cases, with the priority of the log message (llevDebug, llWarn,
     
     >
     
      llevError) based on how critical the error is.
     
     
This seems right to me for problems that should not happen but may
very well be possible. (I.e. can't read/write a file, a passed
object has wrong type/flags/whatever, etc.)

>
     
      And if it is a critical error, to the extend that trying to
     
     >
     
      continue will just cause it to core dump in the next line or
     
     >
     
      two, might as well call abort right then and there.
     
     
These are the types of errors that should be checked with
assert(): to detect an inconsistent internal state that should
never be possible. Therefore assert() prints an error message to
stderr and calls abort() if it fails.

_______________________________________________
crossfire-devel mailing list
     
     crossfire-devel at lists.real-time.com
     
     
     https://mailman.real-time.com/mailman/listinfo/crossfire-devel