diff mbox

[1/9] posix: Sync glob with gnulib [BZ #1062]

Message ID c8e463b3-72e1-93b3-29b1-742da1a3eddf@cs.ucla.edu
State New, archived
Headers

Commit Message

Paul Eggert Sept. 6, 2017, 2:01 a.m. UTC 
  Adhemerval Zanella wrote:
> +/* Any distinct values will do here.
> +   Undef any existing macros out of the way.  */
> +#ifndef DT_UNKNOWN
> +# define DT_UNKNOWN 0
> +#endif
> +#ifndef DT_DIR
> +# define DT_DIR 1
> +#endif
> +#ifndef DT_LNK
> +# define DT_LNK 2
> +#endif

The comment here does not match the code, as nothing is being undeffed. Instead 
of this change, how about the following?

#if !defined _LIBC && !defined HAVE_STRUCT_DIRENT_D_TYPE
/* Any distinct values will do here.
    Undef any existing macros out of the way.  */
# undef DT_UNKNOWN
# undef DT_DIR
# undef DT_LNK
# define DT_UNKNOWN 0
# define DT_DIR 1
# define DT_LNK 2
#endif

This makes it more clear to the casual reader that this code is for use only 
outside glibc. Also, it's more robust for hopefully only-hypothetical non-glibc 
platforms that define some DT_ symbols but not others, because it guarantees 
their uniqueness in that case. I installed the attached patch into Gnulib, along 
these lines.

Comments

Adhemerval Zanella Netto Sept. 6, 2017, 12:52 p.m. UTC | #1 
On 05/09/2017 23:01, Paul Eggert wrote:
> Adhemerval Zanella wrote:
>> +/* Any distinct values will do here.
>> +   Undef any existing macros out of the way.  */
>> +#ifndef DT_UNKNOWN
>> +# define DT_UNKNOWN 0
>> +#endif
>> +#ifndef DT_DIR
>> +# define DT_DIR 1
>> +#endif
>> +#ifndef DT_LNK
>> +# define DT_LNK 2
>> +#endif
> 
> The comment here does not match the code, as nothing is being undeffed. Instead of this change, how about the following?

Indeed, I forgot to update the comments after the fix.

> #if !defined _LIBC && !defined HAVE_STRUCT_DIRENT_D_TYPE
> /* Any distinct values will do here.
>    Undef any existing macros out of the way.  */
> # undef DT_UNKNOWN
> # undef DT_DIR
> # undef DT_LNK
> # define DT_UNKNOWN 0
> # define DT_DIR 1
> # define DT_LNK 2
> #endif
> 
> This makes it more clear to the casual reader that this code is for use only outside glibc. Also, it's more robust for hopefully only-hypothetical non-glibc platforms that define some DT_ symbols but not others, because it guarantees their uniqueness in that case. I installed the attached patch into Gnulib, along these lines.

I do not have a strong preference here, I am ok with your approach and I will
update my patch with this change. In fact I would prefer that all this 
boilerplate that is non required for glibc code to be on a specific header
(maybe glob-gnulib.h or something), but we can think about it later.
diff mbox

Patch

From e73addd2704d4eb68b126210f5b41b428d5d4956 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 5 Sep 2017 18:58:50 -0700
Subject: [PATCH] glob: fix for use in glibc

Problem reported by Adhemerval Zanella in:
https://sourceware.org/ml/libc-alpha/2017-09/msg00213.html
* lib/glob.c (DT_UNKNOWN, DT_DIR, DT_LINK):
Do not redefine if _LIBC.
---
 ChangeLog  | 8 ++++++++
 lib/glob.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 351495b..61e3e8c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@ 
+2017-09-05  Paul Eggert  <eggert@cs.ucla.edu>
+
+	glob: fix for use in glibc
+	Problem reported by Adhemerval Zanella in:
+	https://sourceware.org/ml/libc-alpha/2017-09/msg00213.html
+	* lib/glob.c (DT_UNKNOWN, DT_DIR, DT_LINK):
+	Do not redefine if _LIBC.
+
 2017-09-02  Paul Eggert  <eggert@cs.ucla.edu>
 
 	glob: fix bugs with long login names
diff --git a/lib/glob.c b/lib/glob.c
index 8eb2b97..ddab535 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -80,7 +80,7 @@  static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
 
 typedef uint_fast8_t dirent_type;
 
-#ifndef HAVE_STRUCT_DIRENT_D_TYPE
+#if !defined _LIBC && !defined HAVE_STRUCT_DIRENT_D_TYPE
 /* Any distinct values will do here.
    Undef any existing macros out of the way.  */
 # undef DT_UNKNOWN
-- 
2.7.4