icculus / physfs

A portable, flexible file i/o abstraction.
https://icculus.org/physfs/
zlib License
560 stars 98 forks source link

-Wint-in-bool-context warning #44

Closed sezero closed 2 years ago

sezero commented 2 years ago

From CI run logs:

In file included from /home/runner/work/physfs/physfs/src/physfs.c:12:
/home/runner/work/physfs/physfs/src/physfs.c: In function ‘openDirectory’:
/home/runner/work/physfs/physfs/src/physfs.c:929:40: warning: ?: using integer constants in boolean context [-Wint-in-bool-context]
  929 |     BAIL_IF(!retval, claimed ? errcode : PHYSFS_ERR_UNSUPPORTED, NULL);
      |                      ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/physfs/physfs/src/physfs_internal.h:273:44: note: in definition of macro ‘BAIL_IF’
  273 | #define BAIL_IF(c, e, r) do { if (c) { if (e) PHYSFS_setErrorCode(e); return r; } } while (0)
      |                                            ^

Is the following correct??

diff --git a/src/physfs.c b/src/physfs.c
index e7ceddd..568f1fc 100644
--- a/src/physfs.c
+++ b/src/physfs.c
@@ -921,12 +921,12 @@ static DirHandle *openDirectory(PHYSFS_Io *io, const char *d, int forWriting)
             retval = tryOpenDir(io, *i, d, forWriting, &claimed);
     } /* else */

-    errcode = currentErrorCode();
+    errcode = claimed ? currentErrorCode() : PHYSFS_ERR_UNSUPPORTED;

     if ((!retval) && (created_io))
         io->destroy(io);

-    BAIL_IF(!retval, claimed ? errcode : PHYSFS_ERR_UNSUPPORTED, NULL);
+    BAIL_IF(!retval, errcode, NULL);
     return retval;
 } /* openDirectory */
icculus commented 2 years ago

A more general fix would be:

diff --git a/src/physfs_internal.h b/src/physfs_internal.h
index ecf5f10..0cddd85 100644
--- a/src/physfs_internal.h
+++ b/src/physfs_internal.h
@@ -270,7 +270,7 @@ void __PHYSFS_sort(void *entries, size_t max,
     just to make it clear where the responsibility for the error state lays. */
 #define BAIL(e, r) do { if (e) PHYSFS_setErrorCode(e); return r; } while (0)
 #define BAIL_ERRPASS(r) do { return r; } while (0)
-#define BAIL_IF(c, e, r) do { if (c) { if (e) PHYSFS_setErrorCode(e); return r; } } while (0)
+#define BAIL_IF(c, e, r) do { if (c) { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (e); if (err != PHYSFS_ERR_OK) PHYSFS_setErrorCode(err); return r; } } while (0)
 #define BAIL_IF_ERRPASS(c, r) do { if (c) { return r; } } while (0)
 #define BAIL_MUTEX(e, m, r) do { if (e) PHYSFS_setErrorCode(e); __PHYSFS_platformReleaseMutex(m); return r; } while (0)
 #define BAIL_MUTEX_ERRPASS(m, r) do { __PHYSFS_platformReleaseMutex(m); return r; } while (0)

...but to all of them, since the double-reference to e in there risks macro side-effects:

diff --git a/src/physfs_internal.h b/src/physfs_internal.h
index ecf5f10..d3920fb 100644
--- a/src/physfs_internal.h
+++ b/src/physfs_internal.h
@@ -268,21 +268,21 @@ void __PHYSFS_sort(void *entries, size_t max,
 /* These get used all over for lessening code clutter. */
 /* "ERRPASS" means "something else just set the error state for us" and is
     just to make it clear where the responsibility for the error state lays. */
-#define BAIL(e, r) do { if (e) PHYSFS_setErrorCode(e); return r; } while (0)
+#define BAIL(e, r) do { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (e); if (err) PHYSFS_setErrorCode(err); return r; } while (0)
 #define BAIL_ERRPASS(r) do { return r; } while (0)
-#define BAIL_IF(c, e, r) do { if (c) { if (e) PHYSFS_setErrorCode(e); return r; } } while (0)
+#define BAIL_IF(c, e, r) do { if (c) { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (e); if (err != PHYSFS_ERR_OK) PHYSFS_setErrorCode(err); return r; } } while (0)
 #define BAIL_IF_ERRPASS(c, r) do { if (c) { return r; } } while (0)
-#define BAIL_MUTEX(e, m, r) do { if (e) PHYSFS_setErrorCode(e); __PHYSFS_platformReleaseMutex(m); return r; } while (0)
+#define BAIL_MUTEX(e, m, r) do { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (e); if (err) PHYSFS_setErrorCode(err); __PHYSFS_platformReleaseMutex(m); return r; } while (0)
 #define BAIL_MUTEX_ERRPASS(m, r) do { __PHYSFS_platformReleaseMutex(m); return r; } while (0)
-#define BAIL_IF_MUTEX(c, e, m, r) do { if (c) { if (e) PHYSFS_setErrorCode(e); __PHYSFS_platformReleaseMutex(m); return r; } } while (0)
+#define BAIL_IF_MUTEX(c, e, m, r) do { if (c) { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (e); if (err) PHYSFS_setErrorCode(err); __PHYSFS_platformReleaseMutex(m); return r; } } while (0)
 #define BAIL_IF_MUTEX_ERRPASS(c, m, r) do { if (c) { __PHYSFS_platformReleaseMutex(m); return r; } } while (0)
 #define GOTO(e, g) do { if (e) PHYSFS_setErrorCode(e); goto g; } while (0)
 #define GOTO_ERRPASS(g) do { goto g; } while (0)
-#define GOTO_IF(c, e, g) do { if (c) { if (e) PHYSFS_setErrorCode(e); goto g; } } while (0)
+#define GOTO_IF(c, e, g) do { if (c) { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (err); if (err) PHYSFS_setErrorCode(e); goto g; } } while (0)
 #define GOTO_IF_ERRPASS(c, g) do { if (c) { goto g; } } while (0)
-#define GOTO_MUTEX(e, m, g) do { if (e) PHYSFS_setErrorCode(e); __PHYSFS_platformReleaseMutex(m); goto g; } while (0)
+#define GOTO_MUTEX(e, m, g) do { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (e); if (err) PHYSFS_setErrorCode(err); __PHYSFS_platformReleaseMutex(m); goto g; } while (0)
 #define GOTO_MUTEX_ERRPASS(m, g) do { __PHYSFS_platformReleaseMutex(m); goto g; } while (0)
-#define GOTO_IF_MUTEX(c, e, m, g) do { if (c) { if (e) PHYSFS_setErrorCode(e); __PHYSFS_platformReleaseMutex(m); goto g; } } while (0)
+#define GOTO_IF_MUTEX(c, e, m, g) do { if (c) { const PHYSFS_ErrorCode err = (PHYSFS_ErrorCode) (e); if (err) PHYSFS_setErrorCode(err); __PHYSFS_platformReleaseMutex(m); goto g; } } while (0)
 #define GOTO_IF_MUTEX_ERRPASS(c, m, g) do { if (c) { __PHYSFS_platformReleaseMutex(m); goto g; } } while (0)

 #define __PHYSFS_ARRAYLEN(x) ( (sizeof (x)) / (sizeof (x[0])) )

I'll push this, if there aren't objections.

sezero commented 2 years ago

The macro params and temporary locals need uglifying or something: The following happens because of shadowing:

src/physfs_platform_posix.c: In function 'doOpen':
src/physfs_platform_posix.c:191: warning: unused variable 'err'
icculus commented 2 years ago

Ugh, let's just go with your patch, then. Go ahead and commit.

sezero commented 2 years ago

Replacing the local var from err to err_ handles things.

But if you still want my original patch, I can do that too.

icculus commented 2 years ago

Yeah, let's keep it simple for now. Just go with the original patch.

sezero commented 2 years ago

Done.