Closed Bug 901820 Opened 11 years ago Closed 11 years ago

Replace nsCRT::strdup with strdup

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

Attachments

(4 files, 2 obsolete files)

Also, no one uses nsCRT::strndup, so it should be removed
Comment on attachment 786128 [details] [diff] [review]
Part 2. Remove nsCRT::strdup/nsCRT::strdup impl.

># HG changeset patch
># Parent 97e97cf477c7d772b5b9a68a6bbbecb88cfec780
>
>diff --git a/intl/uconv/ucvcn/nsGBKConvUtil.cpp b/intl/uconv/ucvcn/nsGBKConvUtil.cpp
>--- a/intl/uconv/ucvcn/nsGBKConvUtil.cpp
>+++ b/intl/uconv/ucvcn/nsGBKConvUtil.cpp
>@@ -1,16 +1,16 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsGBKConvUtil.h"
> #include "gbku.h"
>-#include "nsCRT.h"
>+#include "nsDebug.h"
> #define MAX_GBK_LENGTH 24066 /* (0xfe-0x80)*(0xfe-0x3f) */
> //--------------------------------------------------------------------
> // nsGBKConvUtil
> //--------------------------------------------------------------------
> 
> static const PRUnichar gGBKToUnicodeTable[MAX_GBK_LENGTH] = {
> #include "cp936map.h"
> };
>diff --git a/xpcom/ds/moz.build b/xpcom/ds/moz.build
>--- a/xpcom/ds/moz.build
>+++ b/xpcom/ds/moz.build
>@@ -46,17 +46,16 @@ XPIDL_MODULE = 'xpcom_ds'
> MODULE = 'xpcom'
> 
> EXPORTS += [
>     'nsArray.h',
>     'nsAtomService.h',
>     'nsCRT.h',
>     'nsCharSeparatedTokenizer.h',
>     'nsCheapSets.h',
>-    'nsCppSharedAllocator.h',
>     'nsExpirationTracker.h',
>     'nsHashPropertyBag.h',
>     'nsHashtable.h',
>     'nsIByteBuffer.h',
>     'nsIUnicharBuffer.h',
>     'nsMathUtils.h',
>     'nsObserverList.h',
>     'nsObserverService.h',
>diff --git a/xpcom/ds/nsCRT.cpp b/xpcom/ds/nsCRT.cpp
>--- a/xpcom/ds/nsCRT.cpp
>+++ b/xpcom/ds/nsCRT.cpp
>@@ -144,34 +144,16 @@ const char* nsCRT::memmem(const char* ha
>   for (uint32_t i = 0; i < haystackLen - needleLen; i++) {
>     if (!memcmp(haystack + i, needle, needleLen))
>       return haystack + i;
>   }
> #endif
>   return NULL;
> }
> 
>-PRUnichar* nsCRT::strdup(const PRUnichar* str)
>-{
>-  uint32_t len = NS_strlen(str);
>-  return strndup(str, len);
>-}
>-
>-PRUnichar* nsCRT::strndup(const PRUnichar* str, uint32_t len)
>-{
>-	nsCppSharedAllocator<PRUnichar> shared_allocator;
>-	PRUnichar* rslt = shared_allocator.allocate(len + 1); // add one for the null
>-  // PRUnichar* rslt = new PRUnichar[len + 1];
>-
>-  if (rslt == NULL) return NULL;
>-  memcpy(rslt, str, len * sizeof(PRUnichar));
>-  rslt[len] = 0;
>-  return rslt;
>-}
>-
> // This should use NSPR but NSPR isn't exporting its PR_strtoll function
> // Until then...
> int64_t nsCRT::atoll(const char *str)
> {
>     if (!str)
>         return 0;
> 
>     int64_t ll = 0;
>diff --git a/xpcom/ds/nsCRT.h b/xpcom/ds/nsCRT.h
>--- a/xpcom/ds/nsCRT.h
>+++ b/xpcom/ds/nsCRT.h
>@@ -5,17 +5,16 @@
> #ifndef nsCRT_h___
> #define nsCRT_h___
> 
> #include <stdlib.h>
> #include <string.h>
> #include <ctype.h>
> #include "plstr.h"
> #include "nscore.h"
>-#include "nsCppSharedAllocator.h"
> #include "nsCRTGlue.h"
> 
> #if defined(XP_WIN) || defined(XP_OS2)
> #  define NS_LINEBREAK           "\015\012"
> #  define NS_LINEBREAK_LEN       2
> #else
> #  ifdef XP_UNIX
> #    define NS_LINEBREAK         "\012"
>@@ -97,28 +96,16 @@ public:
> 
>   static int32_t strncmp(const char* s1, const char* s2, int32_t aMaxLen) {
>     // inline the first test (assumes strings are not null):
>     int32_t diff = ((const unsigned char*)s1)[0] - ((const unsigned char*)s2)[0];
>     if (diff != 0) return diff;
>     return int32_t(PL_strncmp(s1,s2,unsigned(aMaxLen)));
>   }
>   
>-  static char* strdup(const char* str) {
>-    return PL_strdup(str);
>-  }
>-
>-  static char* strndup(const char* str, uint32_t len) {
>-    return PL_strndup(str, len);
>-  }
>-
>-  static void free(char* str) {
>-    PL_strfree(str);
>-  }
>-
>   /**
> 
>     How to use this fancy (thread-safe) version of strtok: 
> 
>     void main(void) {
>       printf("%s\n\nTokens:\n", string);
>       // Establish string and get the first token:
>       char* newStr;
>@@ -142,29 +129,16 @@ public:
>                          uint32_t aMaxLen);
> 
>   // The GNU libc has memmem, which is strstr except for binary data
>   // This is our own implementation that uses memmem on platforms
>   // where it's available.
>   static const char* memmem(const char* haystack, uint32_t haystackLen,
>                             const char* needle, uint32_t needleLen);
> 
>-  // You must use nsCRT::free(PRUnichar*) to free memory allocated
>-  // by nsCRT::strdup(PRUnichar*).
>-  static PRUnichar* strdup(const PRUnichar* str);
>-
>-  // You must use nsCRT::free(PRUnichar*) to free memory allocated
>-  // by strndup(PRUnichar*, uint32_t).
>-  static PRUnichar* strndup(const PRUnichar* str, uint32_t len);
>-
>-  static void free(PRUnichar* str) {
>-  	nsCppSharedAllocator<PRUnichar> shared_allocator;
>-  	shared_allocator.deallocate(str, 0 /*we never new or kept the size*/);
>-  }
>-
>   // String to longlong
>   static int64_t atoll(const char *str);
>   
>   static char ToUpper(char aChar) { return NS_ToUpper(aChar); }
>   static char ToLower(char aChar) { return NS_ToLower(aChar); }
>   
>   static bool IsUpper(char aChar) { return NS_IsUpper(aChar); }
>   static bool IsLower(char aChar) { return NS_IsLower(aChar); }
>diff --git a/xpcom/ds/nsCppSharedAllocator.h b/xpcom/ds/nsCppSharedAllocator.h
>deleted file mode 100644
>--- a/xpcom/ds/nsCppSharedAllocator.h
>+++ /dev/null
>@@ -1,104 +0,0 @@
>-/* This Source Code Form is subject to the terms of the Mozilla Public
>- * License, v. 2.0. If a copy of the MPL was not distributed with this
>- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>-
>-#ifndef nsCppSharedAllocator_h__
>-#define nsCppSharedAllocator_h__
>-
>-#include "nsMemory.h"     // for |nsMemory|
>-#include <new>					// to allow placement |new|
>-
>-
>-  // under MSVC shut off copious warnings about unused in-lines
>-#ifdef _MSC_VER
>-  #pragma warning( disable: 4514 )
>-#endif
>-
>-#include <limits.h>
>-
>-
>-template <class T>
>-class nsCppSharedAllocator
>-    /*
>-      ...allows Standard Library containers, et al, to use our global shared
>-      (XP)COM-aware allocator.
>-    */
>-  {
>-    public:
>-      typedef T          value_type;
>-      typedef size_t     size_type;
>-      typedef ptrdiff_t  difference_type;
>-
>-      typedef T*         pointer;
>-      typedef const T*   const_pointer;
>-
>-      typedef T&         reference;
>-      typedef const T&   const_reference;
>-
>-
>-
>-      nsCppSharedAllocator() { }
>-
>-     ~nsCppSharedAllocator() { }
>-
>-
>-      pointer
>-      address( reference r ) const
>-        {
>-          return &r;
>-        }
>-
>-      const_pointer
>-      address( const_reference r ) const
>-        {
>-          return &r;
>-        }
>-
>-      pointer
>-      allocate( size_type n, const void* /*hint*/=0 )
>-        {
>-          return reinterpret_cast<pointer>(nsMemory::Alloc(static_cast<uint32_t>(n*sizeof(T))));
>-        }
>-
>-      void
>-      deallocate( pointer p, size_type /*n*/ )
>-        {
>-          nsMemory::Free(p);
>-        }
>-
>-      void
>-      construct( pointer p, const T& val )
>-        {
>-          new (p) T(val);
>-        }
>-      
>-      void
>-      destroy( pointer p )
>-        {
>-          p->~T();
>-        }
>-
>-      size_type
>-      max_size() const
>-        {
>-          return ULONG_MAX / sizeof(T);
>-        }
>-
>-  };
>-
>-
>-template <class T>
>-bool
>-operator==( const nsCppSharedAllocator<T>&, const nsCppSharedAllocator<T>& )
>-  {
>-    return true;
>-  }
>-
>-template <class T>
>-bool
>-operator!=( const nsCppSharedAllocator<T>&, const nsCppSharedAllocator<T>& )
>-  {
>-    return false;
>-  }
>-
>-#endif /* !defined(nsCppSharedAllocator_h__) */
>diff --git a/xpcom/glue/nsDeque.cpp b/xpcom/glue/nsDeque.cpp
>--- a/xpcom/glue/nsDeque.cpp
>+++ b/xpcom/glue/nsDeque.cpp
>@@ -1,15 +1,16 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsDeque.h"
>-#include "nsCRT.h"
>+#include "nsTraceRefcnt.h"
>+#include <string.h>
> #ifdef DEBUG_rickg
> #include <stdio.h>
> #endif
> 
> /**
>  * 07/02/2001  09:17p 509,104 clangref.pdf from openwatcom's site
>  * Watcom C Language Reference Edition 11.0c
>  * page 118 of 297
Attachment #786128 - Attachment is patch: true
Comment on attachment 786124 [details] [diff] [review]
Part 1. Replace nsCRT::strdup with strdup

Replace nsCRT::strdup with strdup/NS_strdup
Attachment #786124 - Flags: review?(benjamin)
Comment on attachment 786128 [details] [diff] [review]
Part 2. Remove nsCRT::strdup/nsCRT::strdup impl.

Remove implementation of strdup/strndup and nsCppSharedAllocator.h
Attachment #786128 - Flags: review?(benjamin)
Attached patch Part 3. comm-central change (obsolete) — Splinter Review
Attachment #786130 - Flags: review?
Attachment #786130 - Flags: review? → review?(mbanner)
Comment on attachment 786130 [details] [diff] [review]
Part 3. comm-central change

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

Neil's got more experience in this area, and I'm cutting down on non-critical patches for the next few weeks.
Attachment #786130 - Flags: review?(mbanner) → review?(neil)
(In reply to Makoto Kato from comment #4)
> Replace nsCRT::strdup with strdup/NS_strdup

What, if any, difference is there between strdup and NS_strdup, in particular for code that isn't linked into libxul, and where the deallocator is NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY?
Attachment #786124 - Flags: review?(benjamin) → review+
Attachment #786128 - Flags: review?(benjamin) → review+
Attachment #786130 - Flags: review?(neil)
At least, Original code in c-c shouldn't use nsCRT::strdup(const char*) since this is XPCOM code.

nsCRT::strdup(const char*) uses PL_strdup...
Attachment #786130 - Attachment is obsolete: true
Attachment #788800 - Flags: review?(neil)
Attachment #788800 - Attachment is patch: true
Comment on attachment 788800 [details] [diff] [review]
Bug 901820 - Part 3. comm-cetral part of replacing nsCRT::strdup

I used to have an allocator mismatch tracking build once. After a few patches I managed to get it to open the browser, but I didn't dare open mail...
Attachment #788800 - Flags: review?(neil) → review+
Looks like you overlooked one occurrence in comm-central:
../../../../ldap/xpcom/src/nsLDAPService.cpp:905:38: error: ‘moz_strndup’ is not a member of ‘nsCRT’
So what is the correct fix for nsLDAPService.cpp? NS_strndup, PL_strndup, strndup? I'm not really familiar with this, so would be nice if someone else can help with that.
Attached patch fix for nsCRT::strndup (obsolete) — Splinter Review
Attachment #794547 - Flags: review?(neil)
(In reply to Frank Wein [:mcsmurf] from comment #17)
> So what is the correct fix for nsLDAPService.cpp? NS_strndup, PL_strndup,
> strndup? I'm not really familiar with this, so would be nice if someone else
> can help with that.

Ah, I add additional patch.  Since It is XPCOM method, we should use nsMemory/NS_Alloc allocator, not PL_strndup/strdup.
Comment on attachment 794547 [details] [diff] [review]
fix for nsCRT::strndup

>         if (*p != '=') {
...
>             return NS_ERROR_UNEXPECTED;
>         }
>-        if (!(attrNameArray[index] = nsCRT::strndup(*component, len))) {
>+        if (!(attrNameArray[index] = ToNewCString(nsDependentCString(*component, len)))) {
I don't know why they have separate variables for len and p = component + len but you'll find that component[len] is not null and therefore you can't use a dependent string on it.
Attachment #794547 - Flags: review?(neil) → review-
Attachment #794547 - Attachment is obsolete: true
Attachment #794558 - Flags: review?(neil)
Comment on attachment 794558 [details] [diff] [review]
Fix bustrage for nsCRT::strndup removal v2

>+        if (!(attrNameArray[index] = (char*)NS_Alloc(len + 1))) {
Nit: prefer static_cast<>. (The line is getting a bit long so you may want to split it into separate assignment and test statements.)

>             NS_ERROR("nsLDAPService::ParseDn: out of memory ");
>             ldap_value_free(dnComponents);
>             ldap_value_free(rdnComponents);
>             NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(index, attrNameArray);
>             return NS_ERROR_OUT_OF_MEMORY;
>         }
>+        memcpy(attrNameArray[index], *component, len);
>+        *(attrNameArray[index] + len) = '\0';
Nit: Just use attrNameArray[index][len].
Attachment #794558 - Flags: review?(neil) → review+
You need to log in before you can comment on or make changes to this bug.