Hi Collin,
> When compiled with the following attribute, the warning is silenced:
>
> __attribute__ ((__access__ (__none__, 1)))
> static inline void *
> ptr_align (void const *ptr, size_t alignment)
> {
> char const *p0 = ptr;
> char const *p1 = p0 + alignment - 1;
> return (void *) (p1 - (size_t) p1 % alignment);
> }
Then it makes sense to support it in Gnulib.
> While it would have been nice to provide a single ATTRIBUTE_ACCESS
> macro, I decided to make them separate. This is because GCC 10.1.0 added
> the read_only, read_write, and write_only options [3], and GCC 11.1.0
> added the additional none option. I didn’t like the idea of dropping the
> entire attribute for GCC 10.x, just because it doesn’t support one of
> the access type arguments.
The way you are writing this, it sounds like the desire for perfection
won over the desire for simplicity. But look: GCC 10 is 6 years old. It is
carried by distributions such as
Debian 11, Ubuntu 20.10, Fedora 32, openSUSE 15.2, NetBSD 10.
When it comes to avoiding compilation errors, we need to consider this
compiler version, of course. But for warnings? Most package maintainers
use a distro that is not older than 3 years. Even RHEL 9 comes with GCC 11.
So, I wouldn't make this an argument against the single ATTRIBUTE_ACCESS
macro.
But for the user, your new 4 macros are easier to use than a single
ATTRIBUTE_ACCESS macro; therefore I would stick with them.
> I attached a proposed patch.
> @@ -357,6 +358,33 @@ AC_DEFUN([gl_COMMON_BODY]
> */
> ]dnl There is no _GL_ATTRIBUTE_ALIGNED; use stdalign's alignas instead.
> [
> +/* _GL_ATTRIBUTE_ACCESS_READ_ONLY ((N, S)) declares that the argument N of
> + optional size S is used to read the referenced object but not write to it.
> + _GL_ATTRIBUTE_ACCESS_WRITE_ONLY ((N, S)) declares that the argument N of
> + optional size S is used to write the referenced object but not read from
> it.
Alphabetically, _GL_ATTRIBUTE_ACCESS_* comes before _GL_ATTRIBUTE_ALIGNED.
Therefore I would insert the new code before the 2 lines regarding
_GL_ATTRIBUTE_ALIGNED.
> +# define _GL_ATTRIBUTE_ACCESS_READ_ONLY(args) __attribute__ ((__access__
> (__read_only__, args)))
> +# define _GL_ATTRIBUTE_ACCESS_WRITE_ONLY(args) __attribute__ ((__access__
> (__write_only__, args)))
> +# define _GL_ATTRIBUTE_ACCESS_READ_WRITE(args) __attribute__ ((__access__
> (__read_write__, args)))
> +# if _GL_GNUC_PREREQ (11, 1)
> +# define _GL_ATTRIBUTE_ACCESS_NONE(args) __attribute__ ((__access__
> (__none__, args)))>
Can you try to keep line lengths at <= 80 columns?
Regarding the comments: Is the "optional size S" measured in bytes or in pointer
units? I.e. can the attribute be used e.g. on the wmemcpy function, whose
argument designates wchar_t elements, not bytes? The GCC docs don't state it,
therefore it would be good to state it here.
Still regarding the comments: Add a few sentences that compare the access
attribute and the nonnull_if_nonzero attribute. Which relation do they have?
Is one stronger than the other? Does it make sense to use both at the same
place?
Thanks for the good work!
Bruno
