This is an archive of the discontinued LLVM Phabricator instance.

Please don't land libc++ patches unless approved by the libc++ team. I won't revert, but this code will be removed in the future unless we find someone who is willing to support the musl configuration in libc++. Our policy is to not accept patches where nobody is willing to provide a CI bot, which is the case here currently.

In D141208#4034019, @philnik wrote:

In D141208#4034013, @sylvestre.ledru wrote:

In D141208#4033997, @brad wrote:

Sorry about that.

no worries ;)

Please don't land libc++ patches unless approved by the libc++ team. I won't revert, but this code will be removed in the future unless we find someone who is willing to support the musl configuration in libc++. Our policy is to not accept patches where nobody is willing to provide a CI bot, which is the case here currently.

Could you show me where that policy is written? I wasn't aware of it.

This issue wasn't raised in D124227 either.

Our policy is to not accept patches where nobody is willing to provide a CI bot, which is the case here currently.

That "policy" is really against the principles of open source. Corporations have money to maintain CI bots. Small open source projects can't afford the huge cost of running CI for libc++.

@philnik Sorry but we don't really have a way to know who is the "libc++ team". I don't know if @brad is part of it or not

provide a CI bot, which is the case here currently.

I am running it for https://apt.llvm.org/ and happy to open bugs in case of regression

In D141208#4035450, @sylvestre.ledru wrote:

@philnik Sorry but we don't really have a way to know who is the "libc++ team". I don't know if @brad is part of it or not

provide a CI bot, which is the case here currently.

I am running it for https://apt.llvm.org/ and happy to open bugs in case of regression

They are the people who are part of the #libc review group, which gets automatically added as a blocking reviewer on libc++ patches.

In D141208#4035239, @thesamesam wrote:

In D141208#4034019, @philnik wrote:

In D141208#4034013, @sylvestre.ledru wrote:

In D141208#4033997, @brad wrote:

Sorry about that.

no worries ;)

Please don't land libc++ patches unless approved by the libc++ team. I won't revert, but this code will be removed in the future unless we find someone who is willing to support the musl configuration in libc++. Our policy is to not accept patches where nobody is willing to provide a CI bot, which is the case here currently.

Could you show me where that policy is written? I wasn't aware of it.

I'm not sure it's written down explicitly anywhere, but we should definitely update our documentation. I guess part of the problem is that we still have multiple configurations in our code base which we have neither removed nor got a CI bot.

This issue wasn't raised in D124227 either.

Yeah, I' not sure why.

In D141208#4035242, @mgorny wrote:

Our policy is to not accept patches where nobody is willing to provide a CI bot, which is the case here currently.

That "policy" is really against the principles of open source. Corporations have money to maintain CI bots. Small open source projects can't afford the huge cost of running CI for libc++.

I'm not sure that this is against the principles of open source, though I don't know what these principles are in your opinion. Yes, this makes it a lot harder for small open source projects, but it's a lot worse to get aware of an issue literally half a year after it has been introduced, while it could have quite easily been detected before the patch has landed.

In D141208#4035242, @mgorny wrote:

Our policy is to not accept patches where nobody is willing to provide a CI bot, which is the case here currently.

That "policy" is really against the principles of open source. Corporations have money to maintain CI bots. Small open source projects can't afford the huge cost of running CI for libc++.

I'm not aware we (libc++) have this policy. Per https://libcxx.llvm.org/

If a platform or compiler is not listed here, it is not officially supported. It may happen to work, and in practice the library is known to work on some platforms not listed here, but we don’t make any guarantees. If you would like your compiler and/or platform to be formally supported and listed here, please work with the libc++ team to set up testing for your configuration.

So without a proper CI we can't officially support a configuration, so we love to have a CI. Some changes are heavily platform dependent which we can't test without a CI. That said, personally I've no objection at all for patches the improve support for the musl libc.

In D141208#4035450, @sylvestre.ledru wrote:

@philnik Sorry but we don't really have a way to know who is the "libc++ team". I don't know if @brad is part of it or not

The libc++ team approves the blocked libc++ reviewer. I know requiring libc++ to approve can sound a bit pedantic, but in the past we had people approving ABI breaking changes.

The policy for contributing to libc++ is documented here: https://libcxx.llvm.org/Contributing.html#the-review-process. Please read it and make sure that you follow it, it ensures that everything runs smoothly for everyone.

We have automation that adds the review group and marks it as blocking automatically in Phabricator. We can't do anything more explicit than that without moving to e.g. GitHub where we could actually remove direct push access and require that all changes go through PRs. In the meantime, we're relying on people following the process in good faith.

Thanks for the patch, this was a subtle one!

@ldionne Should we backport this to 15.0.7 ?

In D141208#4041990, @tstellar wrote:

@ldionne Should we backport this to 15.0.7 ?

I missed that comment — Phabricator doesn’t seem to have a way to make direct mentions stand out from other libc++ review group emails.

Anyway, yes I think it would make sense to cherry-pick but I suspect that isn’t relevant anymore. Sorry about that.

cschlosser mentioned this in D148056: libcxx: Bring back unsigned return from wcstoull_l.Apr 11 2023, 2:47 PM

Revision Contents

Path

Size

libcxx/

include/

__support/

musl/

xlocale.h

2 lines

Diff 487154

libcxx/include/__support/musl/xlocale.h

Show All 33 Lines	strtoull_l(const char __nptr, char *__endptr, int __base, locale_t) {
return ::strtoull(__nptr, __endptr, __base);		return ::strtoull(__nptr, __endptr, __base);
}		}

inline _LIBCPP_HIDE_FROM_ABI long long		inline _LIBCPP_HIDE_FROM_ABI long long
wcstoll_l(const wchar_t __nptr, wchar_t *__endptr, int __base, locale_t) {		wcstoll_l(const wchar_t __nptr, wchar_t *__endptr, int __base, locale_t) {
return ::wcstoll(__nptr, __endptr, __base);		return ::wcstoll(__nptr, __endptr, __base);
}		}

inline _LIBCPP_HIDE_FROM_ABI long long		inline _LIBCPP_HIDE_FROM_ABI unsigned long long
wcstoull_l(const wchar_t __nptr, wchar_t *__endptr, int __base, locale_t) {		wcstoull_l(const wchar_t __nptr, wchar_t *__endptr, int __base, locale_t) {
return ::wcstoull(__nptr, __endptr, __base);		return ::wcstoull(__nptr, __endptr, __base);
}		}

inline _LIBCPP_HIDE_FROM_ABI long double		inline _LIBCPP_HIDE_FROM_ABI long double
wcstold_l(const wchar_t __nptr, wchar_t *__endptr, locale_t) {		wcstold_l(const wchar_t __nptr, wchar_t *__endptr, locale_t) {
return ::wcstold(__nptr, __endptr);		return ::wcstold(__nptr, __endptr);
}		}

#ifdef __cplusplus		#ifdef __cplusplus
}		}
#endif		#endif

#endif // _LIBCPP___SUPPORT_MUSL_XLOCALE_H		#endif // _LIBCPP___SUPPORT_MUSL_XLOCALE_H

This is an archive of the discontinued LLVM Phabricator instance.

libc++: bring back the unsigned in the return type in wcstoull_lClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 487154

libcxx/include/__support/musl/xlocale.h

libc++: bring back the unsigned in the return type in wcstoull_l
ClosedPublic