User Details
- User Since
- Feb 11 2015, 3:26 PM (423 w, 1 d)
- Roles
- Administrator
Today
Generate files.
Yesterday
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
Rebase
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).
Fix clang-tidy
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
Match downstream
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.
Update
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.