Using Class wide types as factories, is this legit?

Using Class wide types as factories, is this legit?

[David Botton]

I have a pattern I have used in the past to make Classwides act as factories for objects.

This worked in versions of GNAT about a year and a half ago. Is this a compiler bug or bad Ada?

The key line is:

Row  : Active_Record'Class := Template;

This is meant to act as a factory for new objects matching the Template class by duplicating Template on assignment to Row.

Code:
   type Active_Record
     (Table_Name : access constant String;
      Connection : access          Gnoga.Server.Database.Connection'Class)
     is new Ada.Finalization.Controlled with
      record
         Is_New     : Boolean                               := True;
         Fields     : Gnoga.Types.Data_Array_Type;
         Values     : Gnoga.Types.Data_Map_Type;
      end record;

   function Find_All
     (Template : Active_Record'Class;
      Like     : String := "";
      Order_By : String := "")
      return Active_Record_Array.Vector
   is
      use Ada.Strings.Unbounded;
      use Ada.Strings;

      SQL  : Unbounded_String :=
        To_Unbounded_String ("select * from ") & Template.Table_Name.all;
   begin
      if Like /= "" then
         SQL := SQL & " where " & Like;
      end if;

      if Order_By /= "" then
         SQL := SQL & " order by " & Order_By;
      end if;

      declare
         RS   : Gnoga.Server.Database.Recordset'Class :=
           Template.Connection.Query (To_String (SQL));

         Rows : Active_Record_Array.Vector;
         Row  : Active_Record'Class := Template;
      begin
         Row.Is_New := False;

         while RS.Next loop
            Row.Values := RS.Field_Values;
            Rows.Append (Row);
         end loop;

         RS.Close;
         return Rows;
      end;
   end Find_All;


On run the following error appears:
db_active(14597,0x7fff775fb310) malloc: *** error for object 0xc00007fe659d040f: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6


Using this work around removes the error, but limits the use of derived types of Active_Record being created by the assignment:

so replacing - Row  : Active_Record'Class := Template;

with:
         Row  : Active_Record (Template.Table_Name, Template.Connection);

All works.

Re: Using Class wide types as factories, is this legit?

[Dmitry A. Kazakov]

On Mon, 22 Sep 2014 22:19:32 -0700 (PDT), David Botton wrote:

> I have a pattern I have used in the past to make Classwides act as factories for objects.

Why not to pass a factory object instead?

> This worked in versions of GNAT about a year and a half ago. Is this a
> compiler bug or bad Ada?

It is hard to say without seeing Initialize, Finalize and Adjust. I would
guess that Adjust is wrong, e.g. not making a deep copy of the things
killed in Finalize, which may lead to deallocating these twice in the first
variant of your code.

-- 
Regards,
Dmitry A. Kazakov
http://www.dmitry-kazakov.de

Re: Using Class wide types as factories, is this legit?

[David Botton]

> It is hard to say without seeing Initialize, Finalize and Adjust. I would
> 
> guess that Adjust is wrong, e.g. not making a deep copy of the things
> 
> killed in Finalize, which may lead to deallocating these twice in the first
> 
> variant of your code.

There is no adjust or finalize. Initialize looks like this:

   procedure Initialize (Object : in out Active_Record) is
      use Gnoga.Server.Database;
   begin
      if Object.Connection = null then
         raise Connection_Error;
      end if;

      Object.Fields :=
        Object.Connection.List_Fields_Of_Table
          (Object.Table_Name.all);
   end Initialize;

Re: Using Class wide types as factories, is this legit?

[Dmitry A. Kazakov]

On Tue, 23 Sep 2014 01:21:58 -0700 (PDT), David Botton wrote:

>> It is hard to say without seeing Initialize, Finalize and Adjust. I would
>> guess that Adjust is wrong, e.g. not making a deep copy of the things
>> killed in Finalize, which may lead to deallocating these twice in the first
>> variant of your code.
> 
> There is no adjust or finalize. Initialize looks like this:
> 
>    procedure Initialize (Object : in out Active_Record) is
>       use Gnoga.Server.Database;
>    begin
>       if Object.Connection = null then
>          raise Connection_Error;
>       end if;
> 
>       Object.Fields :=
>         Object.Connection.List_Fields_Of_Table
>           (Object.Table_Name.all);
>    end Initialize;

You should check controlled components as well.

For debugging, create a controlled type Foo with Initialize, Finalize,
Adjust printing some texts. Insert a member of this type at different
places of the record to determine which component causes crash. Type's
Finalize is called before finalization of components, type's Initialize is
called after initialization of components. Use GNAT.Most_Recent_Exception
to determine of the problem is secondary happens upon exception
propagation. Very often finalization issues hide the actual problem. Use
GNAT.Exception_Traces to find exceptions that silently kill the program.

Regarding your design. The rule of thumb is that controlled and other
copyable objects shall not have access discriminants.

-- 
Regards,
Dmitry A. Kazakov
http://www.dmitry-kazakov.de

Re: Using Class wide types as factories, is this legit?

[David Botton]

> Regarding your design. The rule of thumb is that controlled and other
> 
> copyable objects shall not have access discriminants.

rule of thumb? what would be your reasons to disallow?

I find creative uses for that as you can see.

I'll see if I can create a small reproducer first since if I can't then I know for sure is some place deeper in my code as you point out and which I've seen the case before.

David Botton

Re: Using Class wide types as factories, is this legit?

[Dmitry A. Kazakov]

On Tue, 23 Sep 2014 14:27:01 -0700 (PDT), David Botton wrote:

>> Regarding your design. The rule of thumb is that controlled and other
>> copyable objects shall not have access discriminants.
> 
> rule of thumb? what would be your reasons to disallow?

Because you could return a copy of a local object referencing to another
local object [and will have to fight a long losing battle against Ada's
accessibility checks].

A copyable object must either carry everything with or else use some
collection schema (AKA closure) to prevent premature destruction of their
outstanding parts, e.g. reference counted pointers etc.

-- 
Regards,
Dmitry A. Kazakov
http://www.dmitry-kazakov.de

Re: Using Class wide types as factories, is this legit?

[briot.emmanuel]

The implementation of controlled types has changed, so it is indeed possible that the Finalize calls have changed.
Note the following in the ARM 7.6.1, note 24:  "Finalize procedure should be designed to have no ill effect if it is applied a second time to the same object".
So here is is possible that Finalize is indeed called twice and your Finalize call is not protected.

You could also try that part of the code on linux, with valgrind

Re: Using Class wide types as factories, is this legit?

[David Botton]

> So here is is possible that Finalize is indeed called twice and your Finalize call is not protected.

There is no finalize, It contains two Ada.Containers, I haven't looked in to their code. If there would also be a GNAT bug then.