- User Since
- Jun 26 2014, 12:44 PM (465 w, 6 d)
Wed, May 24
I'm going to unblock this. Because I don't think it's harmful.
@philnik Please revert this change. It breaks a bunch of downstream users. We can discuss how or if to reapply it again later, but for the moment, please revert the change while we figure it out.
We appreciate your help unblocking downstream.
Fri, May 19
Did some digging, and it seems this should be fine. LGTM again.
Wait a second.
Thu, May 18
You need to describe the rational in the description.
Wed, May 17
Is the patch as uploaded correct? Because some bits appear to me as having disappeared.
If the bots are happy, then go ahead and revert+reland.
But if there are merge issues, then I think that forgetting the phab review number in the commit message is OK.
I'm going to try rolling this change out at Google to see the scope of the breakage, how many bugs it finds, and what class of bugs those are.
I'll report back in a couple of days.
Tue, May 16
I'm with ldionne that we should keep using optional here. The correctness seems like a very important quality that we'll be loosing some certainty in.
Do we have -ftime-trace files for this change?
What semantic requirements do these violate. Please add that information to the description.
The __threading header, which defines a stable API that libc++ uses to implement threading primitives. This has been used by a bunch of vendors to shim in the different implementations for their platforms.
I believe we'll want to do the same thing here.
Apr 27 2023
Committed in db381405ced7b65ad1cf8fc60bbdfb505e1ed3de
Apr 15 2023
If we think there's no need for the friend declaration, then lets explain why we think it's unneeded in the commit message for archeology.
Sorry, I meant to comment that this is likely a misspelling of __slice_expr. Though I don't know if it's needed, and the test coverage here is really poor. But I would rather change it to what the author likely intended it to be.
Apr 14 2023
LGTM, but I would wait for another LGTM.
Apr 11 2023
OK, lets try this again. Hopefully with cooler heads.
Can you please update the patch so that it merges?
Have we done any benchmarking on this? I know the optimizer can be quite sensitive to the exact formulation of the loop.
Oh, and of course, after the CI is passing. Please re-upload a new patch so that it merges.
LGTM after addressing @phosek comment about the other delete functions.
Apr 10 2023
@philnik I believe your actions here are unbecoming. I would ask you to reconsider the way you interact in the community.
I think this is ready. It LGTM. Lets just get the CI passing.
Apr 6 2023
OK, so I've come around on this change. But please give me 2 weeks to cleanup the breakages it's going to cause inside Google.
I'll come back and let you know how many breakages were bugs vs how many were just weird meta-programming.
Apr 5 2023
I think this is a bandaid over a bigger clang bug.
Mar 20 2023
Thanks for updating the description. With rational, this makes a ton of sense.
Rereading the bug on github. I think this might deserve a standard defect report.
I think this is a really bad idea.
Splitting the specialization out so that only some files can depend on it, while others cannot, allowing some bits of the code to only see a partial implementation is going to be bug prone.
Has this patch been tested against Chromium? Has it been tested elsewhere?
The more testing it's undergone, the more confident I can be.
I agree with @Mordante
Mar 17 2023
Is this code ODR used? I suspect it is, and so it's not valid, but I'm not sure and I ran into a bunch of different instances of this pattern while testing this change.
Nice tests. Big fan of the work done in this patch.
Why? If we've done everything correctly, there should be no difference between the two type? What's the motivation for this?
Mar 10 2023
Disregard my suggestion about unavailable. It seems to always fire, including in unevaluated contexts. I feel silly.
Just use __attribute__((unavailable("declval cannot be called")) or some such.
Mar 8 2023
Also, I'm not sure a static assert pointing to the stable name in the
standard is helpful to our users. Will they even know what it is?
We went out of our way to make declval as low cost as we possibly could. Adding a body for the compiler to see is just work.
Mar 7 2023
Cool deal. Thanks for working on this.
@philnik Every single change needs a description. The description must include the motivation for making the change. What does in improve? What code does it break/fix? etc.
This increases code duplication substantially.
How is libc++ not working with it yet a blocker for them? We disable warnings in our headers except outside of our build?
Is there a design document describing these sets of changes?
If this affects the library *build* primarily, then it should be possible to make this continue to work.
This change is not correct and cannot proceed.
This cleanup seems incomplete. Why keep __less as a template at all given it's never actually used as one?
Mar 6 2023
Jan 13 2023
I'm not convinced this is something that we want to add the complexity for.
Dec 17 2022
Changes need descriptions.
Nov 29 2022
Nov 21 2022
Why is this a cleanup?
This changes behavior.
Nov 14 2022
Do you know why the hint wasn't initially used? Was there a point where compilers didn't actually provide the hints?
Nov 7 2022
Sorry, I'm going to have to let @ldionne weigh in on this too. I'm just not sure introducing this whole new build mode with a separate exception library is something we want to do -- so I would like input from the other maintainers.
I don't really like the state this leaves variant, any and friends in when exception vtables aren't compiled into the dylib.
There's no way to know only from the headers that using any of these types will result in linker errors.
Nov 6 2022
Stop being clever in tests. These changes add a substantial cognitive load to any person who's trying to read them for the first time and understand what they do.
This is from clang-format? LGTM then.
I don't think the test suite is the place to get clever with code deduplication and template magic.
Nov 3 2022
Disregard me. They were all marked inline already.
This also affects external instantiations. So I'm not sure this is as simple as just "removing boilerplate".
Nov 2 2022
Sorry for the repeated comments. But if I understand the description correctly, the diagnostic below is no longer emitted?
I would like other people to weigh in on this patch. I don't think these changes pull their weight. I'm not convinced they add clarity. And I'm certain they add complexity and cost -- also if we get the assert wrong (and I don't think we will), but we risk breaking totally valid code that would otherwise compile.
I'm not convinced your average user knows what copy insertable means, and I think the static assert is more opaque than the templated messages, which while noiser, contain more information.
This change doesn't improve conformance, right? It just attempts to make the diagnostics nicer in a handful of cases where the error occurs in the immediate function call to the allocator?