Page MenuHomePhabricator

mclow.lists (Marshall Clow)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 16 2012, 3:06 PM (344 w, 1 d)

Recent Activity

Tue, Feb 12

mclow.lists accepted D57778: std::abs should not return double (2735).

I think this is good to go!

Tue, Feb 12, 12:08 PM

Mon, Feb 11

mclow.lists accepted D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.

This looks fine to me - thanks!

Mon, Feb 11, 2:26 PM · Restricted Project
mclow.lists added a comment to D58021: Win32 does not have popcnt64 function and fix bug with defines in ctz function.

This is all pretty much a lost cause on Windows anyway, since none of the compiler intrinsics are constexpr, and with the (upcoming) adoption of P0553 and P0556 these routines will have to be constexpr.

Mon, Feb 11, 8:39 AM
mclow.lists requested changes to D58021: Win32 does not have popcnt64 function and fix bug with defines in ctz function.
Mon, Feb 11, 8:30 AM

Sun, Feb 10

mclow.lists added a comment to D58018: Dummy revision.

I haven't looked at any of the "nonsense" yet.

Sun, Feb 10, 7:21 PM
mclow.lists added inline comments to D57778: std::abs should not return double (2735).
Sun, Feb 10, 11:39 AM
mclow.lists added inline comments to D57778: std::abs should not return double (2735).
Sun, Feb 10, 11:35 AM
mclow.lists added inline comments to D58011: Fix -fsanitize=vptr badness in <__debug>.
Sun, Feb 10, 7:04 AM · Restricted Project

Thu, Feb 7

mclow.lists committed rGaa09911aeffb: Add static_asserts to tuple's comparison operators to enforce the requirement… (authored by mclow.lists).
Add static_asserts to tuple's comparison operators to enforce the requirement…
Thu, Feb 7, 11:04 AM
mclow.lists committed rG954966c1afa2: Add UBSAN annotation to __hash_table::rehash; we don't do anything wrong, but… (authored by mclow.lists).
Add UBSAN annotation to __hash_table::rehash; we don't do anything wrong, but…
Thu, Feb 7, 10:55 AM

Wed, Feb 6

mclow.lists added inline comments to D57778: std::abs should not return double (2735).
Wed, Feb 6, 11:09 AM
mclow.lists added a comment to D57778: std::abs should not return double (2735).

Almost there - down to nits

Wed, Feb 6, 11:07 AM
mclow.lists committed rGc8879ab2fd95: Add a specialization for '__unwrap_iter' to handle const interators. This… (authored by mclow.lists).
Add a specialization for '__unwrap_iter' to handle const interators. This…
Wed, Feb 6, 8:10 AM
mclow.lists added inline comments to D57778: std::abs should not return double (2735).
Wed, Feb 6, 6:52 AM

Tue, Feb 5

mclow.lists added inline comments to D57778: std::abs should not return double (2735).
Tue, Feb 5, 9:50 PM
mclow.lists added a comment to D57778: std::abs should not return double (2735).

The title of https://wg21.link/LWG2735 is "std::abs(short), std::abs(signed char) and others should return int instead of double in order to be compatible with C++98 and C"

Tue, Feb 5, 6:08 PM
mclow.lists added a comment to D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.
In D57761#1385646, @jfb wrote:

I don't think this patch really needs to fix the tests, but I think the tests need a bit of love for this odd use case.

Not so odd, really. For example, it's reasonable to test that a user won't get UB if they fetch a key that does not exist in a map and exceptions are disabled. If we don't, we can basically declare our support for -fno-exceptions to be inexistent.

Tue, Feb 5, 1:05 PM · Restricted Project
mclow.lists added a comment to D57734: priority_queue::replace_top(x).

No one is "forced" to call __sift_down. In fact, it's explicitly forbidden by the standard.

Tue, Feb 5, 10:16 AM · Restricted Project
mclow.lists added a comment to D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.

I think all of these are no-brainers except the one calling __throw_failure.

Tue, Feb 5, 10:13 AM · Restricted Project
mclow.lists added a comment to D57734: priority_queue::replace_top(x).

@mclow.lists: Easy peasy! Please take another look.

Tue, Feb 5, 9:04 AM · Restricted Project
mclow.lists added a comment to D57734: priority_queue::replace_top(x).

Arthur - I'm sorry to say that this will go nowhere.
You're introducing non-reserved identifiers into libc++, which can cause legal code to break.

Tue, Feb 5, 7:56 AM · Restricted Project

Mon, Feb 4

mclow.lists added inline comments to D57403: Extending make_shared to Support Arrays (Partially) - P0674R1.
Mon, Feb 4, 7:22 PM
mclow.lists added a comment to D57688: [libcxx] Remove <ext/hash_set>, <ext/hash_map> and <ext/__hash>.

Oh, I agree. However, when I have proposed this in the past, I have gotten pushback from many people (including Apple, Google and others).
At the very least, you should announce this on llvm-dev, and get feedback there.

Do you mean libcxx-dev? cfe-dev? (this seems really off topic for llvm-dev)

Mon, Feb 4, 11:20 AM
mclow.lists added a comment to D57688: [libcxx] Remove <ext/hash_set>, <ext/hash_map> and <ext/__hash>.

Oh, I agree. However, when I have proposed this in the past, I have gotten pushback from many people (including Apple, Google and others).
At the very least, you should announce this on llvm-dev, and get feedback there.

Mon, Feb 4, 11:12 AM
mclow.lists added a comment to D57624: Support tests in freestanding.

@jfb, I am not going to look at all of these changes. If you assure me that they all look like this: https://reviews.llvm.org/differential/changeset/?ref=1324002
and that the test suite still passes on the desktops, I'm fine with you applying this.

Mon, Feb 4, 10:44 AM · Restricted Project
mclow.lists added a comment to D57624: Support tests in freestanding.

My take on this is

Mon, Feb 4, 9:56 AM · Restricted Project

Fri, Feb 1

mclow.lists added a comment to D57455: [libunwind] Provide inline placement new definition.

@rsmith just reminded me (in IRC) that it's perfectly legal to declare operator new in your classes.

Fri, Feb 1, 2:08 PM · Restricted Project
mclow.lists added a comment to D57455: [libunwind] Provide inline placement new definition.

My problem with declaring our own placement new is that I think we're just kicking the can down the road; that the problem might (will) come back to bite us in the future.
It would certainly be conforming for clang to reject this code at compile time.

Fri, Feb 1, 1:16 PM · Restricted Project
mclow.lists accepted D57606: [libc++] Disentangle the 3 implementations of type_info.

This looks fine to me. It also looks (to me) that it will make applying https://wg21.link/P1328 cleaner (assuming it gets approved)

Fri, Feb 1, 11:54 AM · Restricted Project, Restricted Project
mclow.lists added inline comments to D56692: More calendar bits for <chrono>.
Fri, Feb 1, 10:28 AM
mclow.lists updated the diff for D56692: More calendar bits for <chrono>.

Update based on Eric's comments.

Fri, Feb 1, 10:27 AM

Thu, Jan 31

mclow.lists closed D57391: [libcxx] Portability fix: regex algorithms exceptions made optional..
Thu, Jan 31, 10:55 AM
mclow.lists accepted D57391: [libcxx] Portability fix: regex algorithms exceptions made optional..

Committed as revision 352781.

Thu, Jan 31, 10:54 AM

Wed, Jan 30

mclow.lists added a comment to D57455: [libunwind] Provide inline placement new definition.

Turns out the placement versions of new are not replaceable, so what I said about UB is just wrong.

Wed, Jan 30, 1:27 PM · Restricted Project
mclow.lists added inline comments to D56692: More calendar bits for <chrono>.
Wed, Jan 30, 1:06 PM
mclow.lists added a comment to D57391: [libcxx] Portability fix: regex algorithms exceptions made optional..

Other than the missing // UNSUPPORTED bits, this looks good to me.
Do you need me to commit this?

Wed, Jan 30, 12:34 PM
mclow.lists added a comment to D57455: [libunwind] Provide inline placement new definition.

Um... I suspect that this is technically UB. Having more than two (one in the standard library, and one other) in an executable context is a bad idea.
Marking it as inline might make it work, but I still think it's UB.

Wed, Jan 30, 12:23 PM · Restricted Project

Tue, Jan 29

mclow.lists added a comment to D44823: [libcxx] Improving std::vector<char> and std::deque<char> perfomance.

I just tried this (on Compiler Explorer) using LLVM 7, and the code for my original test in https://bugs.llvm.org/show_bug.cgi?id=35637 is now optimal.

Tue, Jan 29, 9:40 AM
mclow.lists added a comment to D44823: [libcxx] Improving std::vector<char> and std::deque<char> perfomance.

I just tried this (on Compiler Explorer) using LLVM 7, and the code for my original test in https://bugs.llvm.org/show_bug.cgi?id=35637 is now optimal.
Looking briefly at your test case, it seems to be fixed now too. Can you confirm or disprove, please?

Tue, Jan 29, 9:20 AM

Mon, Jan 28

mclow.lists created D57364: Bug fix for PR#40495 is_invokable_v<void> does not compile.
Mon, Jan 28, 5:15 PM
mclow.lists added a comment to D57142: [CMake] Use __libc_start_main rather than fopen when checking for C library.

This change breaks building on Mac OS X - at least for libc++.
The user visible failure is:

Mon, Jan 28, 2:47 PM

Thu, Jan 24

mclow.lists closed D14686: Protect against overloaded comma in random_shuffle and improve tests.

(Finally) committed this as revision 352087.
I cut out most of the random_shuffle_rand.pass.cpp test, because it relied on C++11 features, and didn't work for C++03.
If you want to re-submit the test as a separate patch, I promise to review it in a more timely manner.

Thu, Jan 24, 11:21 AM

Wed, Jan 23

mclow.lists closed D16541: [libc++] Renable test/std/re/re.alg/re.alg.match/awk.pass.cpp.

For some reason, this test was not marked // XFAIL gnu-linux like all the other tests that use the LOCALE_cs_CZ_ISO8859_2 locale.
In revision 352006, I uncommented this entire test, and added the XFAIL line

Wed, Jan 23, 5:53 PM
mclow.lists closed D26110: Add a check for GCC to the _LIBCPP_EXPLICIT define.

Yes, this appears to have been landed. Closing.

Wed, Jan 23, 3:10 PM
mclow.lists closed D28248: Work around GCC PR37804.

I changed the _LIBCPP_TYPE_VIS_ONLY to _LIBCPP_TEMPLATE_VIS and committed this as revision 351993.

Wed, Jan 23, 3:07 PM
mclow.lists closed D11348: Win32 support: wcsnrtombs and mbsnrtowcs don't handle null output buffers correctly..

(Finally) committed as revision 351971

Wed, Jan 23, 10:28 AM
mclow.lists added a comment to D57001: [libunwind] Don't define unw_fpreg_t to uint64_t for __ARM_DWARF_EH__.

I have no problem with the code change, but no context on whether or not it is correct.
I'm hoping some other people familiar with ARM and DWARF can chime in.

Wed, Jan 23, 10:15 AM

Tue, Jan 22

mclow.lists added a comment to D56997: Fix implementation of P0966 - string::reserve Should Not Shrink.

Made some changes to keep behavior the same in older -std= dialects. We shouldn't overload the function to avoid code such as:

auto f(&::std::string::reserve); // Ambiguous post C++17

from breaking for std < 17 (will break if -std=c++2a however)

Tue, Jan 22, 8:12 PM
mclow.lists updated the diff for D57058: Implement LWG3101 - span's Container constructors need another constraint.

Re-gen the diff after I updated the tests based on Louis' comments (see r351887)

Tue, Jan 22, 2:49 PM
mclow.lists added inline comments to D57058: Implement LWG3101 - span's Container constructors need another constraint.
Tue, Jan 22, 12:38 PM
mclow.lists added inline comments to D57058: Implement LWG3101 - span's Container constructors need another constraint.
Tue, Jan 22, 11:07 AM
mclow.lists created D57058: Implement LWG3101 - span's Container constructors need another constraint.
Tue, Jan 22, 8:23 AM
mclow.lists added a comment to D57039: Implement LWG3144 - span does not have a const_pointer typedef.

Thanks for the quick review. I'm going to hold off on landing this until after Kona.

Tue, Jan 22, 8:19 AM

Mon, Jan 21

mclow.lists created D57039: Implement LWG3144 - span does not have a const_pointer typedef.
Mon, Jan 21, 5:03 PM

Jan 18 2019

mclow.lists accepted D56905: [libunwind] [SjLj] Don't use __declspec(thread) in MinGW mode.

Sigh. At least we've got a macro wrapping this; we only have to pore over this once.

Jan 18 2019, 12:06 PM

Jan 15 2019

mclow.lists accepted D56750: Implement feature test macros using a script..

I am coming to like the idea that these tests should live in test/libcxx, not test/std, because they're all about libc++'s implementation status.
That being said, the tests are in test/std today, and this isn't an awful place for them.

Jan 15 2019, 4:33 PM
mclow.lists added inline comments to D56750: Implement feature test macros using a script..
Jan 15 2019, 3:42 PM
mclow.lists accepted D56698: [libc++] Support different libc++ namespaces in the iterator test.

This looks fine to me,

Jan 15 2019, 6:51 AM

Jan 14 2019

mclow.lists added inline comments to D56692: More calendar bits for <chrono>.
Jan 14 2019, 5:26 PM
mclow.lists created D56692: More calendar bits for <chrono>.
Jan 14 2019, 5:25 PM
mclow.lists added inline comments to D51762: First part of the <chrono> calendar stuff.
Jan 14 2019, 10:07 AM
mclow.lists accepted D53763: [libc++] [test] Fix logic error in <compare> tests; enable for MSVC previews.

I'm a bit concerned about the TEST_HAS_NO_SPACESHIP_OPERATOR and how it tracks with _LIBCPP_HAS_NO_SPACESHIP_OPERATOR, but I'm not going to hold this up for that.

Jan 14 2019, 10:00 AM
mclow.lists added a comment to D56578: Fix size_t/off_t mixup in std::filesystem..

this looks fine to me; but I'd like @EricWF to chime in here.

Jan 14 2019, 9:55 AM

Jan 11 2019

mclow.lists closed D55466: Change `tuple_size` from a `class` to a `struct` (see PR#39871).

Committed as revision 350972.

Jan 11 2019, 2:02 PM
mclow.lists added inline comments to D55840: P0722R3: Implement library support for destroying delete.
Jan 11 2019, 11:26 AM
mclow.lists closed D56494: Implement the `sys_time` portions of the C++20 calendaring stuff.

Committed as revision 350929

Jan 11 2019, 7:56 AM
mclow.lists added a comment to D56578: Fix size_t/off_t mixup in std::filesystem..

reference: http://pubs.opengroup.org/onlinepubs/009604499/functions/ftruncate.html

Jan 11 2019, 6:57 AM

Jan 10 2019

mclow.lists added inline comments to D56398: Add new EINTEGRITY errno.
Jan 10 2019, 11:05 AM
mclow.lists added inline comments to D54410: [libc++] Add C++17 deduction guides for std::function.
Jan 10 2019, 10:36 AM
mclow.lists accepted D54772: [libcxx] Reorganize tests since the application of P0602R4.

Sorry this took me so long.

Jan 10 2019, 10:30 AM
mclow.lists accepted D55427: [libcxx] Call __count_bool_true for bitset count.

After removing the __VSTD::, I'm good with this.

Jan 10 2019, 10:28 AM

Jan 9 2019

mclow.lists updated the diff for D56494: Implement the `sys_time` portions of the C++20 calendaring stuff.

Added support for local_time in the calculations as well.

Jan 9 2019, 2:08 PM
mclow.lists created D56494: Implement the `sys_time` portions of the C++20 calendaring stuff.
Jan 9 2019, 8:00 AM

Jan 7 2019

mclow.lists added a comment to D55791: [libcxx] Simplify conditional definition of _LIBCPP_HAS_NO_ALIGNED_ALLOCATION.

Ok, some of those we define ourselves w/o checking.

Jan 7 2019, 10:51 AM
mclow.lists added a comment to D55791: [libcxx] Simplify conditional definition of _LIBCPP_HAS_NO_ALIGNED_ALLOCATION.

In general, we *do* protect against user-defined defines.
This, and defining stuff as negative (_LIBCPP_HAS_NO_XXX) lets users disable parts of the libc++ functionality by defining the macros.

So I think we should leave this "as is"

The problem I have with this is that we're hence implicitly supporting this use case, which means we should make sure we don't break it, we should add a tester with this configuration, and we should document that this macro can be used to alter the behaviour of the library. Either we should do this if we have a reason to, or we should remove this accidental API by not checking whether the user defines the macro. Does this make sense?

Jan 7 2019, 10:50 AM
mclow.lists added a comment to D55791: [libcxx] Simplify conditional definition of _LIBCPP_HAS_NO_ALIGNED_ALLOCATION.

What I'm saying is that this is a pattern we use throughout libc++, and I'd rather follow that pattern than explain why _LIBCPP_HAS_NO_ALIGNED_ALLOCATION is different from all these others.

Jan 7 2019, 10:50 AM
mclow.lists added a comment to D55945: [pstl] Avoid shadowing explicit lambda capture with lambda parameter.

This looks reasonable to me.

Jan 7 2019, 9:15 AM
mclow.lists added a comment to D55840: P0722R3: Implement library support for destroying delete.

You should also update www/cxx2a_status.html to show that P0722 is "complete".

Jan 7 2019, 9:12 AM
mclow.lists added a comment to D55840: P0722R3: Implement library support for destroying delete.

Another question - why do we need to check if clang supports destroying delete to define the type destroying_delete_t? Why not define it always, and just define the feature test macro when we have compiler support?

Jan 7 2019, 8:13 AM
mclow.lists added a comment to D55840: P0722R3: Implement library support for destroying delete.

This is going to need some work before landing.

Jan 7 2019, 8:10 AM
mclow.lists added a comment to D55791: [libcxx] Simplify conditional definition of _LIBCPP_HAS_NO_ALIGNED_ALLOCATION.

In general, we *do* protect against user-defined defines.
This, and defining stuff as negative (_LIBCPP_HAS_NO_XXX) lets users disable parts of the libc++ functionality by defining the macros.

Jan 7 2019, 7:58 AM
mclow.lists added a comment to D55763: [Sparc] Add Sparc V8 support.

If Saleem is happy with this, then I'm good too.

Jan 7 2019, 7:54 AM
mclow.lists accepted D56023: [libcxx] Mark do_open, do_get and do_close parameters unused when catopen is missing.

I like this much better now; making a note to myself to go back to <unordered_map> and <stdexcept> and use this there.

Jan 7 2019, 7:51 AM
mclow.lists added a comment to D55466: Change `tuple_size` from a `class` to a `struct` (see PR#39871).

Oh, look, this is https://github.com/cplusplus/draft/issues/534, where JW wrote:

Jan 7 2019, 7:32 AM
mclow.lists added a comment to D55466: Change `tuple_size` from a `class` to a `struct` (see PR#39871).

Should we commit this before the next release? Maybe ping Wakly to get the editorial fix landed?

Jan 7 2019, 7:19 AM
mclow.lists added a comment to D56357: Fix PR40230 - std::pair may have padding on FreeBSD..

I'm fine with this as well - assuming the FreeBSD people are happy.

Jan 7 2019, 7:17 AM

Dec 23 2018

mclow.lists added inline comments to D56023: [libcxx] Mark do_open, do_get and do_close parameters unused when catopen is missing.
Dec 23 2018, 5:04 PM

Dec 18 2018

mclow.lists accepted D55834: [libcxx] Make sure all experimental tests are disabled when enable_experimental=False.

In other words, I fully agree that we should strive to run experimental tests as often as possible (when it makes sense), and we do have the right default here. But when I explicitly say "don't give me experimental tests", I don't want them to run.

Dec 18 2018, 3:23 PM
mclow.lists added a comment to D55777: [libcxx] Portability fix: add missing includes and static_casts..

Committed all of this except "poisoned_hash_helper.hpp" as revision 349566

Dec 18 2018, 3:22 PM
mclow.lists added a comment to D55777: [libcxx] Portability fix: add missing includes and static_casts..

Did you run these tests after adding the includes?
The "poisoned hash" tests fail - because poisoned_hash_helper.hpp now includes various hash specializations.
In fact, it causes clang to crash (I've reported PR 40093 for that).
I'll remove the changes to "poisoned_hash_helper.hpp" and see what happens.

Dec 18 2018, 1:23 PM
mclow.lists accepted D55674: [libunwind] [SEH] Add initial support for AArch64.

LGTM.

Dec 18 2018, 11:54 AM
mclow.lists added a comment to D55777: [libcxx] Portability fix: add missing includes and static_casts..

I can't commit the patch myself: I don't have rights to do this.

Dec 18 2018, 10:48 AM
mclow.lists accepted D55777: [libcxx] Portability fix: add missing includes and static_casts..

The includes look fine. The static_casts (as always) look ugly. Once you make the changes that I've requested, you can commit this.

Dec 18 2018, 7:44 AM

Dec 17 2018

mclow.lists added a comment to D55746: [libcxx] [test] [re.traits] Correct expected values for invalid UTF-8.

So what do you suggest we do about this test?

Dec 17 2018, 11:02 AM
mclow.lists added a comment to D55746: [libcxx] [test] [re.traits] Correct expected values for invalid UTF-8.

I have written a similar program, but using t.translate_nocase. All the characters from C0 --> DE are translated on Darwin.

Dec 17 2018, 10:35 AM
mclow.lists added a comment to D55746: [libcxx] [test] [re.traits] Correct expected values for invalid UTF-8.

[ It's also possible that different implementations of the locales (part of the C library/OS) are returning different values. If that turns out to be the case, then we should document those differences and move on. ]

I think that's the case. I've written a simple test program to check it. Could you try it on Darwin?

Dec 17 2018, 10:34 AM
mclow.lists added a comment to D55746: [libcxx] [test] [re.traits] Correct expected values for invalid UTF-8.

In my opinion we shall not test UB as each implementation can probably behave in any way.

Dec 17 2018, 10:05 AM
mclow.lists added a comment to D55746: [libcxx] [test] [re.traits] Correct expected values for invalid UTF-8.

On Mac OS, using the locale en_US.UTF-8, the call std::re_traits<char>().translate_nocase('\xDA') returns '\xFA'

Dec 17 2018, 8:28 AM
mclow.lists added a comment to D55746: [libcxx] [test] [re.traits] Correct expected values for invalid UTF-8.

This updated test fails on Mac OS X. (the assert on line 48 fires)

Dec 17 2018, 8:23 AM
mclow.lists added a comment to D55746: [libcxx] [test] [re.traits] Correct expected values for invalid UTF-8.

Amusingly enough, I received this bug report this morning, which appears be related.

Dec 17 2018, 8:18 AM