Re: [PATCH 04/25] syntax-check: fix violations and re-enable sc_cast_of_x_alloc_return_value.
Hi Eric, Chuck,
Thanks both for taking the time to review.
On 15 Nov 2011, at 23:43, Eric Blake wrote:
> On 11/15/2011 05:53 AM, Gary V. Vaughan wrote:
>> * cfg.mk (local-checks-to-fix): Remove
>> sc_cast_of_x_alloc_return_value from list of disabled checks.
>
> That check is only useful for pure C projects. If the intention is that
> libtool can be compiled under both C and C++, then C++ requires that you
> cast the result of allocation functions (only C lets you get away with
> going from void* to any other pointer without cast). Are we sure that
> no one tries to compile libtool with a C++ compiler?
On the contrary, we explicitly support compilation with g++ at least. But
`make distcheck' is already such a beast that I don't run all the variations
(make distcheck CC=g++) on each patch, which in this case was a big
oversight on my part.
On 15 Nov 2011, at 23:43, Charles Wilson wrote:
> On 11/15/2011 11:36 AM, Charles Wilson wrote:
>> On 11/15/2011 7:53 AM, Gary V. Vaughan wrote:
>>> * cfg.mk (local-checks-to-fix): Remove
>>> sc_cast_of_x_alloc_return_value from list of disabled checks.
>>> * libltdl/config/ltmain.m4sh (XMALLOC, XFREE): Unroll into their
>>> xmalloc and free expansions so that this syntax-check can find
>>> violations, and then fix them.
>>> * iibltdl/libltdl/lt__alloc.h (MALLOC, REALLOC): Renamed to
>>> xmalloc and xrealloc so that this syntax-check can find
>>> violations. Adjust all callers.
>>> (FREE, MEMREASSIGN): Removed. All callers unrolled into their
>>> former expansions, and violations of this syntax-check fixed.
>>> * libltdl/loaders/loadlibrary.c (LOCALFREE): Unrolled for
>>> consistency.
>>
>> Why do I get the feeling that the next time I try to build any .exe on
>> cygwin/mingw with -Wall -Werror, I'm going to fail because all these
>> (now removed) casts in the cwrapper source code were there specifically
>> to suppress warnings...
>
> And speaking of casts and C/C++...suppose you have package "foo" and you
> want to build "foo" with CC=g++ -- that ought to be legal, right?
>
> /foo-src/configure --prefix=... CC=g++
>
> But that means on cygwin/mingw, if foo uses libtool then the cwrapper
> will be built using ${CC} -- that is, g++. Bang; you're dead -- because
> the cast is required with C++, isn't it? (IIRC it's not just a warning,
> it's an error, if the cast is missing).
You're both quite correct. I retract this patch from the series.
I'll probably submit another similar (non-cast removing) patch as I
try to adopt more gnulib modules into libtool to replace our bespoke
code, particularly lt__alloc.[ch], as I try to make libtool more GNU-like
for the benefit of future maintainers and contributors.
Cheers,
--
Gary V. Vaughan (gary AT gnu DOT org)
Re: [PATCH 04/25] syntax-check: fix violations and re-enable sc_cast_of_x_alloc_return_value.
On 11/15/2011 11:36 AM, Charles Wilson wrote:
> On 11/15/2011 7:53 AM, Gary V. Vaughan wrote:
>> * cfg.mk (local-checks-to-fix): Remove
>> sc_cast_of_x_alloc_return_value from list of disabled checks.
>> * libltdl/config/ltmain.m4sh (XMALLOC, XFREE): Unroll into their
>> xmalloc and free expansions so that this syntax-check can find
>> violations, and then fix them.
>> * iibltdl/libltdl/lt__alloc.h (MALLOC, REALLOC): Renamed to
>> xmalloc and xrealloc so that this syntax-check can find
>> violations. Adjust all callers.
>> (FREE, MEMREASSIGN): Removed. All callers unrolled into their
>> former expansions, and violations of this syntax-check fixed.
>> * libltdl/loaders/loadlibrary.c (LOCALFREE): Unrolled for
>> consistency.
>
> Why do I get the feeling that the next time I try to build any .exe on
> cygwin/mingw with -Wall -Werror, I'm going to fail because all these
> (now removed) casts in the cwrapper source code were there specifically
> to suppress warnings...
And speaking of casts and C/C++...suppose you have package "foo" and you
want to build "foo" with CC=g++ -- that ought to be legal, right?
/foo-src/configure --prefix=... CC=g++
But that means on cygwin/mingw, if foo uses libtool then the cwrapper
will be built using ${CC} -- that is, g++. Bang; you're dead -- because
the cast is required with C++, isn't it? (IIRC it's not just a warning,
it's an error, if the cast is missing).
--
Chuck
Re: [PATCH 04/25] syntax-check: fix violations and re-enable sc_cast_of_x_alloc_return_value.
On 11/15/2011 7:53 AM, Gary V. Vaughan wrote: > * cfg.mk (local-checks-to-fix): Remove > sc_cast_of_x_alloc_return_value from list of disabled checks. > * libltdl/config/ltmain.m4sh (XMALLOC, XFREE): Unroll into their > xmalloc and free expansions so that this syntax-check can find > violations, and then fix them. > * iibltdl/libltdl/lt__alloc.h (MALLOC, REALLOC): Renamed to > xmalloc and xrealloc so that this syntax-check can find > violations. Adjust all callers. > (FREE, MEMREASSIGN): Removed. All callers unrolled into their > former expansions, and violations of this syntax-check fixed. > * libltdl/loaders/loadlibrary.c (LOCALFREE): Unrolled for > consistency. Why do I get the feeling that the next time I try to build any .exe on cygwin/mingw with -Wall -Werror, I'm going to fail because all these (now removed) casts in the cwrapper source code were there specifically to suppress warnings... -- Chuck
Re: [PATCH 04/25] syntax-check: fix violations and re-enable sc_cast_of_x_alloc_return_value.
On 11/15/2011 05:53 AM, Gary V. Vaughan wrote: > * cfg.mk (local-checks-to-fix): Remove > sc_cast_of_x_alloc_return_value from list of disabled checks. That check is only useful for pure C projects. If the intention is that libtool can be compiled under both C and C++, then C++ requires that you cast the result of allocation functions (only C lets you get away with going from void* to any other pointer without cast). Are we sure that no one tries to compile libtool with a C++ compiler? -- Eric Blake ebl...@redhat.com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[PATCH 04/25] syntax-check: fix violations and re-enable sc_cast_of_x_alloc_return_value.
* cfg.mk (local-checks-to-fix): Remove
sc_cast_of_x_alloc_return_value from list of disabled checks.
* libltdl/config/ltmain.m4sh (XMALLOC, XFREE): Unroll into their
xmalloc and free expansions so that this syntax-check can find
violations, and then fix them.
* iibltdl/libltdl/lt__alloc.h (MALLOC, REALLOC): Renamed to
xmalloc and xrealloc so that this syntax-check can find
violations. Adjust all callers.
(FREE, MEMREASSIGN): Removed. All callers unrolled into their
former expansions, and violations of this syntax-check fixed.
* libltdl/loaders/loadlibrary.c (LOCALFREE): Unrolled for
consistency.
Signed-off-by: Gary V. Vaughan
---
build-aux/ltmain.m4sh | 64 +++-
cfg.mk|2 -
libltdl/libltdl/lt__alloc.h |9 +--
libltdl/loaders/dld_link.c|4 +-
libltdl/loaders/loadlibrary.c |6 +-
libltdl/loaders/preopen.c |3 +-
libltdl/lt_error.c|2 +-
libltdl/ltdl.c| 174 ++--
8 files changed, 131 insertions(+), 133 deletions(-)
diff --git a/build-aux/ltmain.m4sh b/build-aux/ltmain.m4sh
index 5266b01..2ba866b 100644
--- a/build-aux/ltmain.m4sh
+++ b/build-aux/ltmain.m4sh
@@ -3515,11 +3515,6 @@ int setenv (const char *, const char *, int);
# define _O_BINARY 0
#endif
-#define XMALLOC(type, num) ((type *) xmalloc ((num) * sizeof(type)))
-#define XFREE(stale) do { \
- if (stale) { free (stale); stale = 0; } \
-} while (0)
-
#if defined(LT_DEBUGWRAPPER)
static int lt_debug = 1;
#else
@@ -3611,7 +3606,7 @@ main (int argc, char *argv[])
int i;
program_name = (char *) xstrdup (base_name (argv[0]));
- newargz = XMALLOC (char *, argc + 1);
+ newargz = xmalloc (sizeof (*newargz) * (argc + 1));
/* very simple arg parsing; don't want to rely on getopt
* also, copy all non cwrapper options to newargz, except
@@ -3679,7 +3674,7 @@ EOF
lt_debugprintf (__FILE__, __LINE__,
"(main) found exe (after symlink chase) at: %s\n",
actual_cwrapper_path);
- XFREE (tmp_pathspec);
+ free (tmp_pathspec);
actual_cwrapper_name = xstrdup (base_name (actual_cwrapper_path));
strendzap (actual_cwrapper_path, actual_cwrapper_name);
@@ -3687,16 +3682,14 @@ EOF
/* wrapper name transforms */
strendzap (actual_cwrapper_name, ".exe");
tmp_pathspec = lt_extend_str (actual_cwrapper_name, ".exe", 1);
- XFREE (actual_cwrapper_name);
- actual_cwrapper_name = tmp_pathspec;
+ actual_cwrapper_name = (free (actual_cwrapper_name), tmp_pathspec);
tmp_pathspec = 0;
/* target_name transforms -- use actual target program name; might have lt-
prefix */
target_name = xstrdup (base_name (TARGET_PROGRAM_NAME));
strendzap (target_name, ".exe");
tmp_pathspec = lt_extend_str (target_name, ".exe", 1);
- XFREE (target_name);
- target_name = tmp_pathspec;
+ target_name = (free (target_name), tmp_pathspec);
tmp_pathspec = 0;
lt_debugprintf (__FILE__, __LINE__,
@@ -3705,9 +3698,8 @@ EOF
EOF
cat <
-#define LOCALFREE(mem) LT_STMT_START { \
- if (mem) { LocalFree ((void *)mem); mem = NULL; }} LT_STMT_END
#define LOADLIB__SETERROR(errmsg) LT__SETERRORSTR (loadlibraryerror (errmsg))
#define LOADLIB_SETERROR(errcode) LOADLIB__SETERROR (LT__STRERROR (errcode))
@@ -123,7 +121,7 @@ static int
vl_exit (lt_user_data LT__UNUSED loader_data)
{
vtable = NULL;
- LOCALFREE (error_message);
+ error_message = (LocalFree (error_message), NULL)
return 0;
}
@@ -285,7 +283,7 @@ static const char *
loadlibraryerror (const char *default_errmsg)
{
size_t len;
- LOCALFREE (error_message);
+ error_message = (LocalFree (error_message), NULL)
FormatMessageA (FORMAT_MESSAGE_ALLOCATE_BUFFER |
FORMAT_MESSAGE_FROM_SYSTEM |
diff --git a/libltdl/loaders/preopen.c b/libltdl/loaders/preopen.c
index 7149287..48ad51c 100644
--- a/libltdl/loaders/preopen.c
+++ b/libltdl/loaders/preopen.c
@@ -243,8 +243,7 @@ free_symlists (void)
while (lists)
{
symlist_chain *next = lists->next;
- FREE (lists);
- lists = next;
+ lists = (free (lists), next);
}
preloaded_symlists = 0;
diff --git a/libltdl/lt_error.c b/libltdl/lt_error.c
index d7af36d..79213bb 100644
--- a/libltdl/lt_error.c
+++ b/libltdl/lt_error.c
@@ -52,7 +52,7 @@ lt_dladderror (const char *diagnostic)
assert (diagnostic);
errindex = errorcount - LT_ERROR_MAX;
- temp = REALLOC (const char *, user_error_strings, 1 + errindex);
+ temp = xrealloc (user_error_strings, 1 + errindex);
if (temp)
{
user_error_strings = temp;
diff --git a/libltdl/ltdl.c b/libltdl/ltdl.c
index 01853e0..448476e 100644
--- a/libltdl/ltdl.c
+++ b/libltdl/ltdl.c
@@ -339,7 +339,7 @@ lt_dlexit (void)
if ((vtable = lt_dlloader_remove ((char *) vtable->name)))
{
- FREE (vtable);
+ free (vtable);
}
