- User Since
- Jun 26 2014, 12:44 PM (364 w, 4 d)
Fri, Jun 11
We're going to want some documentation about this option. Please add it here: https://libcxx.llvm.org/UsingLibcxx.html#id7
(The files that generate this are under libcxx/docs).
May 11 2021
Assuming this still merges, I don't have any issue with it.
May 5 2021
Removing the std::iterator inheritance is potentially ABI breaking. In particular in the case of reverse_iterator.
If the iterator provided to reverse_iterator also inherits from std::iterator, then the first member of reverse_iterator goes from having an offset of 4 to an offset of 0 bytes
(because the compiler couldn't place the same empty base at the same address).
Apr 30 2021
I'm working to see if I can fix forward the template depth and constexpr
issues, but I would appreciate some help from @ldionne as the original
In general this removes a bunch of carefully constructed SFINAE meant to improve compile times.
I'll look into getting that back.
Apr 29 2021
Has this been tested with various sanitizers on? Does it give warnings about weird object lifetimes?
Apr 19 2021
I would need to see analysis of the cost of always passing an additional parameter to these iterators constructors before I approve this.
Apr 7 2021
There's another bug with SFINAE depth increases that's blowing up at Google.
I suspect we can get that back.
Apr 2 2021
I like this patch. It's targeted and fixes the problem it's aiming at well.
Here is one regression I've found so far:
Apr 1 2021
I'm a bit worried about some of the tests that were deleted. Not the ones for extensions, but some of the ones added to prevent regressions with SFINAE.
Mar 18 2021
Can we please change the .compile.cpp to just .pass.cpp. There's very little to no cost to actually run the test, and doing so seems safer to me.
It avoids allowing runtime tests to be mistakenly added to these files without anybody noticing.
LGTM after addressing the current inline comments.
Please make sure the other reviewers don't have any unresolved comments before submitting.
Mar 11 2021
Due to some complexities libc++ has dealing with the allocator, as well some optimizations it would be a serious undertaking to restructure libc++ to properly construct the types in the hash containers,
This change would be greatly simpler and much appreciated.
LGTM. Nice tests, and thank you for moving the test header into support.
LGTM. I like Arthurs suggestion about the tests. But otherwise this change seems ready to go.
LGTM. I don't want to squash the conversation here, but I think we've discussed it enough. The actual changes here look solid, and I think they should be allowed to proceed.
Mar 4 2021
LGTM assuming the inline comments get addressed.
Feb 17 2021
I don't see any major issues with patch. It looks OK to me.
Jan 15 2021
We definitely want to unqualify __unwrap_iter to unbreak chromium. That's somewhat intended to be a customization point (until the proper contiguous iterator stuff lands).
Nov 30 2020
I don't see a reason to keep the builders.
Nov 17 2020
Bots are clean now.
Attempting to trigger rebuild
I'm trying to trigger a rebuild. Does accepting as libc++ do it?
Revert the addition of unneeded availability XFAIL's to variant tests.
Nov 12 2020
Oct 27 2020
After talking to @ldionne, the negative compilation tests aren't inappropriate here. I'm happy to see the stay.
I'm personally fine taking small bits of a paper.
I would rather it than have a full implementation get stuck in review.
Sep 9 2020
Sep 1 2020
Aug 17 2020
LGTM, this fixes some build failures at Google.
Aug 12 2020
Jul 15 2020
LGTM as well. Thanks for working on this!
Jul 13 2020
IDK if I know what's causing the issue, but this change LGTM.
Jun 19 2020
Jun 18 2020
This is not the ABI we want.
Jun 17 2020
Jun 15 2020
Jun 9 2020
How do we implement the "Mandates" requirement specified here?
Jun 4 2020
std::any, like other type erased types, cannot follow the allocator model and therefore does not use allocators.
All of that being said, this code is correct and LGTM**.
May 29 2020
Why is this code an improvement?
@mvels Do we have macro-benchmark results for this change?
May 13 2020
I'm a bit confused by the build size numbers you gave compared to the patch.
We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.
May 7 2020
I can't find any usages of this internally.
May 6 2020
If we don't want the static env, we should remove it entirely.
Meaning, we should convert each of these tests to use dynamic_test_env and setup the required files like the remainder of the tests do.
While I support this change, it should really be accompanied by a library working group issue or paper.
May 5 2020
Unless I'm mistaken, this change is racy. Tests in different files are run in parallel.
One test could be tearing down the static environment while another in trying to construct it.
When I initially wrote the filesystem tests, I had divided them between "static" and "dynamic"; that is, tests which modified the target filesystem and those which did not.
There are some simplifications suggested in the inline comments.
Once those are addressed, this LGTM.
Apr 23 2020
Apr 20 2020
Apr 16 2020
The entire point of the static test environment is that it's /static/.
Meaning it's checked in and not created on the fly.
This needs a bunch of additional tests. Specifically of the passing kind :-)
Apr 15 2020
Apr 10 2020
Apr 9 2020
Apr 8 2020
I assume this is a behavioral change? Why are there no new tests?
Committed as 601f7631827ae6ac08117a282c83a62b67dedf48
Apr 7 2020
I have some concerns with how we harden our concepts,
but these can be addressed in follow up commits.
Apr 4 2020
Apr 3 2020
I like tests.
Assuming all of them pass;: LGTM.
Mar 31 2020
Mar 26 2020
I don't necessarily agree.
Lets not start writing tests like this.