- User Since
- Feb 11 2015, 3:26 PM (423 w, 1 d)
Adding libc++ vendors for awareness. Once the CI is green, I will take this for a spin and I suggest at least Google does it as well because I suspect this may lead to a lot of small and easy to fix issues. But I expect the number to be quite large. Depending on what we find out, we will be able to tell whether this is reasonable or not.
Rebase to re-trigger CI
Wed, Mar 22
Rebase for CI
Looks like this passed, shipping.
Gentle ping, this was likely a copy-paste error.
Try to re-trigger CI. I don't understand why the job is skipped and the CI reports as green.
Rebase and fix formatting.
Mon, Mar 20
LGTM, the failures in the libc++ CI are known flakes and the Windows one is a Clangd issue, most likely unrelated.
Remove format-specific availability markup, which isn't actually needed.
I just went through the patch and it LGTM. The table of contents was really useful here.
Okay so unfortunately, that didn't fix the issue like we were expecting. I tried a few things and the only thing that reliably fixed the issue we were seeing was reverting 4eddbf9f10a6d1881c93d84f4363d6d881daf848 (aka D122780) entirely. I think I will need to get a reproducer before deciding what's the next step (i.e. whether the issue is sufficiently vexing that it's worth reverting or not).
Sun, Mar 19
Rebase onto main.
CI failures are flukes.
Rebase onto main after applying comments.
Okay, so I got the results of building a large codebase with this change and I didn't see any regressions. I strongly think this won't be noticed by many users, if at all.
Sat, Mar 18
Fix a couple more incorrect XFAILs
Trigger CI with parent diff.
Re-upload with parent to trigger CI properly.
CI failures are unrelated.
CI failures are unrelated and we're on them, landing.
Fix incorrect markup in libcxx/test/std/strings/basic.string/string.capacity/reserve_size.pass.cpp
Can you please just rebase onto main and re-upload to give this one last CI spin? There's a Windows failure I don't understand. I think it's unrelated but rebasing and running the CI again should confirm that.
Fix a few filesystem tests failing on old deployment targets
Fri, Mar 17
Fix CI issue and rebase.
The no-exceptions configuration is failing due to a clang bug that Mark is still bisecting AFAIK, so this is green.
Could you give a really simple example of issues this allows catching? Like the "Hello world" for this new functionality. I think that would be useful for folks (at least for me) to understand concretely what we're gaining (I have no doubt we're gaining something, I just don't see it clearly in my mind).
Considering that this won't be cherry-picked back to release/16.x (right?), then whether we use _LIBCPP_CLANG_VER >= 1600 or _LIBCPP_CLANG_VER >= 1700 is irrelevant for all practical purposes, since libc++ gets released in sync with Clang and the next release this will get in will have Clang 17. I do agree however that if someone (in this case Chrome) is using a not-yet-released snapshot of trunk that happens to have _LIBCPP_CLANG_VER >= 1600 yet isn't something we've released as LLVM 16, then it's probably not reasonable for us to try to support that. Imagine coming up with that policy, I think it would mean that we can't ever rely on _LIBCPP_CLANG_VER >= 1600 during the libc++ 16 release, which would be pretty unfortunate.
Also tackle .verify.cpp tests
Add test to check that availability markup triggers appropriately and fix clang-format.
Thu, Mar 16
Update with commit message.
Commandeering per offline discussion.
I just looked at everything again with the understanding that adding a *copy* assignment operator doesn't have any impact on the generation of a copy constructor by the compiler (which frankly is bonkers), and I think this patch is correct without a doubt. The rules are just crazy though.
I am fine with this since:
I am fine with this in principle, but there seems to be a bit of work needed still. LGTM once CI is green. Also, when you remove transitive includes from a public header, let's add it to a _LIBCPP_REMOVE_TRANSITIVE_INCLUDES block (even if it might not be needed strictly speaking because the transitive include still exists via a private header). That documents what the public header is including and for what reasons (e.g. _LIBCPP_REMOVE_TRANSITIVE_INCLUDES implies the reason is legacy), and that's useful to know.
Can you explain that those are not necessary anymore since we moved to __verbose_abort in your commit message? Thanks for the patch, LGTM!
Thanks for the catch.
Wed, Mar 15
Tue, Mar 14
LGTM, it would be great if @vitaut could have a quick look as well.
I'd like the other folks on the review take a look first since they are more knowledgeable about Unicode.
That's a really nice catch! Could we perhaps add a libc++ specific .verify.cpp test to lock in this behavior?
LGTM once set within CMake
LGTM -- I am fine with either splitting the floating point to_chars and the integral ones or not splitting them. I think the current approach with a combined header that includes the floating-point/integral headers is a fine compromise.
Mon, Mar 13
Remove a stray use of allocator_arg in C++03
Thanks for the stack of patches! This is a really nice start. Before we commit to going into a specific direction, I think it is useful to lay out how we envision things being at the end, and then roughly how we're going to get there.
LGTM w/ passing CI. Please ping me again if you have to change the patch non-trivially to make CI pass.
Fix test failures in C++03 mode. The CI had actually not run on the previous iteration of the patch, I think that's because I uploaded child patches after uploading the parent patch.