518756 - want helper class(es) for saving and restoring values

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for mozilla::AutoRestore

Using classes for saving and restoring values is often helpful for guaranteeing invariants in functions or blocks that have many exit points (return, break, continue).  It's also essential for writing exception-safe code.

We often use classes for complex operations, but I don't know of any use in our tree for simple values.  I think we should have helper classes for doing this.

Here's a proposal for a first such class:  template<class T> AutoRestore (in the mozilla namespace) would have a constructor with a T& argument.  It would save the value at that location in its constructor and restore that value in its destructor.

The attached patch implements this proposal.

I think the header file containing such a class would also be an appropriate location for the XPCOM version of bug 518633 (which has a patch to JS); I expect that would find wide use in content and dom code pretty quickly.

(Another save/restore class that might be useful is something I might call AutoToggle:  AutoToggle(T& aLocation, T aFrom, T aTo) would have a constructor that asserts that aLocation == aFrom, and then assigns aLocation = aTo; the destructor would assign aLocation = aFrom.)


I'd be interested in feedback on the concepts, naming, etc.

Comment 1

[Benjamin Smedberg]

Can you provide an example of how saving a value and restoring it like that would be used?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/6b5eff0db0a6/layout-assertions has an example at the very end of the patch:

+  AutoRestore<PRBool> savePainting(nsContentUtils::sIsPaintingTree);
+  nsContentUtils::sIsPaintingTree = PR_TRUE;

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And thoughts on the AutoToggle proposal in comment 0 are also welcome.  (The point of that would be that it should avoid having to save any state; it just has the values to set and restore in code.)

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #2)
> http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/6b5eff0db0a6/layout-assertions
> has an example at the very end of the patch:
> 
> +  AutoRestore<PRBool> savePainting(nsContentUtils::sIsPaintingTree);
> +  nsContentUtils::sIsPaintingTree = PR_TRUE;

I see a lot of potential for this idiom, but I'm concerned that its usefulness is much diminished for variables that can be manipulated by means that the AutoRestore object doesn't know about.  In other words, if a developer chooses to use AutoRestore, he should have to commit to manipulating the variable only via AutoRestore operations (potentially multiply nested), and otherwise he should regard the variable as read-only.  One way to enforce the const-ness of the variable is to have two AutoRestore constructors, one taking a T& (unusable) and the other a const T& (ok):

  template <typename T>
  class AutoRestore
  {
    const T mOrig;
    T* const mPtr;
  public:
    AutoRestore(const T& lval, const T& rval)
      : mOrig(lval)
      , mPtr(const_cast<T*>(&lval))
    {
      *mPtr = rval;
    }
    AutoRestore(T& lval, const T&)
      : mOrig(lval)
      , mPtr(0)
    {
      First_AutoRestore_parameter_must_be_const(lval);
    }
    ~AutoRestore() { *mPtr = mOrig; }
  };

This will compile as long as the T& constructor is never used, and gives a relatively meaningful error message if you accidentally try to use it.  The protection is easily circumvented, of course, but at least it has to be circumvented deliberately.

Comment 5

[Benjamin Smedberg]

bnewman, the thought of initializing a non-const pointer with a const reference gives me the willies: I suspect that compilers might choose to optimize away checks across that call. And I think that this idiom as it stands is probably useful, even if we could also use a more advanced version in the future.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #5)
> bnewman, the thought of initializing a non-const pointer with a const reference
> gives me the willies: I suspect that compilers might choose to optimize away
> checks across that call.

Right, of course, const references are quirky.  If we made the constructor parameter a const T*, my intent would be preserved.

> And I think that this idiom as it stands is probably
> useful, even if we could also use a more advanced version in the future.

I don't disagree, and I'm not trying block this version from landing.  It just seems to me that the use of AutoRestore implies that the variable-to-be-restored is expected not to make any additional transitions unless those transitions will be undone by the time the variable is restored.  If the variable does make a transition that's intended to persist, the restoration step will blindly clobber the new value.  If you're confident the variable should not make any persistent transitions, that's an assumption it would be nice to enforce, one way or another (by no means necessarily the way I've suggested).

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> It just seems to me that the use of AutoRestore implies that the
> variable-to-be-restored is expected not to make any additional transitions
> unless those transitions will be undone by the time the variable is restored. 

It doesn't imply that to me; I wonder what others think.  If something like this is desirable, however, I think it should be in another helper class.  What's in the current patch is still quite useful.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

AutoRestore landed: http://hg.mozilla.org/mozilla-central/rev/f1d388af67bf

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for mozilla::AutoToggle

I should probably test this before requesting review, but I wrote it a while ago.

Comment 10

[pete]

AutoRestore is ugly:

1. It requires you to specify a type.
2. It requires you to name a dummy variable name.
3. It also has some whacked out guard notification code that tries to (AT RUNTIME!!) detect creating an AutoRestore with no dummy variable name.

Fortunately there is a better solution to all 3 problems based on the trick that C++ requires the destructor of foo:

const Base &foo = Derived();

to run at the end of scope. Which leads to the following simpler implementation:


struct AutoRestoreBase {
};

template<class T>
struct AutoRestore : AutoRestoreBase {
	T &mLocation;
	T mValue;

	AutoRestore(T &aRef) : mLocation(aRef), mValue(aRef)
	{}

	AutoRestore(T &aRef, const T &newVal) : mLocation(aRef), mValue(aRef)
	{
		mLocation = newVal;
	}

	~AutoRestore() { swap(mLocation, mValue); }
};

template<class T>
AutoRestore<T> autoRestoreWrapper(T &aRef)
{
	return AutoRestore<T>(aRef);
}

template<class T>
AutoRestore<T> autoRestoreWrapper(T &aRef, const T &newVal)
{
	return AutoRestore<T>(aRef, newVal);
}

#define AUTO_RESTORE const AutoRestoreBase &aut0r3st0r3_##__LINE__ = autoRestoreWrapper



So instead of:

AutoRestore<bool> savePainting(mIsPainting);
mIsPainting = PR_TRUE;
...

We can write either:

AUTO_RESTORE(mIsPainting);
mIsPainting = PR_TRUE;

or even better:

AUTO_RESTORE(mIsPainting, PR_TRUE);

Notice that AUTO_RESTORE here consolidates the functionality of both AutoRestore and AutoToggle (except the old value matches assertion).

There are a few things that suck about my implementation like no move construction and that it's not a function macro with __VA_ARGS__, but that depends on the compilers you are willing to drop support for.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But that doesn't mean the result is readable to those who haven't approached the code before.  Heavy use of macros tends to make it a lot harder for new contributors to understand code.

Comment 12

[pete]

ALLCAPS_MACROS have the advantage of mentally alerting people reviewing the code that funky stuff is going on here, which is exactly what we want when using auto restore.

The current AutoRestore implementation makes code look like it's initializing an unused local variable which is what triggered me to investigate the AutoRestore code to see if somebody used it by mistake.

Comment 13

[Luke Wagner [:luke]]

(In reply to pete from comment #12)
> ALLCAPS_MACROS have the advantage of mentally alerting people reviewing the
> code that funky stuff is going on here

But funky stuff isn't going on; it's all quite simple until you make it scary with a macro.

Comment 14

[Jeff Walden [:Waldo]]

The Auto* prefix didn't give a vague clue that the "unused" local was deliberate?  I had thought Auto* was fairly idiomatic among moderately skilled C++ programmers, what with auto_ptr and such.  Not ideal, perhaps, but macros seem at least as "whacked", especially as an auto can be debugged through without going through any hidden code as a macro would have.  And you still have the auto idiom in your trick, so isn't it about as un-clear too?

Comment 15

[Andrew McCreight [:mccr8]]

A patch landed here back in 2009 as seen in comment 8. If somebody wants a new AutoToggle class in the future, they can file a new bug I suppose.