Skip to content

Commit

Permalink
Clean up my TODO comments in SmallMap and Time.
Browse files Browse the repository at this point in the history
The TODO in time.h was about removing time_t functions. When I wrote this I was optimistically thinking we wouldn't use time_t any more now that we have the awesome new Time class. This was fantasy so the TODO is removed.

SmallMap uses ManualConstructor. The TODO was that these could be removed when we have C++11 unions. This is true, but the syntax looked worse. There was a use in ConvertToRealMap that still required ManualConstructor for optimum allocations, so it seemed best to just keep using ManualConstructor.

However, the necessity of having the dummy value in the union due to an MSVC error seems no longer applicable. The dummy value is removed.

Review-Url: https://codereview.chromium.org/2363023002
Cr-Commit-Position: refs/heads/master@{#420626}
  • Loading branch information
brettw authored and Commit bot committed Sep 23, 2016
1 parent 7049607 commit 6b0d29e
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 10 deletions.
12 changes: 4 additions & 8 deletions base/containers/small_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -575,17 +575,13 @@ class SmallMap {

// We want to call constructors and destructors manually, but we don't
// want to allocate and deallocate the memory used for them separately.
// So, we use this crazy ManualConstructor class.
// So, we use this crazy ManualConstructor class. Since C++11 it's possible
// to use objects in unions like this, but the ManualDestructor syntax is
// a bit better and doesn't have limitations on object type.
//
// Since array_ and map_ are mutually exclusive, we'll put them in a
// union, too. We add in a dummy_ value which quiets MSVC from otherwise
// giving an erroneous "union member has copy constructor" error message
// (C2621). This dummy member has to come before array_ to quiet the
// compiler.
//
// TODO(brettw) remove this and use C++11 unions when we require C++11.
// union.
union {
ManualConstructor<value_type> dummy_;
ManualConstructor<value_type> array_[kArraySize];
ManualConstructor<NormalMap> map_;
};
Expand Down
2 changes: 0 additions & 2 deletions base/time/time.h
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,6 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
static Time NowFromSystemTime();

// Converts to/from time_t in UTC and a Time class.
// TODO(brettw) this should be removed once everybody starts using the |Time|
// class.
static Time FromTimeT(time_t tt);
time_t ToTimeT() const;

Expand Down

0 comments on commit 6b0d29e

Please sign in to comment.