This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] hurd: add gscope support
- From: Joseph Myers <joseph at codesourcery dot com>
- To: Samuel Thibault <samuel dot thibault at ens-lyon dot org>
- Cc: <libc-alpha at sourceware dot org>
- Date: Thu, 1 Mar 2018 01:17:18 +0000
- Subject: Re: [PATCH] hurd: add gscope support
- Authentication-results: sourceware.org; auth=none
- References: <20180301005014.10220-1-samuel.thibault@ens-lyon.org>
On Thu, 1 Mar 2018, Samuel Thibault wrote:
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 114f77a8e5..be564306f2 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -188,6 +188,9 @@ int (*_dl_make_stack_executable_hook) (void **) = _dl_make_stack_executable;
> /* Function in libpthread to wait for termination of lookups. */
> void (*_dl_wait_lookup_done) (void);
>
> +#ifdef THREAD_GSCOPE_GLOBAL
> +int _dl_thread_gscope_count;
> +#endif
Current preferred practice is always-defined macros, tested with #if
rather than #ifdef.
There might be exceptions for a new macro that's part of an existing group
of closely-related macros using the #if convention. But this doesn't seem
to apply in this case, and the patch writeup has no explanation of why
#ifdef might be needed here.
I would hope that the new macro would have a comment somewhere (I suppose
on its default definition) explaining its semantics (and that this
variable would also have a comment documenting its semantics, as is
generally appropriate for any global variable or function or macro).
> +/* Temporary poor-man's global scope switch support */
Comments should end with ". ".
> +#define THREAD_GSCOPE_GLOBAL
> +#define THREAD_GSCOPE_SET_FLAG() \
> + atomic_increment(&GL(dl_thread_gscope_count))
> +#define THREAD_GSCOPE_RESET_FLAG() do { \
> + if (atomic_decrement_val(&GL(dl_thread_gscope_count)) == 0) \
> + lll_wake(&GL(dl_thread_gscope_count), 0); \
> +} while (0)
> +#define THREAD_GSCOPE_WAIT() do { \
> + int count; \
> + while ((count = GL(dl_thread_gscope_count))) { \
> + lll_wait(&GL(dl_thread_gscope_count), count, 0); \
> + } \
> +} while (0)
Missing spaces before '(' in the calls to atomic_increment,
atomic_decrement_val, lll_wake, lll_wait. (It's correct for the spaces to
be omitted in calls to GL, as that logically expands to a single
identifier.)
do {} while (0) should be written with the normal GNU Coding Standards
formatting, so
#define something
do
{
...;
}
while (0)
(with appropriate backslashes). (Other formatting in these macro
definitions seems to be off as well - two column indentation should be
used, '{' should be on its own line in the 'while' if the {} are needed
there at all, which I don't think they are.)
--
Joseph S. Myers
joseph@codesourcery.com