[PATCH] diff: "S_IFREG | 0644" to "(S_IFREG | 0644)" to avoid warning

Wrap "S_IFREG | 0644" in parentheses to avoid a "suggest parentheses
around arithmetic in operand of |" warning from GCC 4.1.3 on NetBSD
5.0.2.

I spotted and fixed this independently on NetBSD, but later found that
there was a NetBSD Problem Report that included this fix.

NetBSD-PR: http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168
Reported-by: Dan McMahill <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/diff.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index a43d326..e8b7e09 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c

@@ -93,7 +93,7 @@ static int builtin_diff_blobs(struct rev_info *revs,
 			      int argc, const char **argv,
 			      struct blobinfo *blob)
 {
-	unsigned mode = canon_mode(S_IFREG | 0644);
+	unsigned mode = canon_mode((S_IFREG | 0644));
 
 	if (argc > 1)
 		usage(builtin_diff_usage);

-- 
1.7.3.159.g610493

Ævar Arnfjörð Bjarmason wrote:

-	unsigned mode = canon_mode(S_IFREG | 0644);
+	unsigned mode = canon_mode((S_IFREG | 0644));

Just curious:

#define canon_mode(mode) \
        (S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
        S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)

#define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)

Since S_ISREG et al are macros, typically they would put their
argument in parentheses in the definition.  How are they defined
in NetBSD sys/stat.h?  What is canon_mode(S_IFREG | 0644) being
misinterpreted to mean?

On Mon, Oct 4, 2010 at 09:21, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

Wrap "S_IFREG | 0644" in parentheses to avoid a "suggest parentheses
around arithmetic in operand of |" warning from GCC 4.1.3 on NetBSD
5.0.2.

I spotted and fixed this independently on NetBSD, but later found that
there was a NetBSD Problem Report that included this fix.

With this and Jonathan's xdiff patch git compiles without warnings on
NetBSD, aside from this:

    imap-send.c: In function 'ssl_socket_connect':
    imap-send.c:310: warning: assignment discards qualifiers from
pointer target type
    imap-send.c:312: warning: assignment discards qualifiers from
pointer target type

I don't see a sane way around that[1], since it appears the NetBSD
people have patched openssl's function definitions without bumping the
OpenSSL version number. Either that or OpenSSL itself changed from
const char* to char* to const char* again, I didn't investigate that.

But tests on NetBSD with /bin/sh still fail since we use cd -P, but we
have unapplied patches for that so I didn't pursue it:

    http://article.gmane.org/gmane.comp.version-control.git/136561/match=
    http://article.gmane.org/gmane.comp.version-control.git/136562/match=

1. We could check for __NetBSD__ and the NetBSD version, but it's not
   worthwhile for a single warning.

On Mon, Oct 4, 2010 at 09:35, Jonathan Nieder [off-list ref] wrote:

Ævar Arnfjörð Bjarmason wrote:

quoted

-     unsigned mode = canon_mode(S_IFREG | 0644);
+     unsigned mode = canon_mode((S_IFREG | 0644));

Just curious:

#define canon_mode(mode) \
       (S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
       S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)

#define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644)

Since S_ISREG et al are macros, typically they would put their
argument in parentheses in the definition.  How are they defined
in NetBSD sys/stat.h?  What is canon_mode(S_IFREG | 0644) being
misinterpreted to mean?

Oh it's a bug in NetBSD, sorry for not being explicit about that:

    $ grep S_ISREG /usr/include/sys/stat.h
    #define S_ISREG(m)      ((m & _S_IFMT) == _S_IFREG)     /* regular file */

    $ grep S_ISREG /usr/include/linux/stat.h
    #define S_ISREG(m)      (((m) & S_IFMT) == S_IFREG)

I.e. GCC sees `S_IFREG | 0644 & _S_IFMT' on NetBSD but `(S_IFREG |
0644) & _S_IFMT' on Linux.

Since bitwise AND (&) has precedence over bitwise OR it's probably a
logic error on NetBSD too, not just an annoying warning.

Ævar Arnfjörð Bjarmason wrote:

But tests on NetBSD with /bin/sh still fail since we use cd -P, but we
have unapplied patches for that so I didn't pursue it:

    http://article.gmane.org/gmane.comp.version-control.git/136561/match=
    http://article.gmane.org/gmane.comp.version-control.git/136562/match=

Aren't these v1.7.0-rc0~76^2 and v1.7.0-rc0~76^2^?

Here's a patch for the more important of the remaining problems.  I'm
just guessing here; untested, of course.

-- 8< --
Subject: tests: use pwd -P to simulate cd -P for portability

NetBSD supports pwd -P but not cd -P.  POSIX has required both for
a while, so this should not be an issue for most Unix-like platforms.

The test harness uses cd -P to ensure $PWD and $(pwd) agree;
cd $(pwd -P) should do that, too.

If pwd -P fails on some platform, with this patch, the test harness
will die with 'FATAL: Unexpected exit with code 1'.

Signed-off-by: Jonathan Nieder <redacted>
---
 t/test-lib.sh |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 830e5e7..184bf84 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -916,9 +916,10 @@ rm -fr "$test" || {
 }
 
 test_create_repo "$test"
-# Use -P to resolve symlinks in our working directory so that the cwd
-# in subprocesses like git equals our $PWD (for pathname comparisons).
-cd -P "$test" || exit 1
+cd "$test" || exit 1
+# Resolve symlinks in our working directory so that the cwd in
+# subprocesses like git equals our $PWD (for pathname comparisons).
+dir=$(pwd -P) && cd "$dir" || exit 1
 
 HOME=$(pwd)
 export HOME

-- 
1.7.2.3

Am 04.10.2010 11:21, schrieb Ævar Arnfjörð Bjarmason:

-	unsigned mode = canon_mode(S_IFREG | 0644);
+	unsigned mode = canon_mode((S_IFREG | 0644));

That doesn't look pretty.

How about something like the following instead?  It untangles the
?-:-chain in canon_mode and allows passing of an argument with side
effects.  All the S_ISxxx macros get a single variable as parameter.
Does it fix the issue on NetBSD?
---
 cache.h |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 2ef2fa3..3d5ed51 100644
--- a/cache.h
+++ b/cache.h

@@ -277,9 +277,16 @@ static inline int ce_to_dtype(const struct cache_entry *ce)
 	else
 		return DT_UNKNOWN;
 }
-#define canon_mode(mode) \
-	(S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
-	S_ISLNK(mode) ? S_IFLNK : S_ISDIR(mode) ? S_IFDIR : S_IFGITLINK)
+static inline unsigned int canon_mode(unsigned int mode)
+{
+	if (S_ISREG(mode))
+		return S_IFREG | ce_permissions(mode);
+	if (S_ISLNK(mode))
+		return S_IFLNK;
+	if (S_ISDIR(mode))
+		return S_IFDIR;
+	return S_IFGITLINK;
+}
 
 #define flexible_size(STRUCT,len) ((offsetof(struct STRUCT,name) + (len) + 8) & ~7)
 #define cache_entry_size(len) flexible_size(cache_entry,len)

-- 
1.7.3

Jonathan Nieder wrote:

The test harness uses cd -P to ensure $PWD and $(pwd) agree;

As Ævar noticed, this sentence as it stands doesn't make much sense.
The idea is rather to make $(pwd -L) and $(pwd -P) agree --- the
former is accessible through $PWD, the latter through getwd() and
/bin/pwd.

Write good cheer, host.


--------------------------------------------------------
This 
http://www.tomtop.com/black-remote-controller-charger2x-2800mah-battery-packs-for-wii_p11124.html?aid=z
Wii Charger  looks good n stylish. It lights up all blue which made me think
this is good. Get a 
http://www.tomtop.com/mini-bluetooth-keyboard-for-ps3-mac-os-android-pc-pda.html?aid=z
Wireless Keyboard  and mouse combo for your computer workstation and reduce
desktop clutter.

-----
The voice input and output is very good in this 
http://www.tomtop.com/black-ps3-wireless-bluetooth-headset-for-playstation-3.html?aid=z
Wireless PS3 Headset . It is compatible with all PS3 games.Buy from Reliable 
http://www.tomtop.com/google-android-7-notebook-3g-tablet-pc-umpc-wifi-mid-pda.html?aid=z
Google Android PC  apad Wholesalers.
-- 
View this message in context: http://git.661346.n2.nabble.com/PATCH-diff-S-IFREG-0644-to-S-IFREG-0644-to-avoid-warning-tp5598387p5598910.html
Sent from the git mailing list archive at Nabble.com.