Tighten & check the BookmarkScope API contract a bit.

src/scanner.h

Index: src/scanner.h
diff --git a/src/scanner.h b/src/scanner.h
index 92588905ad609b7ea64d8c5c7c2007d3e55d409b..2afa5b27c1a61315ddab75a5c64e35929c1d929b 100644
--- a/src/scanner.h
+++ b/src/scanner.h
@@ -359,18 +359,35 @@ class Scanner {
   // Scoped helper for a re-settable bookmark.
   class BookmarkScope {
    public:
-    explicit BookmarkScope(Scanner* scanner) : scanner_(scanner) {
+    explicit BookmarkScope(Scanner* scanner)
+        : scanner_(scanner), my_bookmark_(false) {
       DCHECK_NOT_NULL(scanner_);
     }
-    ~BookmarkScope() { scanner_->DropBookmark(); }
+    ~BookmarkScope() {
+      if (my_bookmark_) scanner_->DropBookmark();

[rossberg, 2015/08/20 15:45:12]

Hm, this _does_ change the semantics, doesn't it? It looks more correct, though.

[vogelheim, 2015/08/20 16:26:31]

Yes, good observation. But I think the case where this makes a difference
shouldn't happen in practice, so it shouldn't change behaviour overall.

And if the case does happen, this should fix a bug. :)  This is what I
originally meant to implement.

+    }
 
-    bool Set() { return scanner_->SetBookmark(); }
-    void Reset() { scanner_->ResetToBookmark(); }
-    bool HasBeenSet() { return scanner_->BookmarkHasBeenSet(); }
-    bool HasBeenReset() { return scanner_->BookmarkHasBeenReset(); }
+    bool Set() {
+      DCHECK(!my_bookmark_);
+      my_bookmark_ = scanner_->SetBookmark();
+      return my_bookmark_;
+    }
+    void Reset() {
+      DCHECK(my_bookmark_);
+      scanner_->ResetToBookmark();
+    }
+    bool HasBeenSet() {
+      DCHECK(my_bookmark_);
+      return scanner_->BookmarkHasBeenSet();
+    }
+    bool HasBeenReset() {
+      DCHECK(my_bookmark_);
+      return scanner_->BookmarkHasBeenReset();
+    }
 
    private:
     Scanner* scanner_;
+    bool my_bookmark_;  // Did this BookmarkScope set the current bookmark?
 
     DISALLOW_COPY_AND_ASSIGN(BookmarkScope);
   };