apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Saxon" <sa...@saxon.cx>
Subject Re: Borland C Builder 5
Date Thu, 28 Mar 2002 01:29:20 GMT
Hi,

----- Original Message -----
From: "Branko ─îibej" <brane@xbc.nu>
Sent: Thursday, March 28, 2002 3:49 AM


> I think it would be extremeny useful to be abble to compile APR with BC5
> (iirc it can be downloaded for free?).

I think the free BC5 download is just the command-line compiler, not the
whole IDE (which I have).

> What about the project files? Did
> you build your own, or convert the MSVC ones?

I built my own. I created a new project, added all the files to it which are
in the MSVC project, then added some include dirs and defines, and it went
from there.

> >1.
> >
> >In file_io\win32\open.c, there's this function:
> >
> >APR_DECLARE(apr_status_t) apr_os_file_put(apr_file_t **file,
> >                                          apr_os_file_t *thefile,
> >                                          apr_int32_t flags,
> >                                          apr_pool_t *pool)
> >{
> >    (*file) = apr_pcalloc(pool, sizeof(apr_file_t));
> >    (*file)->pool = pool;
> >    (*file)->filehand = *thefile;
> >    (*file)->ungetchar = -1; /* no char avail */
> >    (*file)->flags;
> >    return APR_SUCCESS;
> >}
> >
> >On this line:
> >
> >    (*file)->flags;
> >
> >(Line 521), I get a 'Code has no effect' warning. I presume this should
> >actually be:
> >
> >    (*file)->flags = flags;
> >
> This seems to be a bug, yes. Can you provide a patch?

Sure, here it is:

====================================================================
--- file_io/win32/open.c.old    Wed Mar 27 17:06:33 2002
+++ file_io/win32/open.c        Thu Mar 28 09:06:02 2002
@@ -177,7 +177,6 @@
         apr_wchar_t *wpre, *wfile, *ch;
         apr_size_t n = strlen(file) + 1;
         apr_size_t r, d;
-        apr_status_t rv;

         if (apr_os_level >= APR_WIN_2000) {
             if (global)
@@ -201,7 +200,7 @@
         wfile = apr_palloc(pool, (r + n) * sizeof(apr_wchar_t));
         wcscpy(wfile, wpre);
         d = n;
-        if (rv = apr_conv_utf8_to_ucs2(file, &n, wfile + r, &d)) {
+        if (apr_conv_utf8_to_ucs2(file, &n, wfile + r, &d)) {
             return NULL;
         }
         for (ch = wfile + r; *ch; ++ch) {
@@ -518,7 +517,7 @@
     (*file)->pool = pool;
     (*file)->filehand = *thefile;
     (*file)->ungetchar = -1; /* no char avail */
-    (*file)->flags;
+    (*file)->flags = flags;
     return APR_SUCCESS;
 }

====================================================================

This fixes this bug, and also the 'rv assigned a value but not used'
mentioned later.

> >2.
> >
> >I get a whole lot of warnings like this:
> >
> >signal.h(67): W8017 Redefinition of 'SIGUSR1' is not identical
> >signal.h(68): W8017 Redefinition of 'SIGUSR2' is not identical
> >
> >The signal.h in the warning is Borland's library signal.h. The reason it
> >occurs is that MSVC's signal.h has this:
> >
> >#define SIGINT          2
> >#define SIGILL          4
> >#define SIGFPE          8
> >#define SIGSEGV         11
> >#define SIGTERM         15
> >#define SIGBREAK        21
> >#define SIGABRT         22
> >
> >While Borland's signal.h has this:
> >
> >#define SIGINT           2
> >#define SIGILL           4
> >#define SIGFPE           8
> >#define SIGSEGV         11
> >#define SIGTERM         15
> >#define SIGUSR1         16
> >#define SIGUSR2         17
> >#define SIGUSR3         20
> >#define SIGBREAK        21
> >#define SIGABRT         22
> >
> >So, the problem is that include\arch\win32\apr_private.h sets its own
values
> >for SIGUSR1 and SIGUSR2, then when <signal.h> is included afterwards, it
> >produces a conflict under Borland. Besides the ones defined in MSVC's
> >signal.h, I think the signals in apr_private.h are arbitrary? So this
> >problem could be fixed by re-ordering the specified signals so that
SIGUSR1
> >and SIGUSR2 have the same values as Borland, and then putting #ifndef
> >__BORLANDC__ around them to stop their definition in Borland. So,
something
> >like this:
> >
> I think it would be better to #ifdef on the actual symbol we want to
> define, not on the compiler type. It might also be a good idea to change
> the values used by APR right now; I think Borland's values are the
> correct ones.

Ok, sure. How would I go about doing the #ifdef on the symbol? I can't do
this:

#ifndef SIGUSR1
#define SIGUSR1    16
#endif

etc because this is done before the compiler's signal.h is included, so
SIGUSR1 will always be undefined. Unless I had apr_private.h #include
<signal.h> itself, first?

> >8.
> >
> >In network_io\win32\sockets.c, at the top of apr_socket_create(), on line
> >123, ie this:
> >
> >    int downgrade = (family == AF_UNSPEC);
> >
> >I get a warning that downgrade is assigned a value which is never used.
This
> >*is* actually used in the function, but inside a #if APR_HAVE_IPV6 /
#endif
> >(and APR_HAVE_IPV6 is 0 in apr.hw). So, I imagine that the definition of
> >downgrade should also be wrapped in a #if APR_HAVE_IPV6 / #endif.

Should I submit a patch for this one?

> >9.
> >
> >In mmap\win32\mmap.c, in mmap_cleanup(), I get a warning that rv is
assigned
> >a value that is never used. This is referring to the rv on line 69, ie
this:
> >
> >    apr_status_t rv = 0;
> >
> >Which isn't necessary because the cases which assign something to rv
> >redefine it in their own compound block.
> >
> Well, in this case it's a question of being on the safe side.

I'm not sure what you mean?

The function goes roughly:

static apr_status_t mmap_cleanup(void *themmap)
{
    apr_status_t rv = 0;

    if (something) {
        if (!some_func(something)) {
            apr_status_t rv = apr_get_os_error();
            ...
            return rv;
        }
    }
    if (something_else) {
        if (!some_other_func(something_else)) {
            apr_status_t rv = apr_get_os_error();
            ...
            return rv;
        }
    }
    return APR_SUCCESS;
}

So in the 2 cases which return rv, they declare their own 'apr_status rv' in
their local scope, setting it to apr_get_os_error(), overriding the original
'apr_status_t rv = 0' at the top of the function. So that first rv is never
used?

> >10.
> >
> >I get a bunch (14) of 'Suspicious pointer conversion' warnings, which
seem
> >to be for things like passing a pointer to a signed int to a function
taking
> >a pointer to an unsigned int, or vice versa. Or, passing a pointer to an
int
> >to a function expecting a pointer to a short.
> >
> Those should be fixed. int<->unsigned is not a problem; int->short is
> ... well, on a LE machine it actually isn't ...

Ok sure, I'll look into each of these individually when I get some more
time.

> >13.
> >
> >I get a lot (31) of 'Possibly incorrect assignment' warnings, which is
for
> >things like this:
> >
> >if (x = some_function(y)) {
> >    etc...
> >}
> >
> Probably should have an extra pair of parenthesis around the condition.
> Again, patches are welcome ...

For Borland, to suppress the warning you need:

if ((x = some_function(y)) != 0) {
    etc...
}

If you don't think that's too much, I'll submit patches changing them all to
that.

> >====================================================================
> >--- include/apr.hw.old  Wed Mar 27 17:06:25 2002
> >+++ include/apr.hw      Wed Mar 27 19:10:21 2002
> >@@ -79,6 +79,7 @@
> > extern "C" {
> > #endif
> >
> >+#ifndef __BORLANDC__
> > /* disable or reduce the frequency of...
> >  *   C4057: indirection to slightly different base types
> >  *   C4075: slight indirection changes (unsigned short* vs short[])
> >@@ -89,6 +90,13 @@
> >  *   C4514: unreferenced inline function removed
> >  */
> > #pragma warning(disable: 4100 4127 4201 4514; once: 4057 4075 4244)
> >+#else
> >+/* disable warnings for Borland C */
> >+#pragma warn -sus /* suspicious pointer conversion */
> >+#pragma warn -pch /* cannot create pre-compiled header */
> >+#pragma warn -pia /* possibly incorrect assignment */
> >+#pragma warn -rch /* unreachable code */
> >
> I agree with -rch and -pch, perhaps -sus (although see my comment
> above), but not -pin, because it can be easily fixed.

Ok, here's a new patch with those two, to make it easier:

====================================================================
--- include/apr.hw.old  Wed Mar 27 17:06:25 2002
+++ include/apr.hw      Thu Mar 28 09:31:13 2002
@@ -79,6 +79,7 @@
 extern "C" {
 #endif

+#ifndef __BORLANDC__
 /* disable or reduce the frequency of...
  *   C4057: indirection to slightly different base types
  *   C4075: slight indirection changes (unsigned short* vs short[])
@@ -89,6 +90,11 @@
  *   C4514: unreferenced inline function removed
  */
 #pragma warning(disable: 4100 4127 4201 4514; once: 4057 4075 4244)
+#else
+/* disable warnings for Borland C */
+#pragma warn -pch /* cannot create pre-compiled header */
+#pragma warn -rch /* unreachable code */
+#endif

 #define APR_INLINE __inline
 #define APR_HAS_INLINE         1
====================================================================

cya,
Saxon Druce (saxon@blocksoftware.com)



Mime
View raw message