Interface as Const parameter

[Rollo62 536]

Hi there,

 

@jeroenpJeron's blog notes points me to an old issue, thank you for keeping track on that very detailled.

I wonder, from the two practical workaround solutions, which one is best:

Quote

Two work-arounds that both add extra reference counting::

1. Replace the const parameter with a non-const one (basically removing the const bit) which will incur automatic _AddRef and _Release reference counting in the method.
2. Store the value of the const parameter in another reference, which will incur manual _AddRef and automatic _Release reference counting in the method.

Both solutions will call the _AddRef and _Release on the interface.

I would vote for the 1., as a golden rule: DON't USE CONST ON INTERFACE PARAM's

My argument would be, that is easy to spot at first sight in the interface and implementation,

and its easy possible to find by aoutomated tools.

 

What arguments are for the 2nd solution, are there any better ?

 

 

[Anders Melander 1782]

3. Leave things as-is but educate the developers of the problem so they don't fall into this trap.

 

I don't know what you criteria for "best" is but, if you have to, use whatever fits the individual case where this occurs. There doesn't need to be One True Way to solve this.

Edited February 10, 2021 by Anders Melander

[Dalija Prasnikar 1396]

21 minutes ago, Rollo62 said:

What arguments are for the 2nd solution, are there any better ?

I wouldn't use any of those solutions.

 

21 minutes ago, Rollo62 said:

I would vote for the 1., as a golden rule: DON't USE CONST ON INTERFACE PARAM's

Because that is not golden rule. On the contrary, for reference counted types you would almost always use const to avoid unnecessary reference counting overheard.

 

Problem is not in the method that has const, problem happens at call site. As such it should be handled at call site. First, handling it at call site allows you to trigger reference counting only for references that really need it, next, you will avoid situation where performance will become an issue and someone will go and change method signature according to common practice - using const and will inadvertently break code.

 

Far better solutions than 1. or 2. are using additional variable instead of constructing instance during method call, This is also easy rule to remember - don't create anything inplace. 

Next solution would be explicitly typecasting TFoo.Create as IFoo

And the last one, I like the most because it prevents you to make reference counting mistakes in other code, too, is to have class function that will create instance and return it as interface reference.

 

As for finding all places where you can create possible problem and you need to fix it, simple search and replace can go a long way. 

 

For instance - if you have class function New, you can just replace TFoo.Create with TFoo.New and if have same parameter list, you are good.

[Rollo62 536]

Quote

Far better solutions than 1. or 2. are using additional variable instead of constructing instance during method call, This is also easy rule to remember - don't create anything inplace. 

Ok, I agree on that too.

So you mean "Never use inplace creation, no matter if its object or instance is used" ?

Yes, the basic issue is maybe that inplace constructions are easily overseen, as it not obvious if they are used as object or interface.

You always need to dive into it, to find out.

But this is not 100% efficient either, I'm afraid.

 

To use always an additional variable at caller side is good rule too, but could also lead to clumsy looking code.

Maybe to get the best of both is to use special named functions, as shortcut,

like Add_Obj, Add_Inst, but thats not nice looking either.

 

The .New approach I like also a lot, and use it in many places.

Yes, maybe thats solving issues, but also maybe has not 100% performance.

 

The issue with all that I see is, it doesn't prevent or protect against accidentally using inplace creation,
which the 1st solution would be able to solve.

Edited February 10, 2021 by Rollo62

[Rollo62 536]

33 minutes ago, Anders Melander said:

3. Leave things as-is but educate the developers of the problem so they don't fall into this trap.

My criteria would be an easy to spot solution, that jumps in the eye when not following the "golden rule".

Of course this might be strongly philosophical, and maybe has personal preferences.

Thats why I think its good to list all possible options, with their arguments, in a short list like this.
So that everybody may decide for himself howto deal with it.

So far I count

1. Never use const interface parameters
2. Always use local inside

3. Keep as-is, educate on the issue

4. Never use inplace-creation

5. Always use .New creation approach

 

I think there were many good solutions on the table already, I'm curious if there were more outside.

Maybe really 4.) is really better than 1.), have to think about the pro's and con's.

In fact I rarely use inplace creation anyway, so it wouldn't be a big deal in my case anyway.

What I use is 2.) quite often, 4.) mostly, 5.) more and more often ...

 

Edited February 10, 2021 by Rollo62

[Dalija Prasnikar 1396]

15 minutes ago, Rollo62 said:

So you mean "Never use inplace creation, no matter if its object or instance is used" ?

Yes. Since such coding pattern is not too common in Delphi because of its manual memory management. So you either cannot use it at all, or you are creating objects and passing ownership which is not the most common scenario, or you are creating reference counted instance where inplace creating creates problems.

 

15 minutes ago, Rollo62 said:

The .New approach I like also a lot, and use it in many places.

Yes, maybe thats solving issues, but also maybe has not 100% performance.

There is no performance penalty here. You need to trigger reference counting when creating new instance. That is the original issue you are solving.

 

15 minutes ago, Rollo62 said:

The issue with all that I see is, it doesn't prevent or protect against accidentally using inplace creation,
which the 1st solution would be able to solve.

All you can do to prevent accidental misues is great, but sometimes people just need to use their brains and think what they are doing.

[Rollo62 536]

52 minutes ago, Dalija Prasnikar said:

Performance: That is the original issue you are solving.

Not really, I was thinking in the direction that in 95% of cases have NO performance issue at all,
so I could judge for "safety" over "performance".

Like

1. Never use const interface parameters (in most cases where a little performance loss is acceptable)
2. Always use local inside

3. Keep as-is, educate on the issue

4. Never use inplace-creation

5. Always use .New creation approach

That would make 1.) intrinsically safe under all circumstances, which was my goal, on cost of only a little performance loss.
Well, that makes 4.) and 5.) still very valuable and preferrable, and working perfectly together with 1.) too.

All in all reducing together the whole risk of "pitfalling" somewhere unexpected.

[Dalija Prasnikar 1396]

30 minutes ago, Rollo62 said:

> Performance: That is the original issue you are solving.

 

Not really, I was thinking in the direction that in 95% of cases have NO performance issue at all,
so I could judge for "safety" over "performance".

 

> There is no performance penalty here. You need to trigger reference counting when creating new instance. That is the original issue you are solving.

 

You have misquoted me. I didn't say that original problem you are solving is performance, but that you should not worry about performance with using New because you cannot avoid initial reference counting trigger for reference counted instances. And original issue with const is that it does not trigger reference counting when creating object inplace.

 

Performance comes to play, when you don't use const but in that case you don't have to bother using New either. Yes, there are other places where reference counting can bite you... but you can do whatever you like.

 

30 minutes ago, Rollo62 said:

Like

1. Never use const interface parameters (in most cases where a little performance loss is acceptable)
2. Always use local inside

3. Keep as-is, educate on the issue

4. Never use inplace-creation

5. Always use .New creation approach

That would make 1.) intrinsically safe under all circumstances, which was my goal, on cost of only a little performance loss.
Well, that makes 4.) and 5.) still very valuable and preferrable, and working perfectly together with 1.) too.

All in all reducing together the whole risk of "pitfalling" somewhere unexpected.

 

Additional issue with 1. and 2. is that not all code is under your control. Education is the first thing you should do, regardless of other approaches. Keeping developers in "stupid" mode is never smart thing to do.