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 (183 w, 5 d)

Recent Activity

Tue, Aug 29

dexonsmith added a comment to D37281: Make DIExpression plain metadata and not an MDNode.

Yes, abandoning seems reasonable. I think the main idea of step 4 when I filed PR22780 was to reduce memory usage, but this didn't turn out to be a bottleneck.

Tue, Aug 29, 5:40 PM

Aug 25 2017

dexonsmith accepted D36669: Build LLVM with -Wstrict-prototypes enabled.

LGTM.

Aug 25 2017, 8:38 AM

Aug 23 2017

dexonsmith added a comment to D37075: Parse and print DIExpressions inline to ease IR and MIR testing.

Nice!

Aug 23 2017, 1:29 PM

Aug 21 2017

dexonsmith accepted D36556: [IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level.

LGTM! Thanks for reworking this.

Aug 21 2017, 2:20 PM

Aug 18 2017

dexonsmith requested changes to D36556: [IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level.
Aug 18 2017, 6:02 PM

Aug 17 2017

dexonsmith added a comment to D36556: [IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level.

Who changed the behaviour? Can we get them to review the behaviour of the upgrade?

Aug 17 2017, 11:50 AM

Aug 14 2017

dexonsmith accepted D36713: [libc++] Add a persistent way to disable availability.

LGTM.

Aug 14 2017, 3:29 PM

Aug 9 2017

dexonsmith accepted D36427: [libcxxabi][demangler] Improve representation of substitutions/templates.

I have a few more nitpicks; LGTM after you fix those.

Aug 9 2017, 10:43 AM

Aug 8 2017

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

Don't you need // UNSUPPORTED: c++98, c++03 since std::function is supported in C++11 only?

Aug 8 2017, 10:44 AM

Aug 7 2017

dexonsmith added a comment to D36427: [libcxxabi][demangler] Improve representation of substitutions/templates.

Oh, also found a couple of things you should likely split into prep commits to simplify this patch.

Aug 7 2017, 5:13 PM
dexonsmith requested changes to D36427: [libcxxabi][demangler] Improve representation of substitutions/templates.

This looks like a great improvement. I've littered the patch with nit-picks, but my main concern is that there aren't any unit tests for the new data structures. I wonder if there's a hacky way to set that up...

Aug 7 2017, 5:11 PM

Jul 28 2017

dexonsmith accepted D35951: Remove the obsolete "offset" parameter from @llvm.dbg.value.

The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.

I spot-checked the other testcases, and couldn't find any that had anything other than i64 0. Is that expected? Or can you point me at a non-trivial one?

We have been phasing out any remaining use of the offset field and replaced it with DIExpressions. This is also why the upgrade merely strips any dbg.value intrinsics with nonzero offsets. It's effectively dead code.

Jul 28 2017, 7:39 AM

Jul 27 2017

dexonsmith added a comment to D35951: Remove the obsolete "offset" parameter from @llvm.dbg.value.

The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.

Jul 27 2017, 7:42 PM
dexonsmith added a comment to D35951: Remove the obsolete "offset" parameter from @llvm.dbg.value.

Should there also be an update to LangRef?

For those living downstream I should also note that the IR testcase updates are not strictly necessary, since the auto upgrade code also works for assembler input, but I think it is less confusing to have the testcases use the up-to-date version of the intrinsics.

I feel like it would be better to error, and require the upgrade. Is that not practical for intrinsics?

LLParser::ValidateEndOfModule() always calls UpgradeCallsToIntrinsic(), therefore special-casing just the debug info intrinsics would be inconsistent.

Jul 27 2017, 7:38 PM
dexonsmith accepted D35159: [libcxxabi][demangler] Use an AST to represent the demangled name.

Use LLVM naming conventions for all the new stuff in this patch.

Jul 27 2017, 2:17 PM
dexonsmith added a comment to D35951: Remove the obsolete "offset" parameter from @llvm.dbg.value.

Should there also be an update to LangRef?

Jul 27 2017, 11:16 AM

Jul 18 2017

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

LGTM.

Jul 18 2017, 5:23 AM

Jul 16 2017

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?

Jul 16 2017, 8:05 PM

Jul 13 2017

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).

Jul 13 2017, 8:50 AM

Jul 12 2017

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

+mclow.lists

Jul 12 2017, 2:03 PM
dexonsmith added a reviewer for D34781: Introduce a MCReloc class: enderby.
Jul 12 2017, 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?

Jul 12 2017, 1:34 PM

Jul 9 2017

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?

Jul 9 2017, 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.
Jul 9 2017, 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.
Jul 9 2017, 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.

Jul 9 2017, 6:13 AM

Jul 6 2017

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

Jul 6 2017, 10:20 PM
dexonsmith committed rL307357: cmath: Support clang's -fdelayed-template-parsing.
cmath: Support clang's -fdelayed-template-parsing
Jul 6 2017, 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?

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

LGTM.

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

Ping!

Jul 6 2017, 1:49 PM

Jun 26 2017

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.

Jun 26 2017, 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.

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

LGTM.

Jun 26 2017, 9:57 AM

Jun 23 2017

dexonsmith created D34578: cmath: Support clang's -fdelayed-template-parsing.
Jun 23 2017, 3:56 PM
dexonsmith added inline comments to D34556: [libcxx] Annotate c++17 aligned new/delete operators with availability attribute.
Jun 23 2017, 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.

Jun 23 2017, 8:35 AM

Jun 22 2017

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.

Jun 22 2017, 11:32 AM

Jun 21 2017

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.
Jun 21 2017, 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.

Jun 21 2017, 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