8

I've written a function that accepts a class type (T) and an interface type (I) and returns an interface (I) to the object (T). Here's the code.

interface

function CreateObjectInterface<T: Class, constructor; I: IInterface>(
  out AObject: TObject): I;

...

implementation

function TORM.CreateObjectInterface<T, I>(out AObject: TObject): I;
begin
  AObject := T.Create;

  if not Supports(AObject, GetTypeData(TypeInfo(I))^.Guid, Result) then
  begin
    AObject.Free;
    AObject := nil;

    raise EORMUnsupportedInterface.CreateFmt(
      'Object class "%s" does not support interface "%s"',
      [AObject.ClassName, GUIDToString(GetTypeData(TypeInfo(I))^.GUID)]
    );
  end;
end;

The function works as expected with no memory leaks or other undesirables.

Are there other ways to achieve the same result?

norgepaul
  • 6,013
  • 4
  • 43
  • 76
  • 5
    I'm not sure that this question is appropriate here. Perhaps code review would be a better site. Your final question is certainly opinion based. First two questions. 1. Yes, this is OK. 2. No, no problems. – David Heffernan Mar 18 '15 at 10:15
  • Rephrase last question to "are there other ways to achieve the same result?" – LU RD Mar 18 '15 at 10:18
  • @LURD - I've rephrased the question – norgepaul Mar 18 '15 at 10:21
  • OK, turns out my analysis was wrong. Well done Stefan. – David Heffernan Mar 18 '15 at 10:33
  • 3
    @DavidHeffernan I see no close votes, and I see six upvotes and no downvotes, so apparently this is appropriate here. – Simon Forsberg Mar 18 '15 at 10:59
  • @SimonAndréForsberg I disagree. Which is fine. – David Heffernan Mar 18 '15 at 11:05
  • @DavidHeffernan I am just as surprised as you are. People tend to be very downvoting and close-voting on SO for questions like this, I don't know why this hasn't been. There is some overlap between SO and CR though. If you think this is off-topic for SO, then vote to close. (Just *don't* VTC as "would be better on CR") – Simon Forsberg Mar 18 '15 at 11:07
  • @SimonAndréForsberg This is not just some bulk custom code for review, but rather short snippet of code that can be useful to others too. – Dalija Prasnikar Mar 18 '15 at 15:21
  • 2
    @DalijaPrasnikar for the record, Code Review has TONS of working implementations of various coding problems in many languages, that anyone can browse and find useful, too... who said CR is only useful for the asker? – Mathieu Guindon Mar 18 '15 at 15:32
  • 2
    @Mat'sMug What I meant to say is that while this code would also suitable for CR (even though it has bugs), it may be more visible here than there. – Dalija Prasnikar Mar 18 '15 at 15:39
  • @DalijaPrasnikar Visibility is beside the point – David Heffernan Mar 18 '15 at 15:44
  • @DavidHeffernan People are upvoting because it is interesting question, it is more about how something can be done than CR itself, and since it has more visibility here than in CR site, it is more likely that it can help others that want to achieve similar thing. Things are not always black or white. – Dalija Prasnikar Mar 18 '15 at 17:16
  • 5
    @DalijaPrasnikar The fact that people upvote doesn't mean the question is in the right place. – David Heffernan Mar 18 '15 at 17:18
  • @DalijaPrasnikar I think it's more that the code is interesting, not the question. It really doesn't feel like the right place for a question like this as it opens the door to questions like "What do you think of this code?" or "How can I improve this?". The fact that there was a mistake in the code was just coincidental and sort of distracts from the on/off topic question. – Graymatter Mar 18 '15 at 22:12

1 Answers1

14

There is a bug in this code. Supports will destroy your object instance if it supports IUnknown but not the interface you are asking for.

Simple demonstration:

type
  IFoo = interface
    ['{32D3BE83-61A0-4227-BA48-2376C29F5F54}']
  end;

var
  o: TObject;
  i: IFoo;
begin
  i := TORM.CreateObjectInterface<TInterfacedObject, IFoo>(o); // <- boom, invalid pointer
end.

Best to put IInterface or IUnknown as additional constraint to T.

Or make sure that you are not destroying an already destroyed instance.

Unless you want to support dynamic QueryInterface implementations (where the class does not implement the interface but QueryInterface returns it) I would go with a Supports call on the class:

function TORM.CreateObjectInterface<T, I>(out AObject: TObject): I;
begin
  if not Supports(TClass(T), GetTypeData(TypeInfo(I))^.Guid) then 
    raise EORMUnsupportedInterface.CreateFmt(
      'Object class "%s" does not support interface "%s"',
      [AObject.ClassName, GUIDToString(GetTypeData(TypeInfo(I))^.GUID)]
    );

  AObject := T.Create;
  Supports(AObject, GetTypeData(TypeInfo(I))^.Guid, Result);
end;
Stefan Glienke
  • 20,860
  • 2
  • 48
  • 102