Black magic?

alpinistbg · 2018-12-09 22:17:53

I used to think that the GetID is just a getter of an ID property. Somebody explain me this black magic (because it didn't work in practice):

function TSQLRecord.GetID: TID;
begin
  {$ifdef MSWINDOWS}
  if PtrUInt(self)<PtrUInt(SystemInfo.lpMinimumApplicationAddress) then
    // was called from a TSQLRecord property (sftID type)
    // (will return 0 if current instance is nil)
    result := PtrUInt(self) else
    result := fID;
    // was called from a real TSQLRecord instance
  {$else}
  if PtrUInt(self)<$100000 then // rough estimation, but works in practice
    result := PtrUInt(self) else
  try
    result := fID;
  except
    result := PtrUInt(self);
  end;
  {$endif}
end;

Further:

function TSQLRecord.GetHasBlob: boolean;
begin
  if Self=nil then
    result := false else
    result := RecordProps.BlobFields<>nil;
end;

Into the both snippets, the author have assumption that the instance can be nil (!?). Also in 250+ another lines across the mORMot.pas. How is that possible?

Regards,

ab · 2018-12-10 08:05:33

I don't understand why you stated "it doesn't work in practice".

A class instance being nil happens.

The comment "was called from a TSQLRecord property (sftID type)" and the associated GetID documentation expects the context of this method.
If you don't understand it, ignore it: you probably won't need it.

alpinistbg · 2018-12-10 09:40:32

ab wrote:

I don't understand why you stated "it doesn't work in practice".

Simply: does not work. I don't use TSQLRecord properties. I use my own ID generating scheme (forced ids) and the program behaves differently on different machines (MSWin).

ab wrote:

A class instance being nil happens.

Yes, it happens. But it shouldn't be used. It is a programming error. A big one.

ab · 2018-12-10 10:15:00

So you meant "is not needed for me".

And no, a class instance being nil is perfectly valid, and checking it into methods is a way to make code shorter (no need to test if something<>nil then) and safer, especially with several expressions in a if clause.

You have perfectly the right to ignore this kind of code.
Just use the IDValue property instead of ID in your programs.

edwinsn · 2018-12-10 15:59:58

alpinistbg wrote:

ab wrote:
A class instance being nil happens.
Yes, it happens. But it shouldn't be used. It is a programming error. A big one.

Why do you think so? It's not a programming error but a very good programming technique called "Guard Clause" which makes the program reliable and clear to read.

ref1: https://en.wikipedia.org/wiki/Guard_(computer_science)
ref2: https://medium.com/@scadge/if-statement … 219a1a981a

A article related to the fact that self might be nil: http://blog.marcocantu.com/blog/when_self_is_nil.html
Also notice one of the comments:
"I've actually seen self = nil a few times, especially
in multi-threaded code. I have a couple methods that
start with ". -- Jim McKeeth

Last edited by edwinsn (2018-12-10 16:13:42)

alpinistbg · 2018-12-10 16:04:11

ab wrote:

And no, a class instance being nil is perfectly valid, and checking it into methods is a way to make code shorter (no need to test if something<>nil then) and safer, especially with several expressions in a if clause.

A class instance being nil is a non-existing instance. You can call a (static) instance method on a nil pointer although it's not the sort of thing you want to do deliberately. When that happens, execution continues along quite happily until it needs to access the instance data and then it all goes bang. It is anything but safer. Since VMT^ is part of the instance data (also non-existing) calling any virtual method is out of question, for example:

SynCommons.TObjectListHashed.Add(...); override;

has the similar check (self<>nil), which is pointless - the call won't reach it.

Furthermore, both things are not semantically equivalent, i.e. the life-cycle stage of an object and the default value of an object property. Or to say behavior of non-existing object (?!) in the case of static method.

It is more like a try to bypass and prevent corruptions, instead of solving the real problem whatever it may be.

ab wrote:

So you meant "is not needed for me".

No, I meant "is not needed for me, but it has implications on me".

ab wrote:

You have perfectly the right to ignore this kind of code.
Just use the IDValue property instead of ID in your programs.

I guess I have that right. But how do I know what to ignore until I read every single line from the source? >65K lines just in mormot.pas and growing...

Best regards,

Edit:
@edwinsn: It took me a while to wrote my reply to @ab then I noticed your reply in-between. Now I'll go thorough

Last edited by alpinistbg (2018-12-10 16:10:01)

alpinistbg · 2018-12-10 16:25:12

@edwinsn:

Guard clauses has nothing to do with that. Or to be more specific - yes, it had to have a guard clause before using a nil reference, not after that.

The irony is that I had the intention to refer to the Marco's post in order to support my point. Have you noticed the title?
"When Self in Nil... You Know You are in Trouble"

And, of course, any sort of comments can be expected on any post, I would point to Jolyon Smith's comment, the 5-th after the one you cited.

Last edited by alpinistbg (2018-12-10 16:36:30)

edwinsn · 2018-12-10 16:59:45

@alpinistbg,
I agree with you about that 'one should resort to first trying to find the source of the errors instead of just checking for if Self := nil'. ab might has his opinion based on the new replies here... In a perfect world (which is non-existence) I also wish ab stop using the 'with' keyword, but that's another question

Also need to consider - maybe there is another case where those 'if self = nil' guard clauses have reasons to stay - mORMot is a framework, and the clients (the code that the users of mORMot write) might be some mess multi-thread code and cause 'self = nil' to happen?

alpinistbg · 2018-12-10 17:43:40

edwinsn wrote:

Also need to consider - maybe there is another case where those 'if self = nil' guard clauses have reasons to stay - mORMot is a framework, and the clients (the code that the users of mORMot write) might be some mess multi-thread code and cause 'self = nil' to happen?

I'm afraid that may be the case, If regressions happened at some point in the past, or considering the other "trick" from TSQLRecord.GetID. Ab may have been resorted to such decisions.

ab · 2018-12-10 18:01:26

One valid pattern when you have an instance to nil is for lazy initialization.
For instance, when you set it first to nil, and then during some process you may initialize it only when needed.
It may be in a method, or even at class level, as a field.

No need to search problems in multi-thread access, or the FreeAndNil() pattern...

alpinistbg · 2018-12-10 18:52:53

@ab,
Why you're talking again in 'patterns'? What about the 'common sense' pattern? How about the VMT^ part in my previous reply? Asserting that a nil reference is usable just isn't sane!

Regards,

pvn0 · 2018-12-10 19:27:39

@alpinistbg
If you read the documentation it should be clear that this is a design decision. It's written as such as to prevent an access violation when it's accessed from a TSQLRecord published property, because in that case it's not a real instance and you shouldn't access it as such. Still, they decided to put in a safe guard for whatever reason.

If it was me I would just let it blow up with an exception, people either stick to the ORM design or they don't and also the windows and none-win code is not consistent.

Last edited by pvn0 (2018-12-10 19:29:07)

alpinistbg · 2018-12-10 19:44:17

@pvn,
I know that.

...As a consequence, the nested AnotherRecord property won't be a true class instance, but one ID trans-typed as TSQLRecord. ...

But that kind of guard will prevent only from 0 ids (non initialized). I am stunned by TSQLRecord.GetID dual use of a self reference/fID as TID and guessing from a magic constants.

ab · 2018-12-10 20:51:22

@alpinistbg
The TSQLRecord.GetID is not virtual, so here the VMT doesn't apply.

Some part of the framework may still check for nil, even if the method is now marked as virtual - it comes from the fact that it was not defined as virtual first, then marked as virtual, without changing the method implementation.
And it happened several times that a method was refactored into smaller methods, then the check for nil may have some benefit, since it may have moved into a non virtual method.

So you are right, some part of the code is unreachable now. But it won't hurt.
If you want to clean up this code, feel free to make a pull request.
We didn't do it here, since it has no performance impact. "If it works, don't touch it". We have other priorities, for sure.

danielkuettner · 2018-12-10 21:59:32

I can't understand such kind of critic. In german we say somebody looks for a hair in the soup.
Somebody gives us the opportunity(!) to use such a great framework. We should never forget this.

alpinistbg · 2018-12-10 23:19:23

@danielkuettner,

I apologize (if it is possible at all), in my country we have also saying: "no teeth are examined on a gift horse". You're perfectly right - this single issue can be considered as a "hair in the soup".
We really have to admire people who provide their work for the benefit of others.
Perhaps I should change my attitude.

At the other hand, everybody in that forum can easily check that I've trying to work (benefit) with (from) the framework for the past 4 years. And I really struck an issues.

To put it another way:

@ab,

I want to make a strong contribution to the development as an exchange for the benefits I have received (will receive in the future). But there are things that hinder me, and I believe not only me, but also many other potential contributors. To note a few of them:

- The mammoth size of the main source files; They are beyond my comprehension;
- My unawareness of the development model; AFAIK there is only a development version, nor stable or LTS;
- My ignorance of the framework historical background, in every project there are things you cannot explain without such a context; IMHO there is lot of decisions taken by reasons which I do not fully understand;

I sincerely believe that if you give a little push against the above things, I can be of real benefit to the cause.

Best regards,

edwinsn · 2018-12-11 03:40:52

pvn0 wrote:

...If it was me I would just let it blow up with an exception...

Agreed! Exception is such a godsend for handling errors!

mORMot Open Source

#1 2018-12-09 22:17:53

Black magic?

#2 2018-12-10 08:05:33

Re: Black magic?

#3 2018-12-10 09:40:32

Re: Black magic?

#4 2018-12-10 10:15:00

Re: Black magic?

#5 2018-12-10 15:59:58

Re: Black magic?

#6 2018-12-10 16:04:11

Re: Black magic?

#7 2018-12-10 16:25:12

Re: Black magic?

#8 2018-12-10 16:59:45

Re: Black magic?

#9 2018-12-10 17:43:40

Re: Black magic?

#10 2018-12-10 18:01:26

Re: Black magic?

#11 2018-12-10 18:52:53

Re: Black magic?

#12 2018-12-10 19:27:39

Re: Black magic?

#13 2018-12-10 19:44:17

Re: Black magic?

#14 2018-12-10 20:51:22

Re: Black magic?

#15 2018-12-10 21:59:32

Re: Black magic?

#16 2018-12-10 23:19:23

Re: Black magic?

#17 2018-12-11 03:40:52

Re: Black magic?

Board footer