Menu

#244 Improved lib_mvsprintf()

v3.x
closed-accepted
nobody
None
enhancement
2020-10-14
2020-10-11
No

While looking into the code, I noticed that there is a function called lib_mvsprintf() into vice/src/lib.c.
It seems that this function must return a formatted string allocated from the heap and you used function vsnprintf() by testing continuously into a loop its return value, until it won't return that the print has been done correctly.
In my opinion, there is a better and simpler solution for implementing this task.
When function vsnprinf receives its "size" parameter to zero, it works as usual but it has the effect to ignore the pointer to the output buffer. So, when used in this manner, the function does basically the task to calculate the exact size of the output string and the returned value can be used to allocate the size you need. The body of the new function is much shorter and direct:

va_copy(args, ap);
maxlen = vsnprintf (NULL, 0, fmt, args);

if ((p = lib_malloc (maxlen + 1)) == NULL)
    return NULL;

vsnprintf (p, maxlen + 1, fmt, ap);

This behavior of vsnprintf() is standard, so I think that it can be used without problems.
Sincerely.

Discussion

  • Carlo Bramini

    Carlo Bramini - 2020-10-11

    The patch.

     
  • compyx

    compyx - 2020-10-12

    I haven't checked the patch for correctness yet, but please use brackets for each branch, ie change:

    if ((p = lib_malloc (maxlen + 1)) == NULL)
        return NULL;
    

    into:

    if ((p = lib_malloc (maxlen + 1)) == NULL) {
        return NULL;
    }
    

    I know it's a bit pedantic, but it'll save us from weird bugs in the future.

     
  • gpz

    gpz - 2020-10-12

    that patch is re-introducing another bug... missing va_end :)

     
  • gpz

    gpz - 2020-10-12

    applied a hopefully working adaption in r38771 - thanks for the hint, i should have done it this way in the first place in my previous fixing attempt :)

     
    • Carlo Bramini

      Carlo Bramini - 2020-10-13

      that patch is re-introducing another bug... missing va_end :)

      Ouch! It escaped to my attention, sorry! ^^;

      The second use of va_copy() and va_end() is pointless (the line in between them doesn't use args).

      That's right, also in my opinion those two lines are redundant. If you want to preserve "ap" parameter, "args" should be used also inside the second vsnprintf().

      Thank you very much.

      Sincerely.

       

      Last edit: Carlo Bramini 2020-10-13
  • Greg King

    Greg King - 2020-10-12

    The second use of va_copy() and va_end() is pointless (the line in between them doesn't use args).

     
  • gpz

    gpz - 2020-10-14

    oops

     

Log in to post a comment.

MongoDB Logo MongoDB