Page MenuHomePhabricator

Remove TimeValue usage from llvm/Support

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



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


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
25 ↗(On Diff #75008)

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

64 ↗(On Diff #75008)
labath added inline comments.Oct 18 2016, 12:02 PM
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.

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
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
thakis added a subscriber: thakis.Oct 24 2016, 7:18 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/ -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/ -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/ error: no viable conversion from 'TimePoint<(no argument)>' to 'TimePoint<std::chrono::microseconds>'

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

/Applications/ 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/ 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/ 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:

Please quickly take a look.

labath added inline comments.Nov 9 2016, 3:57 AM
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.