Page MenuHomePhabricator

Remove TimeValue usage from llvm/Support
ClosedPublic

Authored by labath on Oct 18 2016, 8:15 AM.

Details

Summary

This is a follow-up to D25416. It removes all usages of TimeValue from
llvm/Support library (except for the actual TimeValue declaration), and replaces
them with appropriate usages of std::chrono. To fascilitate this, I have added
small utility functions for converting time points and durations into appropriate
OS-specific types (FILETIME, struct timespec, ...).

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 75008.Oct 18 2016, 8:15 AM
labath retitled this revision from to Remove TimeValue usage from llvm/Support.
labath updated this object.
labath added reviewers: zturner, mehdi_amini.
labath added a subscriber: llvm-commits.
mehdi_amini added inline comments.Oct 18 2016, 11:25 AM
include/llvm/Object/Archive.h
25 ↗(On Diff #75008)

It seems to indicate that there as still uses of TimeValue in here?

include/llvm/Support/CachePruning.h
64 ↗(On Diff #75008)
labath added inline comments.Oct 18 2016, 12:02 PM
include/llvm/Object/Archive.h
25 ↗(On Diff #75008)

Affirmative. llvm/Object still uses TimeValue. I am deliberately not trying to do this at once, but in smaller chunks. It was necessary to add this as Support/Filesystem.h no longer includes TimeValue.h.

include/llvm/Support/CachePruning.h
64 ↗(On Diff #75008)

= 0 will not work, as duration objects are not implicitly constructible from integer types. I'd have to write = std::chrono::seconds(0). After re-reading that section it's still not clear to me whether this is a permitted usage of brace-initialization. One could argue that duration is "constructed like an aggregate" (which contains only one member) and it does not do anything fancy.

mehdi_amini added inline comments.Oct 18 2016, 12:21 PM
include/llvm/Support/CachePruning.h
64 ↗(On Diff #75008)

Also in the same document:

Default member initializers (non-static data member initializers): N2756
Only use these for scalar members that would otherwise be left uninitialized. Non-scalar members generally have appropriate default constructors, and MSVC 2013 has problems when braced initializer lists are involved.
``

(We're removing the MSVC 2013 part right now, and still discussing the rule).

I don't know what was the intent/motivation here, so I'm not sure what's the right thing to do.
labath updated this revision to Diff 75116.Oct 19 2016, 1:17 AM

Use default constructor instead of brace initialization

zturner accepted this revision.Oct 19 2016, 9:55 AM
zturner edited edge metadata.
This revision is now accepted and ready to land.Oct 19 2016, 9:55 AM

This broke building on darwin:

thakis-macpro:llvm-build2 thakis$ ninja lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o
[1/1] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o
FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Support -I/Users/thakis/src/llvm-rw/lib/Support -Iinclude -I/Users/thakis/src/llvm-rw/include -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Werror=date-time -std=c++11 -fcolor-diagnostics -O3 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -mmacosx-version-min=10.11 -UNDEBUG -fno-exceptions -fno-rtti -MMD -MT lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o.d -o lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o -c /Users/thakis/src/llvm-rw/lib/Support/Path.cpp
In file included from /Users/thakis/src/llvm-rw/lib/Support/Path.cpp:1170:
/Users/thakis/src/llvm-rw/lib/Support/Unix/Path.inc:454:40: error: no viable conversion from 'TimePoint<(no argument)>' to 'TimePoint<std::chrono::microseconds>'

Times[0] = Times[1] = sys::toTimeVal(Time);
                                     ^~~~

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/chrono:750:29: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'TimePoint<>' (aka 'std::1::chrono::time_point<std::1::chrono::system_clock, std::1::chrono::duration<long long, std::1::ratio<1, 1000000000> > >') to 'const std::1::chrono::time_point<std::1::chrono::system_clock, std::1::chrono::duration<long long, std::1::ratio<1, 1000000> > > &' for 1st argument
class _LIBCPP_TYPE_VIS_ONLY time_point

^

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/chrono:750:29: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'TimePoint<>' (aka 'std::1::chrono::time_point<std::1::chrono::system_clock, std::1::chrono::duration<long long, std::1::ratio<1, 1000000000> > >') to 'std::1::chrono::time_point<std::1::chrono::system_clock, std::1::chrono::duration<long long, std::1::ratio<1, 1000000> > > &&' for 1st argument
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/chrono:772:13: note: candidate template ignored: disabled by 'enable_if' [with _Duration2 = std::1::chrono::duration<long long, std::1::ratio<1, 1000000000> >]

is_convertible<_Duration2, duration>::value
^

/Users/thakis/src/llvm-rw/lib/Support/Unix/Unix.h:94:70: note: passing argument to parameter 'TP' here
inline struct timeval toTimeVal(TimePoint<std::chrono::microseconds> TP) {

eg here: http://lab.llvm.org:8011/builders/lld-x86_64-darwin13/builds/14/steps/build_Lld/logs/stdio

Please quickly take a look.

labath added inline comments.Nov 9 2016, 3:57 AM
include/llvm/Support/CachePruning.h
64 ↗(On Diff #75008)

BTW, I have learned (the hard way) that the duration default constructor does not zero-initialize the object. I've committed rL286359, to address that.