- User Since
- Jun 26 2014, 12:44 PM (418 w, 22 h)
Thank you for addressing my concerns.
Fri, Jun 24
Ah, I see we limit ourselves to valid identifiers. LGTM again, just with a knob to turn it off.
Actually, I think we need to investigate the mangling scheme when the provided ABI tag is a number, because the mangling is B<size-of-tag><tag>, so for example the ABI tag "abc" on foo produces "_Z3fooB3abcv", where 3 is the length of the tag. Now consider the tag 1, which produces B11v, which looks as though it's a tag of length 11.
This seems to break some demanglers, maybe it also will break more?
It's true that this is a strict improvement over the internal_I guess I'm fine with this if we give Google a way to turn it off, so we don't have to pay the cost of the larger manglings.
Tue, Jun 21
Frankly, I'm not sure that keeping logical operators together is "disentangling" them. They're pretty tightly grouped.
And I can't say understand the point or value of all this churn either.
LGTM once we get this clean.
Mon, Jun 20
I don't think this change is something we should do.
Mon, Jun 13
To clarify, I have no problem with the suggested style.
I should add...
OK, so i'm convinced this change should be OK assuming we continue to ensure _LIBCPP_HIDE_FROM_ABI is only ever applied to inline functions.
It would also need to be applied to every single inline function the library produces, otherwise a tagged function could call an untagged function,
since the untagged definition can be selected at random from the available definitions.
I still think it would be nice to produce a minimal test that actually ran (no need to check any output). Since having a runnable test would allow the sanitizers to pick up any weird bugs Cpp20HostileIterator causes at runtime.
I really don't know if this is worth the cost of losing all the blame history.
I know you can get around whitespace changes, but still.
It's worth noting that buildkite supports FreeBSD, so there shouldn't be any fundamental issues trying to add a builder once we have hardware.
Hmm, so I'm not sure this is the best idea. At worst, I think it'll introduce ODR bugs if we use it on anything other than inline functions , and when applied only to inline functions it helps only in a very very limited number of cases .
Sun, Jun 12
This LGTM, as a temporary fix; once we test it.
Is this missing module map changes?
Sat, Jun 11
Amazing tests. Unfortunately, this change causes breakages in std::lower_bound because ranges::distance is not a drop-in replacement for std::distance, and that breaks existing code as it switches between C++17 and C++20.
I'm working on minimizing a reproducer.
Fri, Jun 10
I remember the last time I tried to do this, it broke a bunch of things. I can't remember why -- but it did.
Thu, Jun 9
I'm pretty sure this change results in cases where, under ABI v2, C++14 and C++17 produce different ABI's. Am I mistaken?
Fri, Jun 3
May 26 2022
May 24 2022
You can safely ignore my comments below; I don't intend to block this change.
I'm not sure I understand what "from scratch" means given this patch, but I don't need to understand to see that to LGTM it. LGTM once the inline comments are addressed.
May 23 2022
Thanks for the fix!
May 21 2022
I think this changes necessary to fix libc++, assuming it actually uses this cmake infrastructure, which I'm not sure it does.
May 20 2022
Requesting the __core_is_convertible changes formally to put this review in the correct bucket.
GCC ships this as an extension. I can't find a reason why the extension would be non-conforming or dangerous, and it seems like a safer way for users to generate char values, rather than expecting the user to correctly pass the numerical_limits<char>::max() to the constructor.
I think we should reach out to someone at Microsoft (maybe STL), and see how open they are to shipping this as an extension. If they're willing then we avoid the portability trap that we're trying to protect users from.
I think we already have a trait for this. It's __is_core_convertible. Could you use that instead?
Apr 19 2022
Thanks. Any other sanitizers we can turn up/on?
Mar 25 2022
Other than a couple breakages caused by this not working in C++14/C++11, I haven't seen anything blow up at Google, though I've yet to get a full test run in.
That is to say, I think this is good to land once the other inline comments are addressed.
@rsmith had plans for this header. Richard, are we good to remove this?
Mar 18 2022
I would like to test this against Google's source. I'll try to get that done by early this week. Please do not commit this until then.
Tuple is so sensitive that changing whitespace breaks users.
Mar 16 2022
We discussed making the test look something like:
PS. This is removing an optimization where:
(A) the node allocations are reused during assignment (B) For reused nodes, the types assignment operator is called.
I don't think a custom benchmarking test is appropriate. Please rewrite your benchmarks using Google benchmark.
I still don't love that this patch codifies a way to disable warnings in tests. We don't want to do that, so it's fine that it's ugly to do.
But otherwise I'm OK with this.
Mar 14 2022
I've got a bunch of concerns about the memcpy magic happening here. How is it constexpr, how does it handle lifetimes, is it fully tested?
Mar 13 2022
Have @ldionnes changes to enable warnings in the test suite been submitted yet? Maybe that's why you're not seeing some of the warnings you removed.
Can you double check that (A) The headers aren't included with -isystem or -cxx-isystem and that _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER is defined
when you've tested this?
Every single warning we disable should come with a comment explaining why the warning isn't real and why we're not fixing it some other way. We shouldn't encourage suppressing warnings.
Mar 12 2022
The patch to the headers looks phenomenal. I only reviewed the code. I haven't looked at the spec yet. But the code was gorgeous. Well done.
Mar 10 2022
Do we have tests that fire up a bunch of threads and construct various locale::ids concurrently? If not we should add some. That will allow TSAN to diagnose bugs.
I made a few mistakes in my first set of comments. I better understand now. But I'm even more concerned.
I'll spend some time reading the design doc to see if that answers my quenstions.
The entire point of the <__threading_support> interface is that it allows vendors/system maintainers to provide their own implementation without needing to change libc++.
I don't see why this patch couldn't use that same mechanism?
- Make code compile (Spell the parameter name correctly)
We would very much be open to upstreaming a version of change in order to reduce the burden to both the project and Google. We are working diligently at the moment to remove the patch internally entirely. It's the primary focus of my work at the moment. So anything we upstream would be temporary on the order of 6 months to a year. Let me know what you think is appropriate here.
Mar 8 2022
Doesn't this change make all of the tests with functions decorated with TEST_CONSTEXPR_AFTER_CXX20 undefined behavior, since it's undefined to have constexpr on a function that is never constexpr?
Are we sure this doesn't break the ABI because changing user declared constructors into delete constructors can do that, Can't it?
Please don't change this. Google uses this hook to change the behavior of string_view internally. We're working to get rid of the patch, but this helper function was specifically put in place to allow for the patch.
Mar 3 2022
I also agree with @ldionne's comment about this being potentially ABI breaking because of how std::string is externally instantiated.
I'm not OK with this change given that the description indicates a lack of understanding of the current situation.
Mar 2 2022
Mar 1 2022
What happened to all the tests for operator!=?
We exercise judgement regarding this. We do try to split different functions and overloads into separate files, except when it provides little value.