- User Since
- May 27 2014, 6:39 AM (216 w, 6 d)
Wed, Jul 18
Need to double check what tests we have when using relative path names at the root level. I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.
Tue, Jul 17
Mon, Jul 16
My review is incomplete, especially I cannot say with confidence if the proposed change is entirely free from unintended consequences that might break code not covered by the test suite. So other reviewers are welcome to chime in.
Overall looks good to me. Maybe add a test when a protocol is declared for an interface, not for a category. Something like
Tue, Jul 10
- In C++03 call allocator's destroy when available.
- Rename _Args as it's not a variadic template pack.
Mon, Jul 9
- Allow allocator construct to return a value, not just have return type void.
Fri, Jul 6
Thu, Jul 5
- Clean up tests according to review. We don't need a new test for custom allocators, parent patch covers that.
- Use a better way to detect presence of construct with required signature. Clean up tests.
Wed, Jul 4
Tue, Jul 3
- Proper support for custom allocators without construct.
- Clean up functionality not required by the Standard.
Mon, Jul 2
Fri, Jun 29
Thu, Jun 28
Try to exclude changes available in parent review from this review.
- Don't check !__has_construct for __construct_range_forward.
Wed, Jun 27
Tue, Jun 26
- Don't use memcpy specialization with custom allocators.
Thanks for the review, Richard.
Mon, Jun 25
Jun 22 2018
Looks good to me. The only problem is I'm not entirely sure this warning will interact with real code the way I expect it to. We'll need to keep an eye on it and tweak if necessary.
Jun 19 2018
Related change is https://reviews.llvm.org/D8109 For me the performance improvement was a twice faster execution on a dirty benchmark that doesn't exclude set up and tear down.
Jun 18 2018
Jun 11 2018
With this change and the mentioned libc++ change the tests with old libc++ dylib are passing (didn't test all possible configurations though). Would like to get more feedback from other reviewers on this matter.
Jun 8 2018
Sorry for the churn but can you please take out -nostdinc++ part out of this change? After more thinking and discussion we think there is a chance developers can use -nostdinc++ not only for building the standard library. -nostdinc++ is a signal of building the standard library but it's not strong enough. Most likely developers will be unaware how -nostdinc++ affects aligned allocations and that can lead to linker failures at runtime. So if you deploy on older macOS versions it is safer to assume aligned allocation isn't available. But if you provide your own libc++ with aligned allocations, you can still use -faligned-allocation to make that work. For now we prefer to use that approach and if it proves to be too onerous for developers, we can reintroduce -nostdinc++ logic.
May 30 2018
May 28 2018
Some debugging details that can be useful but too specific for the commit message. When we have infinite loop, the steps are:
- Fix only infinite loop and don't touch the assertion failure.
Looks good to me. Just watch the build bots in case some of them are strict with warnings and require (void)AddFilename(Filename).
May 25 2018
After looking into this more, I think there are 2 different bugs: one with infinite loop and another with DelayedTypos.empty() && "Uncorrected typos!" assertion. And disabling typo correction happened to fix both of them.
May 24 2018
May 23 2018
Somewhat tangential, in discussion with Duncan he mentioned that -nostdinc++ should turn off assumptions about old Darwin. So if you build libc++ yourself, you don't care what does the system stdlib support. I agree with that and think it doesn't interfere with the latest proposal but adds more to it.
May 22 2018
May 18 2018
May 17 2018
Eric, do you have more thoughts on this issue?
May 16 2018
Thanks for the quick review, Richard. I'll keep in mind the idea with comparing results of multiple lookups when I work on other partial specialization-related bugs.
May 15 2018
Thanks for review.
Potentially, VarTemplateDecl is susceptible to the same bug as
ClassTemplateSpecializationDecl. But I wasn't able to trigger it. And based on
code I've convinced myself that the mentioned early exit in
Sema::CheckVarTemplateId is equivalent to reusing available partial
specialization. So seems like no changes for VarTemplateDecl are required.
May 14 2018
Thanks for the review.
May 9 2018
May 8 2018
Here is another approach that should emit an error only when mixing headers
causes compilation problems.
__ALLOW_STDC_ATOMICS_IN_CXX__ approach didn't work in practice, I've reverted all changes.