C++ readability review

components/timers/rtc_alarm.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/timers/rtc_alarm.h"
 
 #include <sys/timerfd.h>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/threading/thread.h"
 
 namespace timers {
 
 namespace {
 // Helper class to ensure that the IO thread we will use for watching file
 // descriptors is started only once.
 class IOThreadStartHelper {
  public:
   IOThreadStartHelper() : thread_(new base::Thread("RTC Alarm IO Thread")) {
     CHECK(thread_->StartWithOptions(
         base::Thread::Options(base::MessageLoop::TYPE_IO, 0)));
   }
   ~IOThreadStartHelper() {}
 
   base::Thread& operator*() const { return *thread_.get(); }

[gromer, 2014/12/05 20:13:41]

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

"Define overloaded operators only if their meaning is obvious, unsurprising, and
consistent with the corresponding built-in operators."

It's surprising to me that a "helper" has * and -> operators, and it's not
obvious to me what its meaning should be. Could you provide a get() method
instead?

[Chirantan Ekbote, 2014/12/31 00:58:29]

This class exists because I needed a thread-safe way to ensure that the
dedicated IO thread for the timer is only started once and base::LazyInstance
provided a convenient guarantee that the constructor of the parameter class
would only be called once.

Otherwise I wanted this class to be as transparent as possible, which is why I
overloaded these operators.  While I admit that using
g_io_thread.Get()->SomeFunc() hides some details about what type is actually
being returned by the call to g_io_thread.Get(), it's not clear to me that
g_io_thread.Get().Get().SomeFunc() is any better.

[gromer, 2015/02/10 23:47:10]

You seem to have implemented exactly the fix I was going to suggest, namely make
this class inherit from Thread. I don't often recommend inheritance in place of
composition, but here it seems like the right choice.

 
   base::Thread* operator->() const { return thread_.get(); }
 
  private:
   scoped_ptr<base::Thread> thread_;
 };
 
 base::LazyInstance<IOThreadStartHelper> g_io_thread = LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
@@ skipping 47 matching lines @@
   // differently.  We queue the task here but we still go ahead and call
   // timerfd_settime with the zero delay anyway to cancel any previous delay
   // that might have been programmed.
   if (delay == base::TimeDelta()) {
     origin_message_loop_->PostTask(FROM_HERE,
                                    base::Bind(&RtcAlarm::OnTimerFired,
                                               scoped_refptr<RtcAlarm>(this),
                                               origin_event_id_));
   }
 
   // Make sure that we are running on a MessageLoopForIO.

[gromer, 2014/12/05 20:13:41]

This seems misleading- "Make sure X" to me implies that it's some sort of error
if X is not true, but IIUC it's perfectly fine if "we" are running on another
event loop; the code just needs to make sure ResetImpl runs on a
MessageLoopForIO. Maybe "Run ResetImpl on a MessageLoopForIO"?

[Chirantan Ekbote, 2014/12/31 00:58:29]

Acknowledged.

   if (!base::MessageLoopForIO::IsCurrent()) {
     g_io_thread.Get()->task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&RtcAlarm::ResetImpl,
                    scoped_refptr<RtcAlarm>(this),
                    delay,
                    origin_event_id_));
     return;
   }
 
   ResetImpl(delay, origin_event_id_);

[gromer, 2014/12/05 20:13:41]

If this were inside an else-block, it would be much clearer that it and the
above if-block are two different ways of accomplishing the same thing.

[Chirantan Ekbote, 2014/12/31 00:58:29]

Acknowledged.

 }
 
 void RtcAlarm::OnFileCanReadWithoutBlocking(int fd) {
   DCHECK_EQ(alarm_fd_, fd);
 
   // Read from the fd to ack the event.
   char val[sizeof(uint64_t)];
   base::ReadFromFD(alarm_fd_, val, sizeof(uint64_t));
 
   // Make sure that the parent timer is informed on the proper message loop.
@@ skipping 49 matching lines @@
 void RtcAlarm::OnTimerFired(int event_id) {
   DCHECK(origin_message_loop_->RunsTasksOnCurrentThread());
 
   // Check to make sure that the timer was not reset in the time between when
   // this task was queued to run and now.  If it was reset, then don't do
   // anything.
   if (event_id != origin_event_id_)
     return;
 
   if (parent_)
     parent_->OnTimerFired();

[gromer, 2014/12/05 20:13:41]

Isn't there a race condition where parent_ could be destroyed between these two
lines?

[Chirantan Ekbote, 2014/12/31 00:58:29]

No.  WeakPtrs can only be dereferenced and invalidated on the
SequencedTaskRunner on which they were created.  A SequencedTaskRunner is
essentially a FIFO of tasks.  So parent_ could be invalidated before
OnTimerFired is called and it could be invalidated after OnTimerFired returns
but it could not be invalidated in the middle of OnTimerFired.

 }
 
+
 }  // namespace timers