Closed Bug 1201287 Opened 9 years ago Closed 9 years ago

Cleanup nsSupportsPrimitives.cpp

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: mikokm, Assigned: mikokm)

Details

Attachments

(1 file, 3 obsolete files)

- There is a lot of code duplication in xpcom/ds/nsSupportsPrimitives.cpp.
- The buffer lengths for nsMemory::clone() are checked using O(n) strlen even though snprintf can return the number of characters written.
Attached patch Cleanup nsSupportsPrimitives.cpp (obsolete) — Splinter Review
Attached patch Cleanup nsSupportsPrimitives.cpp (obsolete) — Splinter Review
Attachment #8656251 - Attachment is obsolete: true
Attachment #8656251 - Flags: review?(bugs)
Assignee: nobody → mikokm
Comment on attachment 8656265 [details] [diff] [review]
Cleanup nsSupportsPrimitives.cpp


>+template<int N, typename T>
>+static char*
>+DataToString(const char* aFormat, T aData)
>+{
>+  char buf[N];
>+  int len = snprintf_literal(buf, aFormat, aData);
>+
>+  if(len < 0 || len > N) {
>+    return NULL;
s/NULL/nullptr/


>+  }
>+
>+  return (char*)nsMemory::Clone(buf, (len + 1) * sizeof(char));
>+}
Hmm, do we actually need to pass N explicitly?
We could just use some large enough size for buf and then MOZ_ASSERT here
that len is always smaller than that size. Since buf is stack only allocation, it doesn't affect much to performance whether say 8 or 128 is used.

Also, looks like nsMemory::Clone uses NS_Alloc, which is infallible, so this all can be simplified some more.

https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsMemory.cpp?rev=b276ce875275#31
http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPCOM.h?rev=90e184f46b30#208

using C++ casting here would be nice.
Attachment #8656265 - Flags: review?(bugs) → review-
Attached patch Cleanup nsSupportsPrimitives.cpp (obsolete) — Splinter Review
Attachment #8656265 - Attachment is obsolete: true
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug]
> Comment on attachment 8656265 [details] [diff] [review]
Thank you for the review and help.

> Cleanup nsSupportsPrimitives.cpp
> 
> s/NULL/nullptr/

Done.

> Hmm, do we actually need to pass N explicitly?
> We could just use some large enough size for buf and then MOZ_ASSERT here
> that len is always smaller than that size. Since buf is stack only
> allocation, it doesn't affect much to performance whether say 8 or 128 is
> used.

Changed N to fixed size 32. Added MOZ_ASSERT.

> Also, looks like nsMemory::Clone uses NS_Alloc, which is infallible, so this
> all can be simplified some more.
> using C++ casting here would be nice.
Removed all memory allocation checks from infallible allocations and replaced the C-style casts.
Also fixed line breaks to be more consistent and removed few useless temporary variables.
Comment on attachment 8656798 [details] [diff] [review]
Cleanup nsSupportsPrimitives.cpp

z>+template<typename T>
>+static char*
>+DataToString(const char* aFormat, T aData)
>+{
>+  static const int size = 32;
>+  char buf[size];
>+
>+  int len = snprintf_literal(buf, aFormat, aData);
>+  MOZ_ASSERT(len > 0 && len < size);
>+
>+  return static_cast<char*>(nsMemory::Clone(buf, (len + 1) * sizeof(char)));

Silly me, double for example take more than 32 chars as string.
So, remove the assert and we just keep the existing behavior where some decimals are possibly cut
and for cloning use std::min(len + 1, buf); since len may be longer than buf.
Flags: needinfo?(bugs)
Attachment #8656798 - Flags: review+
Attachment #8656798 - Attachment is obsolete: true
Comment on attachment 8657598 [details] [diff] [review]
Cleanup nsSupportsPrimitives.cpp

- Fixed some trailing white space and line lengths.
- Buffer length is now min(length + 1, 32).
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/13104b0d5cfd
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.