Signed-off-by: Michael Stefaniuc mstefani@winehq.org --- There are about 40+ more strdupW() functions around. Most using that name, some prefixed with something else. Of course that is fodder for more patches.
dlls/appwiz.cpl/appwiz.h | 16 ---------------- dlls/dpnet/dpnet_private.h | 8 -------- dlls/dwrite/dwrite_private.h | 16 ---------------- dlls/ieframe/ieframe.h | 16 ---------------- dlls/inetcomm/protocol.c | 16 ---------------- dlls/jscript/jscript.h | 16 ---------------- dlls/mshtml/mshtml_private.h | 16 ---------------- dlls/msxml3/msxml_private.h | 16 ---------------- dlls/ole32/errorinfo.c | 16 ---------------- dlls/sapi/sapi_private.h | 15 --------------- dlls/schedsvc/schedsvc_private.h | 10 ---------- dlls/taskschd/taskschd_private.h | 10 ---------- dlls/urlmon/urlmon_main.h | 16 ---------------- dlls/vbscript/vbscript.h | 16 ---------------- dlls/wbemprox/wbemprox_private.h | 8 -------- dlls/wininet/internet.h | 16 ---------------- include/wine/heap.h | 16 ++++++++++++++++ 17 files changed, 16 insertions(+), 227 deletions(-)
diff --git a/dlls/appwiz.cpl/appwiz.h b/dlls/appwiz.cpl/appwiz.h index 2165ce11b1..5571ac158a 100644 --- a/dlls/appwiz.cpl/appwiz.h +++ b/dlls/appwiz.cpl/appwiz.h @@ -28,22 +28,6 @@ BOOL install_addon(addon_t) DECLSPEC_HIDDEN;
extern HINSTANCE hInst DECLSPEC_HIDDEN;
-static inline WCHAR *heap_strdupW(const WCHAR *str) -{ - WCHAR *ret; - - if(str) { - size_t size = strlenW(str)+1; - ret = heap_alloc(size*sizeof(WCHAR)); - if(ret) - memcpy(ret, str, size*sizeof(WCHAR)); - }else { - ret = NULL; - } - - return ret; -} - static inline WCHAR *heap_strdupAtoW(const char *str) { WCHAR *ret = NULL; diff --git a/dlls/dpnet/dpnet_private.h b/dlls/dpnet/dpnet_private.h index 2e72360c89..467ee87f6f 100644 --- a/dlls/dpnet/dpnet_private.h +++ b/dlls/dpnet/dpnet_private.h @@ -155,12 +155,4 @@ typedef struct { #define FE(x) { x, #x } #define GE(x) { &x, #x }
-static inline WCHAR *heap_strdupW( const WCHAR *src ) -{ - WCHAR *dst; - if (!src) return NULL; - if ((dst = heap_alloc( (strlenW( src ) + 1) * sizeof(WCHAR) ))) strcpyW( dst, src ); - return dst; -} - #endif diff --git a/dlls/dwrite/dwrite_private.h b/dlls/dwrite/dwrite_private.h index 31b977a159..867356fd73 100644 --- a/dlls/dwrite/dwrite_private.h +++ b/dlls/dwrite/dwrite_private.h @@ -31,22 +31,6 @@ static const DWRITE_MATRIX identity = 0.0f, 0.0f };
-static inline LPWSTR heap_strdupW(const WCHAR *str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - static inline LPWSTR heap_strdupnW(const WCHAR *str, UINT32 len) { WCHAR *ret = NULL; diff --git a/dlls/ieframe/ieframe.h b/dlls/ieframe/ieframe.h index 8adf47e053..d2ba262a34 100644 --- a/dlls/ieframe/ieframe.h +++ b/dlls/ieframe/ieframe.h @@ -335,22 +335,6 @@ static inline void unlock_module(void) { InterlockedDecrement(&module_ref); }
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - static inline LPWSTR co_strdupW(LPCWSTR str) { WCHAR *ret = CoTaskMemAlloc((strlenW(str) + 1)*sizeof(WCHAR)); diff --git a/dlls/inetcomm/protocol.c b/dlls/inetcomm/protocol.c index 028463a269..7741dc8fb5 100644 --- a/dlls/inetcomm/protocol.c +++ b/dlls/inetcomm/protocol.c @@ -63,22 +63,6 @@ typedef struct { static const WCHAR mhtml_prefixW[] = {'m','h','t','m','l',':'}; static const WCHAR mhtml_separatorW[] = {'!','x','-','u','s','c',':'};
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - static HRESULT parse_mhtml_url(const WCHAR *url, mhtml_url_t *r) { const WCHAR *p; diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 1a247511ac..f3ed3d3523 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -55,22 +55,6 @@ void heap_pool_clear(heap_pool_t*) DECLSPEC_HIDDEN; void heap_pool_free(heap_pool_t*) DECLSPEC_HIDDEN; heap_pool_t *heap_pool_mark(heap_pool_t*) DECLSPEC_HIDDEN;
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - typedef struct jsdisp_t jsdisp_t;
extern HINSTANCE jscript_hinstance DECLSPEC_HIDDEN; diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 5ed626477f..2b7018f38a 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -1189,22 +1189,6 @@ static inline void * __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, size_t le return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, len); }
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - static inline LPWSTR heap_strndupW(LPCWSTR str, unsigned len) { LPWSTR ret = NULL; diff --git a/dlls/msxml3/msxml_private.h b/dlls/msxml3/msxml_private.h index 94ef66b23d..6880294160 100644 --- a/dlls/msxml3/msxml_private.h +++ b/dlls/msxml3/msxml_private.h @@ -170,22 +170,6 @@ static inline void* __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, size_t siz return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, size); }
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - /* XSLProcessor parameter list */ struct xslprocessor_par { diff --git a/dlls/ole32/errorinfo.c b/dlls/ole32/errorinfo.c index d5ec17207a..19f1b04b21 100644 --- a/dlls/ole32/errorinfo.c +++ b/dlls/ole32/errorinfo.c @@ -41,22 +41,6 @@
WINE_DEFAULT_DEBUG_CHANNEL(ole);
-static inline WCHAR *heap_strdupW(const WCHAR *str) -{ - WCHAR *ret = NULL; - - if(str) { - size_t size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - typedef struct ErrorInfoImpl { IErrorInfo IErrorInfo_iface; diff --git a/dlls/sapi/sapi_private.h b/dlls/sapi/sapi_private.h index b069222c7d..cfc5f3e1ad 100644 --- a/dlls/sapi/sapi_private.h +++ b/dlls/sapi/sapi_private.h @@ -24,18 +24,3 @@ HRESULT data_key_create( IUnknown *outer, REFIID iid, void **obj ) DECLSPEC_HIDDEN; HRESULT token_category_create( IUnknown *outer, REFIID iid, void **obj ) DECLSPEC_HIDDEN; HRESULT token_enum_create( IUnknown *outer, REFIID iid, void **obj ) DECLSPEC_HIDDEN; - -static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - DWORD size; - - if (str) - { - size = (strlenW( str ) + 1) * sizeof(WCHAR); - ret = heap_alloc( size ); - if (ret) memcpy( ret, str, size ); - } - - return ret; -} diff --git a/dlls/schedsvc/schedsvc_private.h b/dlls/schedsvc/schedsvc_private.h index 4404486998..60901d8ad8 100644 --- a/dlls/schedsvc/schedsvc_private.h +++ b/dlls/schedsvc/schedsvc_private.h @@ -24,14 +24,4 @@
void schedsvc_auto_start(void) DECLSPEC_HIDDEN;
-static inline WCHAR *heap_strdupW(const WCHAR *src) -{ - WCHAR *dst; - unsigned len; - if (!src) return NULL; - len = (strlenW(src) + 1) * sizeof(WCHAR); - if ((dst = heap_alloc(len))) memcpy(dst, src, len); - return dst; -} - #endif /* __WINE_SCHEDSVC_PRIVATE_H__ */ diff --git a/dlls/taskschd/taskschd_private.h b/dlls/taskschd/taskschd_private.h index 6c7f916a0f..67383cbf59 100644 --- a/dlls/taskschd/taskschd_private.h +++ b/dlls/taskschd/taskschd_private.h @@ -32,14 +32,4 @@ HRESULT RegisteredTaskCollection_create(const WCHAR *path, IRegisteredTaskCollec
WCHAR *get_full_path(const WCHAR *parent, const WCHAR *path) DECLSPEC_HIDDEN;
-static inline WCHAR *heap_strdupW(const WCHAR *src) -{ - WCHAR *dst; - unsigned len; - if (!src) return NULL; - len = (strlenW(src) + 1) * sizeof(WCHAR); - if ((dst = heap_alloc(len))) memcpy(dst, src, len); - return dst; -} - #endif /* __WINE_TASKSCHD_PRIVATE_H__ */ diff --git a/dlls/urlmon/urlmon_main.h b/dlls/urlmon/urlmon_main.h index a4a1c78d52..89cdd389a6 100644 --- a/dlls/urlmon/urlmon_main.h +++ b/dlls/urlmon/urlmon_main.h @@ -240,22 +240,6 @@ static inline void* __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, size_t siz return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, size); }
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - static inline LPWSTR heap_strndupW(LPCWSTR str, int len) { LPWSTR ret = NULL; diff --git a/dlls/vbscript/vbscript.h b/dlls/vbscript/vbscript.h index 5e935ff76a..d8af4d6855 100644 --- a/dlls/vbscript/vbscript.h +++ b/dlls/vbscript/vbscript.h @@ -431,22 +431,6 @@ HRESULT create_safearray_iter(SAFEARRAY *sa, IEnumVARIANT **ev) DECLSPEC_HIDDEN; HRESULT WINAPI VBScriptFactory_CreateInstance(IClassFactory*,IUnknown*,REFIID,void**) DECLSPEC_HIDDEN; HRESULT WINAPI VBScriptRegExpFactory_CreateInstance(IClassFactory*,IUnknown*,REFIID,void**) DECLSPEC_HIDDEN;
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - #define VBSCRIPT_BUILD_VERSION 16978 #define VBSCRIPT_MAJOR_VERSION 5 #define VBSCRIPT_MINOR_VERSION 8 diff --git a/dlls/wbemprox/wbemprox_private.h b/dlls/wbemprox/wbemprox_private.h index 54b1ba3d8e..47dca73a26 100644 --- a/dlls/wbemprox/wbemprox_private.h +++ b/dlls/wbemprox/wbemprox_private.h @@ -228,14 +228,6 @@ HRESULT service_stop_service(IWbemClassObject *, IWbemClassObject *, IWbemClassO HRESULT security_get_sd(IWbemClassObject *, IWbemClassObject *, IWbemClassObject **) DECLSPEC_HIDDEN; HRESULT security_set_sd(IWbemClassObject *, IWbemClassObject *, IWbemClassObject **) DECLSPEC_HIDDEN;
-static inline WCHAR *heap_strdupW( const WCHAR *src ) -{ - WCHAR *dst; - if (!src) return NULL; - if ((dst = heap_alloc( (strlenW( src ) + 1) * sizeof(WCHAR) ))) strcpyW( dst, src ); - return dst; -} - static const WCHAR class_processW[] = {'W','i','n','3','2','_','P','r','o','c','e','s','s',0}; static const WCHAR class_serviceW[] = {'W','i','n','3','2','_','S','e','r','v','i','c','e',0}; static const WCHAR class_stdregprovW[] = {'S','t','d','R','e','g','P','r','o','v',0}; diff --git a/dlls/wininet/internet.h b/dlls/wininet/internet.h index 498a79b080..736506a9ce 100644 --- a/dlls/wininet/internet.h +++ b/dlls/wininet/internet.h @@ -95,22 +95,6 @@ static inline void * __WINE_ALLOC_SIZE(2) heap_realloc_zero(void *mem, size_t le return HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, mem, len); }
-static inline LPWSTR heap_strdupW(LPCWSTR str) -{ - LPWSTR ret = NULL; - - if(str) { - DWORD size; - - size = (strlenW(str)+1)*sizeof(WCHAR); - ret = heap_alloc(size); - if(ret) - memcpy(ret, str, size); - } - - return ret; -} - static inline char *heap_strdupA(const char *str) { char *ret = NULL; diff --git a/include/wine/heap.h b/include/wine/heap.h index 97d3a5662b..d970e03731 100644 --- a/include/wine/heap.h +++ b/include/wine/heap.h @@ -55,4 +55,20 @@ static inline void *heap_calloc(SIZE_T count, SIZE_T size) return HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len); }
+static inline WCHAR *heap_strdupW(const WCHAR *str) +{ + WCHAR *dst; + SIZE_T len; + + if(!str) + return NULL; + + len = (lstrlenW(str) + 1) * sizeof(*str); + dst = heap_alloc(len); + if (dst) + memcpy(dst, str, len); + + return dst; +} + #endif /* __WINE_WINE_HEAP_H */
Michael Stefaniuc mstefani@winehq.org writes:
+static inline WCHAR *heap_strdupW(const WCHAR *str) +{
- WCHAR *dst;
- SIZE_T len;
- if(!str)
return NULL;- len = (lstrlenW(str) + 1) * sizeof(*str);
- dst = heap_alloc(len);
- if (dst)
memcpy(dst, str, len);- return dst;
+}
I'm not sure the NULL check is a good idea.
(just when you thought this one would not be controversial ;-)
On 02/12/2018 11:23 AM, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@winehq.org writes:
+static inline WCHAR *heap_strdupW(const WCHAR *str) +{
- WCHAR *dst;
- SIZE_T len;
- if(!str)
return NULL;- len = (lstrlenW(str) + 1) * sizeof(*str);
- dst = heap_alloc(len);
- if (dst)
memcpy(dst, str, len);- return dst;
+}
I'm not sure the NULL check is a good idea.
And I disagree with your disagreement!
I just sent in [PATCH] wininet: Avoid passing NULL to heap_strdupW() to show how ugly that would make the code. And that's the stuff that just crashed due to the tests.
If you want the NULL check removed for the other strdup variants, especially the heap_strdupAW(), that would uglify the code even more as that is used in a ton of A functions that just forward to the W functions.
(just when you thought this one would not be controversial ;-)
bye michael
Michael Stefaniuc mstefani@winehq.org writes:
On 02/12/2018 11:23 AM, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@winehq.org writes:
+static inline WCHAR *heap_strdupW(const WCHAR *str) +{
- WCHAR *dst;
- SIZE_T len;
- if(!str)
return NULL;- len = (lstrlenW(str) + 1) * sizeof(*str);
- dst = heap_alloc(len);
- if (dst)
memcpy(dst, str, len);- return dst;
+}
I'm not sure the NULL check is a good idea.
And I disagree with your disagreement!
I just sent in [PATCH] wininet: Avoid passing NULL to heap_strdupW() to show how ugly that would make the code. And that's the stuff that just crashed due to the tests.
If you want the NULL check removed for the other strdup variants, especially the heap_strdupAW(), that would uglify the code even more as that is used in a ton of A functions that just forward to the W functions.
The current usage is clean only because we are not checking for allocation failure, but that's broken. If we add proper handling, then the NULL checks will be needed anyway.
On 2018-02-12 22:43, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@winehq.org writes:
On 02/12/2018 11:23 AM, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@winehq.org writes:
+static inline WCHAR *heap_strdupW(const WCHAR *str) +{
- WCHAR *dst;
- SIZE_T len;
- if(!str)
return NULL;- len = (lstrlenW(str) + 1) * sizeof(*str);
- dst = heap_alloc(len);
- if (dst)
memcpy(dst, str, len);- return dst;
+}
I'm not sure the NULL check is a good idea.
And I disagree with your disagreement!
I just sent in [PATCH] wininet: Avoid passing NULL to heap_strdupW() to show how ugly that would make the code. And that's the stuff that just crashed due to the tests.
If you want the NULL check removed for the other strdup variants, especially the heap_strdupAW(), that would uglify the code even more as that is used in a ton of A functions that just forward to the W functions.
The current usage is clean only because we are not checking for allocation failure, but that's broken. If we add proper handling, then the NULL checks will be needed anyway.
I assumed you want less HeapAlloc failure handling and not more! Especially as in the strdup cases the current "return NULL" seems to be good enough in practice. I don't remember to have ever seen a patch that adds extra error handling for that case.
So as your use case seems to be the exotic one it makes sense to move the extra check inside the helper and not have every caller deal with that. That would require changing the signature of the helper, e.g. (I don't care about the name nor parameter ordering):
BOOL heap_strdup_w(WCHAR **dst, const WCHAR *src);
FALSE would be only returned if the heap_alloc() fails.
I can easily automate the transformation to the new form without adding any extra error handling: foo = heap_strdupW(bar); would become heap_strdup_w(&foo, bar);
But adding the error check is left for the interested reader that cares about that. That cannot be automated. if (heap_strdup_w(&foo, bar)) return WHATERRORISAPPROPRIATEHERE; I doubt that we will see many of those.
bye michael
Michael Stefaniuc mstefani@mykolab.com writes:
The current usage is clean only because we are not checking for allocation failure, but that's broken. If we add proper handling, then the NULL checks will be needed anyway.
I assumed you want less HeapAlloc failure handling and not more! Especially as in the strdup cases the current "return NULL" seems to be good enough in practice. I don't remember to have ever seen a patch that adds extra error handling for that case.
You could argue that strdup() should be treated as a nofail function, but then you should remove the second null check and let it crash on the memcpy, or use HEAP_GENERATE_EXCEPTIONS. I'd prefer to have an explicit xstrdup() function for this though, and have the regular strdup() require error handling.
On 02/13/2018 10:50 AM, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@mykolab.com writes:
The current usage is clean only because we are not checking for allocation failure, but that's broken. If we add proper handling, then the NULL checks will be needed anyway.
I assumed you want less HeapAlloc failure handling and not more! Especially as in the strdup cases the current "return NULL" seems to be good enough in practice. I don't remember to have ever seen a patch that adds extra error handling for that case.
You could argue that strdup() should be treated as a nofail function, but then you should remove the second null check and let it crash on the memcpy, or use HEAP_GENERATE_EXCEPTIONS. I'd prefer to have an explicit xstrdup() function for this though, and have the regular strdup() require error handling
I don't mind a xstrdup() variant, remains to be seen how useful it would be.
I consider the current strdup() behavior to be superior to what you propose as it is generic: - One can still check for alloc errors iff it is needed. - Or let subsequent code deal with the NULL pointer. This seems to be the default and seems to work pretty well in practice.
The ship has sailed anyway: wine$ git grep -i -P 'strdup(\w+)?\s*(' origin dlls/ programs/ | wc -l 1606
Not all are call sites but even 1500 cases are way too many to figure out manually what error to return. And how many of those will be a "Needs tests"? A ton of work without a clear benefit.
As the strdup() work is controversial I'll skip making those generic.
bye michael
-
Alexandre Julliard -
Michael Stefaniuc -
Michael Stefaniuc