Loosen/improve AtomicFlag semantics.

base/synchronization/atomic_flag.h

 // Copyright (c) 2011 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.
 
 #ifndef BASE_SYNCHRONIZATION_ATOMIC_FLAG_H_
 #define BASE_SYNCHRONIZATION_ATOMIC_FLAG_H_
 
 #include "base/atomicops.h"
 #include "base/base_export.h"
 #include "base/macros.h"
-#include "base/threading/thread_checker.h"
+#include "base/sequence_checker.h"
 
 namespace base {
 
 // A flag that can safely be set from one thread and read from other threads.
 //
 // This class IS NOT intended for synchronization between threads.
 class BASE_EXPORT AtomicFlag {
  public:
   AtomicFlag();
   ~AtomicFlag() = default;
 
-  // Set the flag. May only be called on the thread which created the object.
+  // Set the flag. Must always be called from the same sequence.
   void Set();
 
-  // Returns true iff the flag was set.
+  // Returns true iff the flag was set. If this returns true, the current thread
+  // is guaranteed to be synchronized with all memory operations on the sequence
+  // which invoked Set() up until at least the first call to Set() on it.
   bool IsSet() const;
 
   // Resets the flag. Be careful when using this: callers might not expect
-  // IsSet() to return false after returning true once.
-  void UnsafeResetForTesting();
+  // IsSet() to return false after returning true once. After a call to
+  // UnsafeReset(), the AtomicFlag is safe and regains it's initial properties

[danakj, 2016/08/01 17:14:56]

I dont like the comment saying it becomes safe by calling an unsafe method.

[gab, 2016/08/01 17:46:45]

Well it's "unsafe" unless external measures (as described) are taken to make it
safe.

But okay to keep it as ForTesting for now.

+  // (Set() may even bind to a new sequence) if and only if all threads using it
+  // are synchronized past the call to UnsafeReset().
+  void UnsafeReset();

[fdoray, 2016/08/01 16:15:51]

Keep ForTesting() if you no longer need to call this outside of tests.

[danakj, 2016/08/01 17:14:56]

I agree.

 
  private:
   base::subtle::Atomic32 flag_ = 0;
-  ThreadChecker thread_checker_;
+  SequenceChecker set_sequence_checker_;

[fdoray, 2016/07/29 20:41:37]

Given that there are no guarantees about the synchronization of memory
operations made after the first call to Set(), I'm not convinced that
SequenceChecker is useful (Why should we disallow calls to Set() from different
sequences if IsSet() callers cannot really assume anything about memory
synchronization when multiple calls to Set() are possible?)

I'm not strongly against keeping the SequenceChecker if you think it's useful.

[gab, 2016/08/01 15:08:29]

I think it's useful because the IsSet() callers can at least know which sequence
they're synchronizing with. If there are multiple callers to Set() than there
might as well not be barriers as the IsSet() callers can't assume anything about
the state of memory anyways (they're synced with "a" thread/sequence but don't
know which one...).

[fdoray, 2016/08/01 16:15:51]

If there are multiple callers to Set(), IsSet() callers can't assume anything
about the state of memory even if there is a SequenceChecker. Because of that, I
don't think that making sure that all calls to Set() occur on the same sequence
is useful.

However, SequenceChecker prevents Set() from being called from a PARALLEL task,
which is nice because it forces users to think about memory synchronization.

 
   DISALLOW_COPY_AND_ASSIGN(AtomicFlag);
 };
 
 }  // namespace base
 
 #endif  // BASE_SYNCHRONIZATION_ATOMIC_FLAG_H_