276483 - replacement for PR_smprintf/PR_vsmprintf

Status: NEW

(NSPR :: NSPR, enhancement)

Description

[Tim Ruehsen]

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 1

[Tim Ruehsen]

Attachment: cvs diff for prprf.h and prprf.c

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Tim Ruehsen]

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. 
 

Comment 4

[Tim Ruehsen]

Attachment: cvs diff for prprf.h and prprf.c

changed error return from -1 to 0

Comment 5

[Tim Ruehsen]

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. 
 

Comment 6

[Tim Ruehsen]

Attachment: extended sprintf test case for PR_asprintf

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.

Comment 7

[Wan-Teh Chang]

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

Comment 8

[Rich Megginson]

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 9

[Frank Wein [:mcsmurf]]

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

obsolete review request

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.