[CF-Devel] Questions about plugin code

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

Andreas Kirschbaum wrote:

>
     
       - CFSetSkillExperience() calls HOOK_ADDEXP(..., uint64 *exp, ...) but
     
     >
     
         CFWAddExp() expects "int *exp".
     
     >
     
     
     >
     
         I'd like to change the plugin interface to use "uint64 *". My
     
     >
     
         question: is it acceptable to change the plugin interface? That is:
     
     >
     
         do other plugin modules (outside the server repository) exist or is
     
     >
     
         it sufficient to ensure that the server interface and the plugin
     
     >
     
         implementations match?
     
     
  I'm not aware of any outside of the official distribution - doesn't mean they 
don't exist, but I certainly think they should get fixed.

  If backwards compatibility is really desired, could follow the model of other 
libraries and make something like 'CFWAddExp64()', and then just hae CFWAddExp 
just be a wrapper around that that copies the data in/out of a private variable.

>
     
     
     >
     
       - CFForgetSpell() calls HOOK_FORGETSPELL(..., object *spell) but
     
     >
     
         CFWDoForgetSpell expects "char *spell".
     
     >
     
     
     >
     
         In this case, I changed the Python api to use the spell name instead
     
     >
     
         of the spell object. (That is, use PyArg_ParseTuple(..."ls"...)
     
     >
     
         instead ..."li"...)
     
     
  That makes sense to me - while using strings may be a bit slower, it is 
certainly easier/more flexible, as otherwise the code has to look up the object, 
pass that in, etc.

>
     
     
     >
     
       - CFCreateObjectInside() contains some code I do not understand but
     
     >
     
         assume it to be very broken:
     
     >
     
     
     <snip>
>
     
          - (1) uses malloc() but calls free_string()
     
     
  which is bad.

>
     
          - calls malloc() repeatedly in the loop. This is not necessary
     
     >
     
            because the first allocation of malloc(strlen(...)+1) is
     
     >
     
            sufficient for all following uses.
     
     
  yep.

>
     
     
     >
     
          - calls query_name(myob) multiple times. Since query_cost() is quite
     
     >
     
            expensive, there should be only one call like
     
     >
     
            "char *myob_name = query_name(myob)"
     
     
  I agree.

>
     
     
     >
     
          - does not check if malloc() was successful
     
     
  IMO, that is less a concern, as lots of code in crossfire probably doesn't. 
And the best it can really do if it was not successful is to just bail out and 
end executation.

  There is certainly problem depending on what the malloc'd string is also used 
for - if for example it is then assigned to something in the object structure 
which free_string is used on later, that doesn't work.

  I was thinking at some point that getting rid of the shared string stuff all 
together could make sense, just because there are issues like this.



>
     
       - CFSetCha() (and similar functions for other stats) do
     
     >
     
     
     >
     
           if (value>30) return NULL;
     
     >
     
           if (value<-30) return NULL;
     
     >
     
     
     >
     
         to restrict the new value to valid values. I would like to change
     
     >
     
         these statements to
     
     >
     
     
     >
     
           if (value>MAX_STAT) return NULL;
     
     >
     
           if (value<MIN_STAT) return NULL;
     
     >
     
     
     >
     
         but MIN_STAT is not -30. Therefore: is my fix broken or is the value
     
     >
     
         -30 incorrect?
     
     
  I believe the issue is that the MAX/MIN_STAT values are defined to cover 
creatures/players, becuase that is basically the legal range of the tables which 
say what the respective bonuses are.

  However, certainly objects can have negative stats - cursed items, and even 
some of the non cursed ones (there are some that give pluses and minuses).

  So I'm not sure the best way to cover that case.

>
     
     
     >
     
       - CFWDumpObject() allocates a buffer with malloc() and returns it to
     
     >
     
         the plugin implementation. This buffer is never freed.
     
     >
     
     
     >
     
         Should I use a static buffer or free it in the plugin implementation?
     
     
  Well, if it can be done via static buffer, that may be the best way to go - 
sometimes that doesn't work if the data is referenced at a later time.  But in 
that case, malloc with it being freed when it is no longer needed would be the 
way to go.


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