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

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

Also remove the superfluous "to" field in PolicyCompiler::Range,
and fix the next-syscall-number calculation so we can remove the
loop in SyscallIterator::operator++() (nee SyscallIterator::Next()).

BUG=414363
Committed: https://crrev.com/2e58094a81b0291aa2b6f12c99cb2653c361d8a0
Cr-Commit-Position: refs/heads/master@{#300633}

[mdempsky, 2014-10-21 22:04:08]

mdempsky@chromium.org changed reviewers:
+ jln@chromium.org

[mdempsky, 2014-10-21 22:04:09]

(Now that "enum class" is approved, I can finally mail out this CL.)

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

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/syscall_iterator.h:46: SANDBOX_EXPORT SyscallIterator
begin(SyscallSet set);
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?

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/syscall_iterator.h:59: SyscallIterator& operator++();
I don't think we need / use the return value. Let's make it a const reference
instead?

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/syscall_iterator.h:69: bool done_;
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]

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/syscall_iterator.h:46: SANDBOX_EXPORT SyscallIterator
begin(SyscallSet set);
On 2014/10/21 23:31:24, jln wrote:
> 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?

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.

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/syscall_iterator.h:59: SyscallIterator& operator++();
On 2014/10/21 23:31:24, jln wrote:
> I don't think we need / use the return value. Let's make it a const reference
> instead?

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.

https://chromiumcodereview.appspot.com/659723002/diff/40001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/syscall_iterator.h:69: bool done_;
On 2014/10/21 23:31:24, jln wrote:
> 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)?

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.

[mdempsky, 2014-10-22 02:18:47]

So after arguing against making SyscallSet into a proper class, I decided to at
least experiment with what it would look like.  It does end up with some more
boiler plate than I'd like, but overall it's no bad, and does have some
advantages (provides a better place to put "IsValid", allows
SyscallSet::Iterator instead of SyscallIterator, and overall probably a bit more
obvious readability wise).

I left the "bool done" argument in the constructor for Iterator, but I've also
made it private, so I think that's more reasonable.  (Also, I couldn't come up
with a good name for "enum class XXX { BEGIN, END }".)

PTAL

[jln (very slow on Chromium), 2014-10-22 02:34:43]

lgtm, I think it looks much better!

https://codereview.chromium.org/659723002/diff/60001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://codereview.chromium.org/659723002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall_iterator.h:56: SyscallSet(Set set) : set_(set)
{}
explicit

[mdempsky, 2014-10-22 04:00:56]

https://codereview.chromium.org/659723002/diff/60001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/syscall_iterator.h (right):

https://codereview.chromium.org/659723002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/syscall_iterator.h:56: SyscallSet(Set set) : set_(set)
{}
On 2014/10/22 02:34:43, jln wrote:
> explicit

Done.  (Ugh, I always forget that.)

[mdempsky, 2014-10-22 04:01:01]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-10-22 04:01:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/659723002/80001

[commit-bot: I haz the power, 2014-10-22 04:46:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-22 04:46:47]

Patchset 5 (id:??) landed as
https://crrev.com/2e58094a81b0291aa2b6f12c99cb2653c361d8a0
Cr-Commit-Position: refs/heads/master@{#300633}