SyscallIterator: support C++11 range-based for loops

sandbox/linux/seccomp-bpf/syscall_iterator.h

 // Copyright (c) 2012 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 SANDBOX_LINUX_SECCOMP_BPF_SYSCALL_ITERATOR_H__
 #define SANDBOX_LINUX_SECCOMP_BPF_SYSCALL_ITERATOR_H__
 
 #include <stdint.h>
 
 #include "base/macros.h"
 #include "sandbox/sandbox_export.h"
 
 namespace sandbox {
+class SyscallIterator;
 
 // Iterates over the entire system call range from 0..0xFFFFFFFFu. This
 // iterator is aware of how system calls look like and will skip quickly
 // over ranges that can't contain system calls. It iterates more slowly
 // whenever it reaches a range that is potentially problematic, returning
 // the last invalid value before a valid range of system calls, and the
 // first invalid value after a valid range of syscalls. It iterates over
 // individual values whenever it is in the normal range for system calls
 // (typically MIN_SYSCALL..MAX_SYSCALL).
-// If |invalid_only| is true, this iterator will only return invalid
-// syscall numbers, but will still skip quickly over invalid ranges,
-// returning the first invalid value in the range and then skipping
-// to the last invalid value in the range.
 //
 // Example usage:
-//   for (SyscallIterator iter(false); !iter.Done(); ) {
-//     uint32_t sysnum = iter.Next();
+//   for (uint32_t sysnum : SyscallSet::ALL) {
 //     // Do something with sysnum.
 //   }
-//
-// TODO(markus): Make this a classic C++ iterator.
+
+enum class SyscallSet {
+  // The "ALL" system call set contains both valid and invalid system
+  // call numbers.
+  ALL,
+
+  // The "INVALID_ONLY" system call set contains only valid system call
+  // numbers, but will still skip quickly over invalid ranges,
+  // returning the first invalid value in the range and then skipping
+  // to the last invalid value in the range.
+  INVALID_ONLY,
+
+  // TODO(mdempsky): Add ValidOnly.
+};
+
+// Begin returns an iterator pointing to the first element of the
+// specified system call set.
+SANDBOX_EXPORT SyscallIterator begin(SyscallSet set);

[jln (very slow on Chromium), 2014/10/21 23:31:24]

I find it a little strange to have a top-level begin() in the sandbox namespace.
Could we make SyscallSet a class instead and have begin as a class method?

[mdempsky, 2014/10/22 00:15:33]

We can, but it would add some more boiler plate, and top-level begin()/end()
methods are required in some places anyway (e.g., range-based for loops over
plain arrays), so I think it should be safe.

Still, I'll keep this in mind.

+
+// End returns an iterator pointing to the past-the-end element of the
+// specified system call set.
+SANDBOX_EXPORT SyscallIterator end(SyscallSet set);
+
+// SyscallIterator provides C++ forward iterator semantics for
+// traversing a system call set.
 class SANDBOX_EXPORT SyscallIterator {
  public:
-  explicit SyscallIterator(bool invalid_only)
-      : invalid_only_(invalid_only), done_(false), num_(0) {}
+  SyscallIterator(SyscallSet set, bool done);
 
-  bool Done() const { return done_; }
-  uint32_t Next();
+  uint32_t operator*() const;
+  SyscallIterator& operator++();

[jln (very slow on Chromium), 2014/10/21 23:31:24]

I don't think we need / use the return value. Let's make it a const reference
instead?

[mdempsky, 2014/10/22 00:15:33]

Technically, the C++ spec's requirements on iterators specifies that given
iterator "r" of type "X", that "++r" should have type "X&" (see
[iterator.iterators]).  I think things would still work in practice if we did
otherwise, so I can change it if you feel strongly; but my weak preference is to
stick with the standard interface.

+
+  friend bool operator==(const SyscallIterator& lhs,
+                         const SyscallIterator& rhs);
+
+  // TODO(mdempsky): Move elsewhere.
   static bool IsValid(uint32_t num);
 
  private:
-  bool invalid_only_;
+  SyscallSet set_;
   bool done_;

[jln (very slow on Chromium), 2014/10/21 23:31:24]

done_ feels weird.

Could we just replace it with bool IsDone() const { return 0 == num_; } and have
the constructor's parameter be a { BEGIN, END } enum instead (or a bool with a
better name)?

[mdempsky, 2014/10/22 00:15:33]

We can't simply get rid of "bool done_", because "num_ == 0" by itself might
indicate either we've just started or we've just wrapped around.  We could
increase num_ to an int64_t or something, but that doesn't feel much more
appealing.

   uint32_t num_;
+};
 
-  DISALLOW_IMPLICIT_CONSTRUCTORS(SyscallIterator);
-};
+SANDBOX_EXPORT bool operator!=(const SyscallIterator& lhs,
+                               const SyscallIterator& rhs);
 
 }  // namespace sandbox
 
 #endif  // SANDBOX_LINUX_SECCOMP_BPF_SYSCALL_ITERATOR_H__