New issue
Advanced search Search tips

Issue 761570 link

Starred by 4 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-12-11
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug

Blocked on:
issue 612995



Sign in to add a comment

constexpr as much of base/time as is feasible/reasonable

Project Member Reported by m...@chromium.org, Sep 2 2017

Issue description

In order to avoid unintended run-time static initializers in client code, and to improve readability by eliminating the extra code + "enum integer" hacks, we should strive to make as much of base/time code constexpr-defined as possible.

This is now possible because all platforms' toolchains have a sufficient level of support (and without major showstopper bugs).

Some of this work has already been done by individual contributors, but much more can be done.

Also, note that some things, like base::TimeDelta::operator/() might not be migrated to constexpr until bug 612995 is resolved.
 
The NextAction date has arrived: 2017-12-11
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4ce5f788987cc9f11654f04599ce2d6eb0330f3

commit d4ce5f788987cc9f11654f04599ce2d6eb0330f3
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Jan 30 15:34:36 2018

constexpr simple TimeDelta methods.

(removed constexpr for In*() methods found in earlier patch sets
as the inlining incurred too big of a code size increase)

Bug: 761570
Change-Id: Iced714ec5541fa67fcafe0c0dbbc56c9353418c1
Reviewed-on: https://chromium-review.googlesource.com/883126
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532884}
[modify] https://crrev.com/d4ce5f788987cc9f11654f04599ce2d6eb0330f3/base/time/time.h
[modify] https://crrev.com/d4ce5f788987cc9f11654f04599ce2d6eb0330f3/base/time/time_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae87b7732fe762ef241ea55eff8f2127f2cd5a20

commit ae87b7732fe762ef241ea55eff8f2127f2cd5a20
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Jan 30 18:12:19 2018

constexpr TimeDelta::operator/

The mul/div operators are tricky. operator/ works when |a| is small
enough (in absolute value) to deterministically not risk overflow.

operator*() on the other hand doesn't because of a limitation in
__builtin_mul_overflow. We could do some template hacking to make
operator*(a) == operator/(1.0/a) when |a| is a constant expression.

Bug: 761570
Change-Id: I9906edfc049017ad19872e4a586b2ec675404850
Reviewed-on: https://chromium-review.googlesource.com/886344
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532947}
[modify] https://crrev.com/ae87b7732fe762ef241ea55eff8f2127f2cd5a20/base/time/time.h
[modify] https://crrev.com/ae87b7732fe762ef241ea55eff8f2127f2cd5a20/base/time/time_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/61bb11847577617e10b318dca90d79e599ab2ffb

commit 61bb11847577617e10b318dca90d79e599ab2ffb
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Feb 06 05:16:59 2018

Use TimeDelta::is_min() directly in time_unittest.cc instead of custom IsMin().

This was done for IsMax() in a prior pass, not sure why IsMin() was
missed but I don't see the benefit of having either.

R=miu@chromium.org

Bug: 761570
Change-Id: I42bf016c7309ba3495ac7914282be5f691e7090e
Reviewed-on: https://chromium-review.googlesource.com/897639
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534634}
[modify] https://crrev.com/61bb11847577617e10b318dca90d79e599ab2ffb/base/time/time_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6

commit db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6
Author: Max Morin <maxmorin@chromium.org>
Date: Mon Aug 27 09:45:24 2018

Make TimeDelta TriviallyCopyable for std::atomic.

We're having some awkwardness with working around the issue with stuff
like std::atomic<int64_t> and conversions back and forth. It would be
preferable to use std::atomic<TimeDelta> directly.

Removes the workaround added for  bug 635974 , since that was a bug in
vs 2015 and we use clang now.

Also fixes a couple of lint issues in time.h.

Bug:  635974 , 851959, 761570
Change-Id: I4683f960b0c348748c5f0aaf222da4dda40256ec
Reviewed-on: https://chromium-review.googlesource.com/1184781
Commit-Queue: Max Morin <maxmorin@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586219}
[modify] https://crrev.com/db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6/base/time/time.h
[modify] https://crrev.com/db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6/base/time/time_unittest.cc

Sign in to add a comment