1231900 - API Document for JS_snprintf() is non-consistent with the Implementation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wei Wu [:wuwei UTC+8]]

Separated from bug 1231581.
According to bug 1231581 comment 2, the document for JS_snprintf() is non-consistent with the code.

Comment 1

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1231900.patch

PASSED tests and jit-test.
I also modified the return value to outlen when JS_snprintf thought the outlen was converted from negative int32_t. The behaviour is consistent with JS_vsnprintf now. However, I am not sure whether the modification violates anything we care, so please correct me if anything goes wrong.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8697517 [details] [diff] [review]
bug1231900.patch

Review of attachment 8697517 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.  Just a couple of comments...

::: js/src/jsprf.cpp
@@ +997,5 @@
>      int rv;
>  
>      MOZ_ASSERT(int32_t(outlen) > 0);
>      if (int32_t(outlen) <= 0)
> +        return outlen;

Please remove the casting to int32_t here as well.  I can't see anywhere that int32_t is used to hold the length so we don't need to check for overflow, so this is unnecessary and confusing.  The if condition will can just be == since the type will be unsigned.

::: js/src/jsprf.h
@@ +31,5 @@
>  
>  /*
>  ** sprintf into a fixed size buffer. Guarantees that a NUL is at the end
>  ** of the buffer. Returns the length of the written output, NOT including
> +** the NUL, or the size of the buffer if an error occurs.

This is over specific.  The important thing to say is that the function has succeeded only if the return value is less than the size of the buffer.  The fact that we return the buffer length is an implementation detail.

Comment 3

[Wei Wu [:wuwei UTC+8]]

Attachment: v2

ver2

removed the int32_t things.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8697557 [details] [diff] [review]
v2

Review of attachment 8697557 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thank you.

Comment 5

[Wei Wu [:wuwei UTC+8]]

(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8697557 [details] [diff] [review]
> v2
> 
> Review of attachment 8697557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thank you.

My pleasure! Thank you for your review :)

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9267feff45

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab9267feff45