[libcxx-commits] [PATCH] D98097: [libc++] "Merged wording" for D98077 and D96986
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Mar 11 10:38:42 PST 2021
Quuxplusone added inline comments.
Comment at: libcxx/include/__config:1462
+#if defined(__GNUC__) || defined(__clang__)
+#define _LIBCPP_FORMAT_PRINTF(...) \
+ __attribute__((__format__(__printf__, __VA_ARGS__)))
> joerg wrote:
> > Quuxplusone wrote:
> > > Mordante wrote:
> > > > Wouldn't it be better use 2 named arguments instead of `...`. The latter isn't officially part of C++98 and the attribute requires 3 arguments.
> > > I had thought I was copying style from elsewhere in this file, but I guess only `_LIBCPP_DIAGNOSE_WARNING`/`_LIBCPP_DIAGNOSE_ERROR` actually use `...` for this purpose. So the only benefit I'm getting from `...` is that I don't have to worry about naming or whitespace conventions.
> > > How do we feel about
> > > ```
> > > #define _LIBCPP_FORMAT_PRINTF(a, b) \
> > > __attribute__((__format__(__printf__, a, b)))
> > > ```
> > > We already use `a, b` as macro parameter names on line 227. If this naming/whitespace looks acceptable, I don't mind making this change.
> > >
> > > FWIW, I claim C++98-portability is not a concern, given that we already use `...` specifically in C++98 mode, e.g. in `#define static_assert(...)` on line 898.
> > Macro arguments are limited to the expansion as scope and disappear afterwards. They don't interact with tokens outside the macro expansion. I don't think compatibility with C++98 is a concern here, but there is no reason to give up the checking on the number of arguments here. E.g. in NetBSD they are called `fmtarg` and `firstvararg` to make it self-documenting.
> You're right we already use `...` in C++98 mode. I prefer named arguments if the number required is fixed. I like the ones @joerg proposes. I had to read the manual to see what the syntax of the attribute was.
I strongly prefer //not// `fmtarg, firstvararg`, but I'll do `a, b`.
(Note that as with any attribute that names parameters by index, there are issues such as whether to count `this` and whether to index starting at 0 or 1. I also would have to look up the exact semantics, or better, cut-and-paste from somewhere that already got it right.)
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits