Date: prev next · Thread: first prev next last
2019 Archives by date, by thread · List index


Chiming in a little late.

Am 15.04.19 um 09:50 schrieb Kaganski Mike:
On 15.04.2019 10:44, Adrien Ollier wrote:
OutputDevice::AcquireGraphics() is a pure virtual function, so we cannot 
know in the general case what it does.
And what is written is not really equivalent to what you wrote because 
if AcquireGraphics() is false, there is not a second check for 
mpGraphics. That's why it is an issue.

AcquireGraphics() is not just "any function". Despite someone could of 
course implement it to play poker, its purpose is to acquire mpGraphics 
and return if it succeeded. Failing that is programmer's error breaking 
contract. There's no use to introduce that kind of checks here. Please 
see existing implementations, like VirtualDevice::AcquireGraphics() in 
vcl/source/gdi/virdev.cxx.


This is not the nicest API - probably the bool function should be renamed, or
also return the SalGraphics* for consistency; but then naming is hard...

$ git grep AcquireGraphics include/ vcl/inc/
include/vcl/outdev.hxx:    virtual bool AcquireGraphics() const = 0;
vcl/inc/salframe.hxx:    virtual SalGraphics* AcquireGraphics() = 0;
vcl/inc/salprn.hxx:    virtual SalGraphics* AcquireGraphics() = 0;
vcl/inc/salvd.hxx:    virtual SalGraphics* AcquireGraphics() = 0;

I was interested in the fact that the bool variant is const, which is a lie.
Looking at the code it's not true and in fact ~ half of OutputDevice's member
variables are mutable. That's probably done to do some lazy init for other const
functions, quite probably all the const font based ones. Don't know if it buys a
lot of time for some usage, or should be factored out somehow... Anyway, I did:

$ git log -G AcquireGraphics include/vcl/outdev.hxx

commit 665bc42d7504c3896a485c8bab17b8eff7d119df
Author: Chris Sherlock <chris.sherlock79@gmail.com>
Date:   Wed Apr 23 00:57:50 2014 +1000

    Rename VCL's ImplInitGraphics to AcquireGraphics

    Turns out, we don't try to initialize a graphics context, much less
    *acquire* one. e.g. Window instances can have many frames (subwindows),
    of which one some are really being used at any time so we try to
    "steal" one of the graphics contexts from the frame to use ourself,
    later on that frame will steal it from someone else, etc.

    Change-Id: I66d5dbb7015301bc2d2be51627061c91e1f2ee5d

This has nothing to do with the const, but explains the clash - kind of.

Normally the bool variants will - at some point - call
  mpGraphics = ...->AcquireGraphics();

so much about the background stuff I was looking into.
Back to the original mail.

$ git grep "mpGraphics.*AcquireGraphics" vcl/
if ( mpGraphics || AcquireGraphics() )
if ( !mpGraphics && !AcquireGraphics() )
... 69 times

shows this idom is used some times in vcl. And IMHO it should be everywhere
reduced to:

if ( !AcquireGraphics() )
or
if ( AcquireGraphics() )

All the three bool variants start with:

$ git grep "bool.*::AcquireGraphics" vcl
vcl/source/gdi/print.cxx:bool Printer::AcquireGraphics() const
vcl/source/gdi/virdev.cxx:bool VirtualDevice::AcquireGraphics() const
vcl/source/window/window.cxx:bool Window::AcquireGraphics() const

bool *::AcquireGraphics() const
{
    DBG_TESTSOLARMUTEX();

    if ( mpGraphics )
        return true;

The additional mutex check in debug builds is nice too.

For security I would rename the variants to Do / Impl and add an inline to the
OutputDevice header like so the "header" can't be stripped accidently:

inline bool OutputDevice::AcquireGraphics() const
{
    DBG_TESTSOLARMUTEX();

    if ( mpGraphics )
        return true;

    return DoAcqureGraphics();
}

That also prevents the function call, if that really matters here.

Anyone want to make this an easy hack?
Revert the function name to ImplInitGraphics?

Comments?

Jan-Marek

Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.