bugGNU Octave - Bugs: bug #54882, fail to build with Qt 4.8: class...

 
 

bug #54882: fail to build with Qt 4.8: class QPointer<> has no member named 'clear'

Submitter:  Mike Miller <mtmiller>
Submitted:  Mon 22 Oct 2018 09:05:46 PM UTC
   
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 25 Oct 2018 06:11:57 PM UTC, comment #6: 

That builds with Qt 4.8 now, thank you!

Mike Miller <mtmiller>
Group Member
Wed 24 Oct 2018 07:55:15 PM UTC, comment #5: 
Torsten Lilge <ttl>
Group Member
Wed 24 Oct 2018 04:28:26 PM UTC, comment #4: 

You are right, the current code (clear) does not destroy the octave_cmd object. Obviously, I have misinterpreted the documentation. The following simple patch fixes the issue and is backward compatible


diff -r e9c24b5e8673 libgui/src/octave-cmd.cc
--- a/libgui/src/octave-cmd.cc  Thu Oct 11 18:46:05 2018 +0200
+++ b/libgui/src/octave-cmd.cc  Wed Oct 24 13:37:40 2018 +0200
@@ -178,7 +178,7 @@ namespace octave
             cmd_gp->execute (interp);
           }

-        cmd_gp.clear ();    // remove the original cmd pointer
+        delete cmd_gp;    // destroy the referred octave_cmd object
       }

     if (repost)  // queue not empty, so repost event for further processing


I am going to prepare a changeset.

Torsten Lilge <ttl>
Group Member
Wed 24 Oct 2018 05:53:58 AM UTC, comment #3: 

Thanks for picking this up. This isn't urgent, but it seems simple enough to make compatible.

For my part, I tried adding a debug destructor to the octave_cmd class, but it was never printed, either the way the code is now or with my attempts to fix this. So if your intent was that the object is deleted, that may not be happening with the 'clear' method as you expected.

Mike Miller <mtmiller>
Group Member
Wed 24 Oct 2018 04:45:50 AM UTC, comment #2: 

Mike, sorry for not having replied up to now. I am currently testing whether the command object to which the QPointer is referring is also destroyed as intended.

Torsten Lilge <ttl>
Group Member
Tue 23 Oct 2018 08:25:56 PM UTC, comment #1: 

It seems to me that simply deleting the call to 'clear' might also work, because the QPointer goes out of scope with the next line of code anyway, so the object it points to should be deleted implicitly.

So the following change should also be acceptable


--- a/libgui/src/octave-cmd.cc
+++ b/libgui/src/octave-cmd.cc
@@ -177,8 +177,6 @@ namespace octave

             cmd_gp->execute (interp);
           }
-
-        cmd_gp.clear ();    // remove the original cmd pointer
       }

     if (repost)  // queue not empty, so repost event for further processing


I've tested both of these and I don't see any ill effects, but since this is about memory management I'm not really sure that I can see any. I would really appreciate some feedback from someone who understands the ownership of the command queue and the command objects on the queue, or suggestions as to how to test whether either of these changes are safe.

Mike Miller <mtmiller>
Group Member
Mon 22 Oct 2018 09:05:46 PM UTC, original submission:  

In Qt 4.8, the QPointer class template does not have a method named 'clear'.

The following change (not yet tested) should be backwards compatible and equivalent:


--- a/libgui/src/octave-cmd.cc
+++ b/libgui/src/octave-cmd.cc
@@ -178,7 +178,7 @@ namespace octave
             cmd_gp->execute (interp);
           }

-        cmd_gp.clear ();    // remove the original cmd pointer
+        cmd_gp = nullptr;    // remove the original cmd pointer
       }

     if (repost)  // queue not empty, so repost event for further processing


Torsten, since you wrote the change that added a call to QPointer<>::clear, can you test or verify that this change is safe and equivalent to your intentions?

Mike Miller <mtmiller>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by ttl (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-25 mtmiller StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-10-24 ttl StatusPatch Submitted Ready For Test
    2018-10-24 mtmiller Carbon-CopyRemoved 80942 -
    2018-10-23 mtmiller StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code