On Feb 28, 2014, at 3:22 PM, mathog wrote:
On 28-Feb-2014 14:32, Jon Cruz wrote:
On Jan 23, 2014, at 4:34 PM, mathog wrote:
Inkscape::DrawingContext::Save save(dc);
Inkscape::DrawingContext::ContextSaver saver(dc);Yes, that is less confusing than the original.
That said, the construct is maintenance programmer hostile, since it is hiding something important for not much in return. This appears to be OOP anathema, because the second action must be explicitly coded, but for me the code would be much clearer it it just did the explicit:
cairo_save(ct); ... cairo_restore(ct);
Except that is C and not C++. Also, that does not help in the cases where an exception or early return (something I dislike) is encountered before the explicit cairo_restore(ct) call is made.
I think it is just that exceptions and RAII are common C++ approaches that one working in C++ needs to learn and recognize. That said, the names involved can definitely help out in order to signal more explicitly when RAII is in use.
To help with people unfamiliar with the RAII paradigm, I'll add some more explicit and visible info to our wiki, etc.
I understand (now) what Inkscape::DrawingContext::Save does, but it took some time to figure it out. Conversely, the cairo save/restore pair is fundamental, and that pair is commonly used in many other places in Inkscape (and hundreds of other programs). If it must be done with the existing methodology, the construct should say clearly what it does:
Inkscape::DrawingContext::CairoContextSaversaved_cairo_context(dc);
Definitely something to keep in mind as we clean things up. In this case it looks like the code itself is good, and that it only needed more explicit naming to make its intent clear.