dexonsmith (Duncan P. N. Exon Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 18 2014, 10:33 AM (174 w, 2 d)

Recent Activity

Tue, Jul 18

dexonsmith accepted D34439: Add GCC's noexcept-type alias for c++1z-compat-mangling.

LGTM.

Tue, Jul 18, 5:23 AM

Sun, Jul 16

dexonsmith added a comment to D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.

Rebase. I don't think the issue of purging underscores from this file/libcxx should block this, if we want to discuss that cfe-dev would probably be the place. I agree that it would be nice to clang-format this file, would anyone have any problems with me committing that after this? I found the git-blame of this file to be pretty useless, almost every line just points to r184097.
Are there any other thoughts on this change?

Sun, Jul 16, 8:05 PM

Thu, Jul 13

dexonsmith added a comment to D34331: func.wrap.func.con: Unset function before destroying anything.

@dexonsmith Mind if I hijack this and check in your changes to <functional> with my tests?

Not at all.

@dexonsmith I'm not sure it's sane to allow reentrant behavior. Could you explain why you think it is?

It didn't seem sane to me at first either, despite this being supported by std::unique_ptr and std::shared_ptr. I found our user fairly convincing, though:

  • They had an underlying reference counted object, so only the destruction of the last copy of A nulled out the pointer (mimicked that with the bool cancel).
  • They had a callback function intended to be called once, and dropping that function on cancellation (mimicked that with a global variable). The cancel operation nulled out a std::function, but destroying the objects captured in the lambda in that std::function also separately decided to perform a cancel, which should have been OK. The cancel function just contained callback = nullptr.

According to [reentrancy] it is implementation defined what STL functions are allowed to be recursively reentered. I'm not opposed to providing reentrancy where it's useful, but we would have to provide it well.
This is where I was running into problems while I was writing tests. There are so many different ways you can reenter std::function's special members that it makes it impossible for an implementation
to truly support reentrancy as the standard would require.

If you consider that every possible copy construction or destruction of a user type is a potential reentrancy point, the complexity of having well-defined reentrant behavior starts to become clear.
Any time a copy constructor or destructor is invoked you have a potential reentrancy point which, in order to provide well defined behavior, must also act as a sort of _sequence point_ where the side effects of the current call are visible to the reentrant callers (For example your patches use of operator=(nullptr_t)).

The methods fixed in this patch are seemingly improvements; It's clear that operator=(nullptr) can safely be made reentrant. On the other hand consider operator=(function const& rhs), which is specified as equivalent to function(rhs).swap(*this).
I posit swap is not the kind of thing that can easily be made reentrant, but that making std::function assignment reentrant in general clearly requires it.

If fixing this bug is sufficiently important I'm willing to accept that, but I don't think it improves the status quo; We may have fixed a specific users bug, but we haven't made these functions safely reentrant (in fact most of the special members are likely still full of reentrancy bugs).

Thu, Jul 13, 8:50 AM

Wed, Jul 12

dexonsmith added a reviewer for D35159: [libcxxabi][demangler] Use an AST to represent the demangled name: mclow.lists.

+mclow.lists

Wed, Jul 12, 2:03 PM
dexonsmith added a reviewer for D34781: Introduce a MCReloc class: enderby.
Wed, Jul 12, 1:39 PM
dexonsmith added a comment to D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.

This LGTM. Mehdi, do you have any other concerns?

Wed, Jul 12, 1:34 PM

Sun, Jul 9

dexonsmith added a comment to D34331: func.wrap.func.con: Unset function before destroying anything.

@dexonsmith Mind if I hijack this and check in your changes to <functional> with my tests?

Sun, Jul 9, 6:41 AM
dexonsmith added a dependency for D35159: [libcxxabi][demangler] Use an AST to represent the demangled name: D35158: [libcxxabi][demangler] NFC: Don't make everything a template.
Sun, Jul 9, 6:13 AM
dexonsmith added a dependent revision for D35158: [libcxxabi][demangler] NFC: Don't make everything a template: D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.
Sun, Jul 9, 6:13 AM
dexonsmith accepted D35158: [libcxxabi][demangler] NFC: Don't make everything a template.

Assuming nothing in arena<>...Db actually changed (just moved), this LGTM if you split the move into a separate prep commit.

Sun, Jul 9, 6:13 AM

Thu, Jul 6

dexonsmith closed D34578: cmath: Support clang's -fdelayed-template-parsing.

Ping! Hal, you committed r283051. Do you have a problem with this?

It looks like you're just renaming __libcpp_isnan and friends to __libcpp_isnan_or_builtin` and similar. LGTM

Thu, Jul 6, 10:20 PM
dexonsmith committed rL307357: cmath: Support clang's -fdelayed-template-parsing.
cmath: Support clang's -fdelayed-template-parsing
Thu, Jul 6, 10:14 PM
dexonsmith added a comment to D34578: cmath: Support clang's -fdelayed-template-parsing.

Ping! Hal, you committed r283051. Do you have a problem with this?

Thu, Jul 6, 1:58 PM
dexonsmith accepted D35069: [Frontend] Verify that the bitstream is not empty before reading the serialised diagnostics.

LGTM.

Thu, Jul 6, 1:56 PM
dexonsmith added a comment to D34331: func.wrap.func.con: Unset function before destroying anything.

Ping!

Thu, Jul 6, 1:49 PM

Mon, Jun 26

dexonsmith added a comment to D34574: [Sema] Disable c++17 aligned new and delete operators if not implemented in the deployment target's c++ standard library.

Should we also remove the recently-added availability attributes in libc++ too? If I'm using a recent libc++ and providing my own aligned allocation functions, we shouldn't reject attempts to call those aligned allocation functions.

Mon, Jun 26, 2:19 PM
dexonsmith added a comment to D34574: [Sema] Disable c++17 aligned new and delete operators if not implemented in the deployment target's c++ standard library.

Duncan and I had a discussion on this.

We are thinking about adding a warning that tells users that aligned allocation /deallocation operators are being called but they are not defined in the library.

Mon, Jun 26, 2:08 PM
dexonsmith accepted D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute.

LGTM.

Mon, Jun 26, 9:57 AM

Fri, Jun 23

dexonsmith created D34578: cmath: Support clang's -fdelayed-template-parsing.
Fri, Jun 23, 3:56 PM
dexonsmith added inline comments to D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute.
Fri, Jun 23, 9:03 AM
dexonsmith added a comment to D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute.

I followed the other availability macros in using "strict", but I'm not sure this was the right decision. The documentation seems to recommend not using "strict" (it says that weakly-linking is almost always a better API choice).

libc++ is one of the exceptions. Use strict.

Fri, Jun 23, 8:35 AM

Thu, Jun 22

dexonsmith added a reviewer for D34446: [Support] sys::getProcessTriple should return a macOS triple using the system's version of macOS: lhames.

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

The primary users of this function are the JIT libraries, but so far I haven't found any checks for the OS version in the JIT code. And it looks like most of the other users of this function don't check the OS version.

Thu, Jun 22, 11:32 AM

Wed, Jun 21

dexonsmith added inline comments to D34264: Introduce -Wunguarded-availability-new, which is like -Wunguarded-availability, except that it's enabled by default for new deployment targets.
Wed, Jun 21, 5:42 PM
dexonsmith added a comment to D34446: [Support] sys::getProcessTriple should return a macOS triple using the system's version of macOS.

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Wed, Jun 21, 3:04 PM

Jun 19 2017

dexonsmith added a comment to D34251: Add a new driver option to disable warning about c++17's non-throwing exception specification in function signature.

We use the -Wc++NN-compat- prefix on all the other subwarnings of -Wc++NN-compat warnings

Jun 19 2017, 4:25 PM
dexonsmith accepted D34251: Add a new driver option to disable warning about c++17's non-throwing exception specification in function signature.

I think -Wc++1z-mangling is best. IMO, with C++1z in the name, it's already clear that this is about compatibility. I also don't think we need to get as specific as -Wc++1z-compat-exception-spec.

Jun 19 2017, 3:58 PM

Jun 18 2017

dexonsmith closed D34332: path: Use string_view_t consistently.

Fixed in r305661.

Jun 18 2017, 9:28 PM
dexonsmith committed rL305661: path: Use string_view_t consistently.
path: Use string_view_t consistently
Jun 18 2017, 9:28 PM
dexonsmith created D34332: path: Use string_view_t consistently.
Jun 18 2017, 12:13 PM
dexonsmith created D34331: func.wrap.func.con: Unset function before destroying anything.
Jun 18 2017, 11:52 AM
dexonsmith committed rL305649: iostreams: Fix deployment target for streams dylib support.
iostreams: Fix deployment target for streams dylib support
Jun 18 2017, 9:51 AM
dexonsmith committed rL305648: func.wrap.func.con: Fix test comment.
func.wrap.func.con: Fix test comment
Jun 18 2017, 8:35 AM
dexonsmith closed D33177: any: Add availability for experimental::bad_any_cast.

Committed in r305647.

Jun 18 2017, 7:53 AM
dexonsmith committed rL305647: any: Add availability for experimental::bad_any_cast.
any: Add availability for experimental::bad_any_cast
Jun 18 2017, 7:53 AM

Jun 17 2017

dexonsmith accepted D34175: [driver][macOS] Pick the system version for the deployment target if the SDK is newer than the system.

LGTM.

Jun 17 2017, 7:15 PM

Jun 15 2017

dexonsmith added inline comments to D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple.
Jun 15 2017, 9:45 PM
dexonsmith added inline comments to D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple.
Jun 15 2017, 9:36 PM
dexonsmith added inline comments to D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple.
Jun 15 2017, 9:34 PM
dexonsmith requested changes to D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple.
Jun 15 2017, 3:14 PM
dexonsmith added a comment to D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple.

This is the right idea, although it only covers macOS.

Jun 15 2017, 3:13 PM

Jun 14 2017

dexonsmith committed rL305418: AST: Add missing break at end of switch.
AST: Add missing break at end of switch
Jun 14 2017, 2:27 PM

Jun 13 2017

dexonsmith added a comment to D33977: [libcxx][WIP] Experimental support for a scheduler for use in the parallel stl algorithms.

You mention that the lock-free deque gives a speedup. I agree that should be left for a follow-up, but what kind of speedup did it give?

Jun 13 2017, 3:58 PM

Jun 1 2017

dexonsmith accepted D33739: [Sema] Improve -Wstrict-prototypes diagnostic message for blocks.

LGTM.

Jun 1 2017, 9:55 AM

May 29 2017

dexonsmith added a comment to D33513: [ThinLTO] Fix ThinLTO crash while destroying context.

The current patch seems to be missing context on Phab (-U999999). Please remember to add it on the next iteration.

May 29 2017, 10:06 AM · lld

May 25 2017

dexonsmith added a comment to D33513: [ThinLTO] Fix ThinLTO crash while destroying context.

If there is no other path to exercise LazyLoading of Metadata, let's just create one! (add an option to llvm-dis for example).

I agree.

May 25 2017, 5:06 PM · lld
dexonsmith added a comment to D33513: [ThinLTO] Fix ThinLTO crash while destroying context.

If there is no other path to exercise LazyLoading of Metadata, let's just create one! (add an option to llvm-dis for example).

May 25 2017, 5:05 PM · lld
dexonsmith added a reviewer for D33513: [ThinLTO] Fix ThinLTO crash while destroying context: mehdi_amini.

I'm probably a good person to review this, but it's been a while since I've thought this so I might need help to refresh my context. This use-tracking code is unfortunately rather subtle.

May 25 2017, 4:58 PM · lld
dexonsmith accepted D28404: IRGen: Add optnone attribute on function during O0.

Actually, looking through the comments, it appears that everyone (eventually) agreed with the approach in the patch. I agree too. LGTM.

May 25 2017, 4:38 PM

May 23 2017

dexonsmith added inline comments to D33368: [libcxxabi][demangler] Fix a crash in the demangler.
May 23 2017, 7:08 PM

May 22 2017

dexonsmith added a reviewer for D33393: [PATCH] Libcxxabi Demangler PR32890: erik.pilkington.
May 22 2017, 10:49 PM

May 21 2017

dexonsmith added inline comments to D33368: [libcxxabi][demangler] Fix a crash in the demangler.
May 21 2017, 10:43 PM

May 19 2017

dexonsmith committed rL303487: Docs: Fix pluralization in CMake docs.
Docs: Fix pluralization in CMake docs
May 19 2017, 10:39 PM

May 17 2017

dexonsmith accepted D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.

Tests LGTM.

May 17 2017, 9:07 PM

May 16 2017

dexonsmith added a comment to D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.

As an aside, it seems the logic around unreachable (with and without the patch) might pessimize code paths that lead to noreturn functions, such as posix_spawn(). Is that really what we want? Why?

Heh, I thought about bringing this up, but it really seems orthogonal and something I wanted to look at.

Yeah, generally, the unreachable logic I wrote eons ago was dead wrong and this is still wrong in the case of nice "run forever" functions. I'll either send a patch or file a bug about this.

May 16 2017, 10:58 AM
dexonsmith added a comment to D30633: [BPI] Reduce the probability of unreachable edge to minimal value greater than 0.

The testcase updates look good to me, and demonstrate the patch is doing what's intended. It might be worth adding a couple of tests for multiple branch targets (switch statements), since I'm not convinced everything adds up to 1 right now.

May 16 2017, 10:42 AM

May 14 2017

dexonsmith created D33177: any: Add availability for experimental::bad_any_cast.
May 14 2017, 4:55 PM

May 13 2017

dexonsmith added inline comments to D33049: [libcxx] Support for Objective-C++ tests.
May 13 2017, 2:12 PM

May 9 2017

dexonsmith added a comment to D32724: [Modules] Handle sanitizer feature mismatches when importing modules.
In D32724#750074, @vsk wrote:

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

I'm not sure if there is (or should be) a hard-and-fast rule, but certainly in this case changing the hash SGTM. Otherwise, users toggling back and forth between build configurations would have to rebuild the modules each time.

Great, it looks like changing the module hash is acceptable. However, I'm not sure whether it's OK to make sanitizer feature mismatches errors for explicit modules. @aprantl suggests checking for language option mismatches early on instead of just relying on the module hash, but @rsmith mentioned:

I would expect this [sanitizer features] to be permitted to differ between an explicit module build and its use. (Ideally we would apply the sanitization settings from the module build to the code generated for its inline functions in that case, but that can wait.)

May 9 2017, 12:27 PM
dexonsmith added a comment to D32724: [Modules] Handle sanitizer feature mismatches when importing modules.

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

May 9 2017, 10:17 AM

May 3 2017

dexonsmith added a comment to D32838: [libcxx] Make __shared_weak_count VTable consistent across all build configurations.

Breaking the ABI of that configuration SGTM.

May 3 2017, 4:58 PM
dexonsmith closed D32836: CMake: Only add test/ subdirectory when it exists.
May 3 2017, 4:49 PM
dexonsmith added a comment to D32836: CMake: Only add test/ subdirectory when it exists.

Thanks for being flexible; committed in r302095. Maybe longer term we could add a CMake configuration for whether to include the tests, but naming it might be hard ;).

May 3 2017, 4:49 PM
dexonsmith committed rL302095: CMake: Only add test/ subdirectory when it exists.
CMake: Only add test/ subdirectory when it exists
May 3 2017, 4:47 PM
dexonsmith created D32836: CMake: Only add test/ subdirectory when it exists.
May 3 2017, 4:36 PM
dexonsmith accepted D32796: [Driver] Add a "-mmacos_version_min" option that's an alias for "-mmacosx_version_min".

LGTM.

May 3 2017, 8:11 AM

May 2 2017

dexonsmith accepted D32777: Remap metadata attached to global variables..

(I must have misclicked somewhere.)

May 2 2017, 8:39 PM
dexonsmith requested changes to D32777: Remap metadata attached to global variables..

This LGTM too.

May 2 2017, 8:38 PM

Apr 29 2017

dexonsmith accepted D31739: Add markup for libc++ dylib availability.

This LGTM, and it's liable to bitrot if it hangs out here any longer. We can always iterate in tree if we find a better way to organize the markup and/or tests.

Apr 29 2017, 9:48 PM

Apr 28 2017

dexonsmith committed rL301719: Fuzzer: Mark test/cxxstring.test UNSUPPORTED: windows.
Fuzzer: Mark test/cxxstring.test UNSUPPORTED: windows
Apr 28 2017, 5:12 PM
dexonsmith added a comment to D32648: Remove line and file from DINamespace.

Upgrade LGTM. (I'm not planning to review the rest, but let me know if you need me to take a closer look.)

Apr 28 2017, 10:28 AM
dexonsmith added inline comments to D32648: Remove line and file from DINamespace.
Apr 28 2017, 9:48 AM

Apr 27 2017

dexonsmith closed D31856: Headers: Make the type of SIZE_MAX the same as size_t.
Apr 27 2017, 3:22 PM
dexonsmith added a comment to D31856: Headers: Make the type of SIZE_MAX the same as size_t.

rsmith: I just noticed you still have a red "X" here (since Phab won't let me "close"). I think I addressed your comments, but let me know if you want me to revert until you have time to look.

Apr 27 2017, 3:09 PM
dexonsmith accepted D31856: Headers: Make the type of SIZE_MAX the same as size_t.

Thanks for the reviews! Committed in r301593.

Apr 27 2017, 3:04 PM
dexonsmith committed rL301593: Headers: Make the type of SIZE_MAX the same as size_t.
Headers: Make the type of SIZE_MAX the same as size_t
Apr 27 2017, 3:02 PM
dexonsmith closed D32263: Preprocessor: Suppress -Wnonportable-include-path for header maps.

Fixed in r301592.

Apr 27 2017, 2:55 PM
dexonsmith committed rL301592: Preprocessor: Suppress -Wnonportable-include-path for header maps.
Preprocessor: Suppress -Wnonportable-include-path for header maps
Apr 27 2017, 2:55 PM
dexonsmith added a comment to D32603: Build the Apple-style stage2 with modules and full debug info.

(Both changes SGTM, but I doubt you're looking for my advice on the CMake logic.)

Apr 27 2017, 11:21 AM

Apr 26 2017

dexonsmith added a comment to D32576: [Modules] Improve diagnostics for incomplete umbrella.

Oh, and it would be nice to split out Preprocessor::diagnoseMissingHeaderInUmbrellaDir in a separate NFC commit ahead of time.

Apr 26 2017, 7:27 PM
dexonsmith accepted D32576: [Modules] Improve diagnostics for incomplete umbrella.

LGTM after a couple of changes inline.

Apr 26 2017, 7:26 PM
dexonsmith committed rL301508: Darwin: Define __STDC_NO_THREADS__ on Darwin targets.
Darwin: Define __STDC_NO_THREADS__ on Darwin targets
Apr 26 2017, 7:00 PM

Apr 22 2017

dexonsmith added inline comments to D31856: Headers: Make the type of SIZE_MAX the same as size_t.
Apr 22 2017, 11:27 AM

Apr 21 2017

dexonsmith added inline comments to D31856: Headers: Make the type of SIZE_MAX the same as size_t.
Apr 21 2017, 6:12 PM
dexonsmith updated the diff for D31856: Headers: Make the type of SIZE_MAX the same as size_t.

Thanks Eli and Richard.

Apr 21 2017, 6:10 PM
dexonsmith closed D31561: cmath: Skip Libc for integral types in isinf, etc..

Committed in r301060.

Apr 21 2017, 4:28 PM
dexonsmith committed rL301060: cmath: Skip Libc for integral types in isinf, etc..
cmath: Skip Libc for integral types in isinf, etc.
Apr 21 2017, 4:28 PM
dexonsmith added a comment to D31561: cmath: Skip Libc for integral types in isinf, etc..

Since I haven't heard from Marshall and Hal's fine with the less-future-proof std::is_floating_point, I'll commit that and we can iterate in tree. Just running tests.

Apr 21 2017, 4:26 PM
dexonsmith added a comment to D32263: Preprocessor: Suppress -Wnonportable-include-path for header maps.

s/Eli/Eric/

Apr 21 2017, 12:38 PM
dexonsmith updated the diff for D32263: Preprocessor: Suppress -Wnonportable-include-path for header maps.

Thanks for the review, Eli.

Apr 21 2017, 12:37 PM

Apr 20 2017

dexonsmith added a comment to D31856: Headers: Make the type of SIZE_MAX the same as size_t.

It looks like __INTPTR_TYPE__ was introduced in r64495 in order to help define intptr_t, and then they diverged in r89237.

Apr 20 2017, 7:17 PM
dexonsmith updated the diff for D31856: Headers: Make the type of SIZE_MAX the same as size_t.

Here's an updated patch that uses __SIZE_MAX__ and also handles the other pointer-like integers.

Apr 20 2017, 7:02 PM
dexonsmith added a comment to D31856: Headers: Make the type of SIZE_MAX the same as size_t.

Hmm... presumably, this test should pass:

Apr 20 2017, 6:59 PM
dexonsmith added a comment to D31561: cmath: Skip Libc for integral types in isinf, etc..

Marshall, are you fine with figuring this out in-tree? Or should I revert to using std::is_floating_point for now?

Apr 20 2017, 5:11 PM
dexonsmith added a comment to D31856: Headers: Make the type of SIZE_MAX the same as size_t.

Thanks for the review. __SIZE_MAX__ makes sense to me; and better even if not for the #if requirement. I'll do that for both SIZE_MAX and PTRDIFF_MAX when I get a chance.

Apr 20 2017, 4:35 PM

Apr 19 2017

dexonsmith abandoned D3125: Phabricator review: blockfreq: Rewrite block frequency analysis.

This was committed in some form a long time ago.

Apr 19 2017, 7:08 PM
dexonsmith added a comment to D31561: cmath: Skip Libc for integral types in isinf, etc..

Eric: somehow I never got an email notification for your review :/. If Marshall is okay with the current state, I'll document that before commit.

Apr 19 2017, 7:08 PM
dexonsmith added a comment to D31561: cmath: Skip Libc for integral types in isinf, etc..

Marshall, that's what I assumed originally, but I figured Hal had some non-standard-but-worth-supporting use case in mind.

Apr 19 2017, 7:06 PM
dexonsmith added a comment to D31856: Headers: Make the type of SIZE_MAX the same as size_t.

Ping.

Apr 19 2017, 7:05 PM
dexonsmith created D32263: Preprocessor: Suppress -Wnonportable-include-path for header maps.
Apr 19 2017, 7:01 PM

Apr 14 2017

dexonsmith committed rL300380: Modules: Do not serialize #pragma pack state.
Modules: Do not serialize #pragma pack state
Apr 14 2017, 5:20 PM