[libcxx-commits] [PATCH] D117811: [libc++] Remove _VSTD

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/format_arg.h:141
       _LIBCPP_AVAILABILITY_FORMAT friend __format_arg_store<_Ctx, _Args...>
-      _VSTD::make_format_args(const _Args&...);
+      std::_LIBCPP_ABI_NAMESPACE::make_format_args(const _Args&...);
 
----------------
Mordante wrote:
> Quuxplusone wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > Why is this required? Is this the `Add gcc workarounds`? This deviates from the previous assertion that there was no difference between using `std::` and `std::__1::`. The requirement to sometimes use `std::_LIBCPP_ABI_NAMESPACE` instead of `std::` feels error prone to me.
> > > > 
> > > > 
> > > Yes, these are the GCC workarounds. I think this is a GCC bug, but nobody has confirmed it yet, so I didn't open an issue. It seems GCC doesn't handle friends properly with inline namespaces.
> > I think it should work to just remove `_VSTD::` from this line, because `make_format_args` is already in the same namespace as `basic_format_arg`, isn't it? (And ADL applies to //calls//, not //friend declarations//.) Thus:
> > ```
> >   template <class _Ctx, class... _Args>
> >   _LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT
> >   friend __format_arg_store<_Ctx, _Args...> make_format_args(const _Args&...);
> > ```
> > (Drive-by fixed indentation.) If this way works, which I believe it should, I'd very much prefer to do it (unless @Mordante is planning some future change that will explain why we //don't// want to do this even though it works).
> > 
> > Ditto below.
> > 
> > Also, if this works, please land this should-be-NFC change (and below) in their own separate commit. Let's have changing-the-`#define _VSTD` be a one-line commit, so it's really really trivial to revert if needed.
> Please make sure it's a GCC bug and not a Clang bug, feel free to discuss it on Discord.
> I'm not happy when it's a GCC bug and we need to add `_LIBCPP_ABI_NAMESPACE`. This is error prone and only caught by the CI. (At least I expect most of us using Clang as compiler.)
It seems our messages crossed @Quuxplusone, I'll have a look at removing the `_VSTD` from the friend declarations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117811/new/

https://reviews.llvm.org/D117811