tdf#99352: explicit ctoring of VclPtrs?

classic Classic list List threaded Threaded
7 messages Options
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

tdf#99352: explicit ctoring of VclPtrs?

Hi all,

so while poking around for tdf#99352, I created a local branch where most
contructions of VclPtr<> from a raw pointer have been replaced with an explicit
construction of a VclPtr<>. I havent used anything fancy (clang rewriter), just
some vim macros. I didnt catch all, but the majority of them I guess.

Question: Do we want that on master?

Having killed the remaining uses of operator=(someptr*) and then removing it
might make finding fishy vcl-foo much easier. Killing the remaining ones could
either be a bit-for-bit EasyHack or a something for clang plugin. So should I
push what I have on master? Im asking as I would need to rebase the patch over
other debug changes I did to find leaking VclPtrs<>, so asking here first
before doing that and pushing to gerrit.

Opinions?

Best,

Bjoern

FWIW: the changes are along the lines of:

-    mpPage = pPage;
+    mpPage = VclPtr<TabPage>(pPage);

with mpPage being a VclPtr<> and pPage being a raw pointer.

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: tdf#99352: explicit ctoring of VclPtrs?

Considering :
1) tdf#91310 (meta vclptr) has only 3 dependent bugs remaining (including 1 for which you've already submitted a commit to review), there shouldn't be too much extra work for backport in other branches.

2) https://wiki.documentfoundation.org/ReleasePlan indicates that 5.4 branch freeze is at end of April, so it gives more than 1 month (if you push the changes this week) to fix remaining pbs which could be due with these.

3) the fact that vclptr state is not final and announced at the beginning of it as a temporary step (see http://opengrok.libreoffice.org/xref/core/vcl/README.lifecycle, "what remains to be done" part)

=> +1 for me as QA + dev but not expert (at all!)

Julien
Noel Grandin-2 Noel Grandin-2
Reply | Threaded
Open this post in threaded view
|

Re: tdf#99352: explicit ctoring of VclPtrs?

In reply to this post by Bjoern Michaelsen


On 2017/03/07 1:58 PM, Bjoern Michaelsen wrote:
> FWIW: the changes are along the lines of:
>
> -    mpPage = pPage;
> +    mpPage = VclPtr<TabPage>(pPage);
>
> with mpPage being a VclPtr<> and pPage being a raw pointer.


I can't see how that would flush out any bugs. How could it go wrong? Whether it is null or not makes no difference, the
VclPtr<T> is happy either way.

What is more likely to flush out bugs (and what would make sberg very happy), would be to remove the current
auto-conversion-to-T* operator, since that would make explicit the places where we are passing around raw pointers
instead of VclPtr<T>

_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Michael Meeks-5 Michael Meeks-5
Reply | Threaded
Open this post in threaded view
|

Re: tdf#99352: explicit ctoring of VclPtrs?

In reply to this post by Bjoern Michaelsen
Hi Bjoern,

On 07/03/17 11:58, Bjoern Michaelsen wrote:
> so while poking around for tdf#99352, I created a local branch where most
> contructions of VclPtr<> from a raw pointer have been replaced with an explicit
> construction of a VclPtr<>. I havent used anything fancy (clang rewriter), just
> some vim macros. I didnt catch all, but the majority of them I guess.

        Ok ? =) sounds sensible to me. Ultimately - I'd love to see all of our
OutputDevice * sub-types replaced with VclPtr<>s everywhere.

> Question: Do we want that on master?

        Good question - the code looks a bit uglier ... ;-) ultimately my hope
would be that we would have not:

> -    mpPage = pPage;
> +    mpPage = VclPtr<TabPage>(pPage);

        But: mpPage = pPage - and pPage is defined as a VclPtr ;-) so - I
wonder if this is the right way to go if we want to re-write it back
again ultimately ?

        Would it be better to have a big easy-hack to cleanup all naked VclPtr
derived pointer types around the place ? initially we did just members
for lifecycle reasons - but it would make sense to do it wider.

        Let me CC Noel - who is really the expert here I think =)

        HTH,

                Michael..

--
[hidden email] <><, Pseudo Engineer, itinerant idiot
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: tdf#99352: explicit ctoring of VclPtrs?

In reply to this post by Noel Grandin-2
Hi,

On Tue, Mar 07, 2017 at 02:58:31PM +0200, Noel Grandin wrote:
> I can't see how that would flush out any bugs.

E.g. In a local debug build I added metadata on where a VclPtr was created (via
__FILE__, __LINE__) so that I could learn where they were leaked from without
hunting them down individually in the debugger.

> What is more likely to flush out bugs (and what would make sberg very
> happy), would be to remove the current auto-conversion-to-T* operator, since
> that would make explicit the places where we are passing around raw pointers
> instead of VclPtr<T>

Yeah. :/

But in the end dont we want to have it locked down both ways: no VclPtr to raw*
conversion, nor the opposite? Likely were one happens, the other will happen too.

Anyway, not something I have too strong emotions about, just wanted to know, if
anyone cares before I throw away the branch.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
sberg sberg
Reply | Threaded
Open this post in threaded view
|

Re: tdf#99352: explicit ctoring of VclPtrs?

On 03/07/2017 03:07 PM, Bjoern Michaelsen wrote:
> On Tue, Mar 07, 2017 at 02:58:31PM +0200, Noel Grandin wrote:
>> I can't see how that would flush out any bugs.
>
> E.g. In a local debug build I added metadata on where a VclPtr was created (via
> __FILE__, __LINE__) so that I could learn where they were leaked from without
> hunting them down individually in the debugger.

So if you want to make such instrumentation of VclPtr ctor, it's not
much hassle to add that instrumentation also to the from-raw-ptr
VclPtr::operator=?

>> What is more likely to flush out bugs (and what would make sberg very
>> happy), would be to remove the current auto-conversion-to-T* operator, since
>> that would make explicit the places where we are passing around raw pointers
>> instead of VclPtr<T>

Yes, that would be my thought too.  Looks more promising to invest
energy in that direction.
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

Re: tdf#99352: explicit ctoring of VclPtrs?

Hi,

On Tue, Mar 07, 2017 at 04:05:40PM +0100, Stephan Bergmann wrote:
> So if you want to make such instrumentation of VclPtr ctor, it's not much
> hassle to add that instrumentation also to the from-raw-ptr
> VclPtr::operator=?

__LINE__ and __FILE__ only work at the position of the assignment, which is not
trivial to grep/sed, while a explicit constructor is reasonably easy.


But I wasnt asking about what I did with it, just wondered if anyone else would
want to keep it for whatever reason. Doesnt seem so, so will be gc'ed soon.

Best,

Bjoern
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice