User-Agent: Mozilla/5.0 (compatible; Konqueror/3.3; Linux) (KHTML, like Gecko) Build Identifier: PR_smprintf/PR_vsmprintf do not return the length of the string buffer that has been created. In many cases we (redundantly) call strlen() to retrieve the length. To avoid this, I added the two functions PR_asprintf and PR_vasprintf which return the length (like sprintf does). They are similar to the two GNU functions asprintf/vasprintf. Summary: In order to avoid pairs of PR_smprintf/strlen, we should use PR_asprintf. It is more elegant and faster since the length has been calculated within PR_*printf anyway. Reproducible: Always

Comment on attachment 169917 [details] [diff] [review] cvs diff for prprf.h and prprf.c Tim, Thanks a lot for the bug report and the patch. Could you configure your text editor so it doesn't delete white spaces at the end of lines? Could you show me a few examples of pairs of PR_smprintf/strlen? > /* >+** Replacement for PR_smprintf() >+** >+** sprintf into a PR_MALLOC'd buffer. >+** If successful, returns the number of bytes written. outp points to the >+** allocated string buffer. >+** On error, returns -1. outp is initialized with 0. >+** >+** Call "PR_asprintf_free" to release the memory returned. >+*/ >+NSPR_API(PRUint32) PR_asprintf(char **outp, const char *fmt, ...); The -1 return value is impossible for two reasons. 1. The return type PRUint32 is unsigned. 2. Your code returns n ? n - 1 : n. This expression can never have the value -1.

to 1. I already thought about the -1 return code. Anyway, after calling PR_asprintf() we have to check if buffer allocation has been successful. So it does not matter if we return (the ambiguously) 0. I make a new patch. to 2. I took this line from PR_vsnprintf. It seems to be ok, since n is always >=0. So we never return a negative number. For the future, I will switch my editor to not delete trailing whitespace. In this case the changes are already done and saved.

changed error return from -1 to 0

a few examples: http://lxr.mozilla.org/mozilla1.7/source/dom/src/base/nsDOMException.cpp#322 (followed by an implicit strlen in Assign(temp) - could have been Assign(temp,len)) http://lxr.mozilla.org/mozilla1.7/source/mailnews/addrbook/src/nsVCardObj.cpp#546 (many times) http://lxr.mozilla.org/mozilla1.7/source/mailnews/compose/src/nsMsgCompUtils.cpp#450 (followed by PUSH_STRING which contains a call to PL_strlen()) http://lxr.mozilla.org/mozilla1.7/source/mailnews/compose/src/nsMsgSend.cpp#4429 (followed by PL_strlen()) as a rule of thumb: whenever one creates a string there comes the time that the string size is needed (directly or indirectly). Sometimes you can see buf = PL_smprintf(...); ... if (buf) some_cstring_class instance_of_cstring = buf; This assignment also calls PL_strlen(buf) to call PR_Malloc(). But it should be possible to do something like len = PL_asprintf(&buf,...); ... if (buf) some_cstring_class instance_of_cstring(buf,len); After all, one of the main tasks of firefox/mozilla/thunderbird is parsing strings and generating strings. We should always strive to have the best performance that is possible.

I added tests for PR_asprintf (nsprpub/pr/tests/sprintf.c). PR_asprintf passes the tests. BUT: pattern='%0lld' ll=0 PR_smprintf='0' PR_snprintf='0' PR_asprintf='0' sprintf='' FAIL Is it a glibc bug or feature? Is it worth a bug request? What can I do to get this bug fixed soon? I would like to start working on different files using PR_asprintf.

Brendan, Brian, Darin, could you review the proposed PR_asprintf function?

I believe this same functionality could be obtained if the %n specification were supported. You could do something like this: char *str = PR_smprintf("%s/%s%n", foo, bar, &len); Then %n could be used in all PR_*printf functions.

Comment on attachment 169917 [details] [diff] [review] cvs diff for prprf.h and prprf.c obsolete review request