User Details
- User Since
- Jul 29 2013, 1:59 AM (390 w, 6 d)
Fri, Jan 22
That's funny because https://en.cppreference.com/w/cpp/compiler_support is already updated!
Thu, Jan 21
True, I was aware of the presence of some of these options, thank you for indicating others. I'm not yet entirely convinced, especially that clang-tidy behaviour would be possibly different.
Just an assert is ok IMO. We may fix it when LLVM will be compiled with C++20 but this code may change before it happens.
LGTM. Thanks for tidying up!
There's a clang-tidy check for it.
And clang-format should not, IMO, add not remove tokens but only handle whitespace.
Wed, Jan 20
LGTM.
Fix has_include. Use result_type.
Tue, Jan 19
LGTM per se. But, as it's a somehow a breaking change, I'd rather wait for release 12 branch before landing to main, so that folks have time to veto before release 13.
Please update release notes.
Also given that @MyDeveloperDay was involved in the bug discussion, please wait for his review too (if he has anything to add).
Mon, Jan 18
Rebase to trigger CI to reverify.
LGTM.
As a note, I agree with @MyDeveloperDay.
It's more user-friendly and IMO less surprising to set the base wrapping style in BreakBeforeBraces and then customize it changing in BraceWrapping.
I'd argue that it won't be a breaking change for .clang-format configs that are "correct" and would just make the examples you brought above work as intended.
Current behaviour is even strange in one case, what values are set to elements *omitted* in BraceWrapping when BreakBeforeBraces is Custom? Those from LLVM style?
Anyway, that goes way beyond this patch. I'd think about implementing this time permitting.
Sat, Jan 16
LGTM apart from the minor comment.
But I'd like @MyDeveloperDay to have a look too.
Fri, Jan 15
FWIW, you can take a look at the stub I've started writing at https://github.com/mkurdej/llvm-project/commit/0b7eccea2fbab13e897502c6d44ff958a0d550a9.
You might have a look at tests and other stuff like extended integer types.
It's just a WIP and far from ready though, so use with caution.
Thu, Jan 14
That looks like a strange style option to me. Is there any bigger codebase formatting the code this way?
One last nit, otherwise LGTM.
Shouldn't this patch implement also the renames of leap and link?
https://wg21.link/P1981 Rename leap to leap_second
https://wg21.link/P1982 Rename link to time_zone_link
Wed, Jan 13
Tue, Jan 12
Concerning Python version, I don't know of any official policy but given that Python 3.5 is already end-of-life, requiring Python 3.6 seems OK.
The implementation looks okay at the first glance but you need to rework the tests.
- Test noexcept using ASSERT_NOEXCEPT
- Add tests in non-constexpr context. Look at other recently added commits and add a test function with asserts that gets called twice, once normally (runtime) and once in static_assert.
- Split tests for each added function. Please follow the naming of test files according to the standard paragraphs.
- Add more tests with all sorted integer types and extended integer types.
- Test various values and their combinations: 0 limits::min/max +-1, min/2, max/2 and so on. Test interesting combinations of different types
Friendly ping.
LGTM.
Address comments.
Mon, Jan 11
This review should probably be abandoned, as it was implemented in D91778.
Address review comments.
Sun, Jan 10
Great.
LGTM if CI passes. But please wait for an approval from somebody from libc++ group, e.g. @ldionne.
Please reupload the diff setting the repo to rG LLVM Monorepo so that the CI gets triggered. You might need to do it twice to make it work. Otherwise use arcanist and it should do the job.
Otherwise looks good!
Fri, Jan 8
LGTM. I'm looking forward to seeing all your 50+ patches landed :).
@ldionne, shouldn't the related bug be updated?
https://llvm.org/PR41900
I may be repeating myself, but my biggest remark is that there are no tests. I know there's no libcxx CI on Windows, but still, that will make reviewing easier, looking what are the new test cases and what has been forgotten.
There is already D94227. You might want to wait for it and rebase when it's merged.
Rebase and resolve conflicts.
Thu, Jan 7
Seems to be a duplicate of D94206.
Yes, built and tested. Works fine. Will land it soon.
Sun, Jan 3
Sat, Jan 2
I agree on this. If like to see a more exhaustive test suite for all the combinations of AfterEnum and AllowShortEnumsOnASingleLine, not only fixing the single wrong test.
Wed, Dec 30
LGTM. Thank you!
You should add ["UNSUPPORTED: libcpp-has-no-threads"] to barrier, latch and semaphore.
Please add and fix tests.
Tue, Dec 29
Nit.
You might want to wait for D93383 to land, so that it updates C++2a to C++20.
Then, we need to have a way to test C++2b. I have a patch D93520 that adds Tip of the Trunk clang supporting -std=c++2b.
When this is done and the builders are updated, we need to add C++2b to the tested configurations.
Mon, Dec 28
My 2 cents. Otherwise I agree with @MyDeveloperDay's comments.
Sat, Dec 26
Fri, Dec 25
It would be great if we had some spec for this style, it gets a bit hard to verify if the formatting is actually what is expected.
Having said that, LGTM if the bug reporter confirms the behaviour is ok.
Dec 23 2020
Seems okay at the first glance. I'm wondering if we can find a better name though.
Dec 22 2020
LGTM now. I tried to find other cases where this change may change the behaviour but couldn't. Have you tried applying to some bigger repo and see what you get? The best would be a repo with SpaceAfterCStyleCast=true.
Dec 21 2020
- Quote {gdb}.
Hmm, just one thought, it could be tested using non-copyable comparators passed by (non-reduced) lvalue ref, nope? How did you find this "missed optimization"?
LGTM.
Dec 20 2020
Excuse my ignorance, but could you please explain why letting _Compare get deduced is wrong/suboptimal?
IIUC the case you mention is when e.g. __buffered_inplace_merge has _Compare being some non-ref type T, so then calling __half_inplace_merge will deduce its _Compare to be an lvalue ref T &. So far so good. But why/when is it bad?
Ok. For the moment I just fixed what was failing with my current setup but will fix cxx and gdb too.
There's already an ongoing effort to implement this: D90111.
You might want to sync with the author of the other patch.