Fixed variable's object value being deleted before marking the variable as uninitialised

[HelgeffegleH]

Reason, the variable might be read, directly or indirectly, under the false impression that the variable is set (with the object as its value), from the objects __delete, routine.

Example,

x := {__delete : t=>msgbox(isset(x))}
x := unset

[Lexikos]

There is a subtle error on this line:

auto obj = (var.mAttrib & VAR_ATTRIB_IS_OBJECT) ? mObject : nullptr;

mObject should be var.mObject.

As an optimization, we can replace the AddRef call with var.mAttrib &= ~VAR_ATTRIB_IS_OBJECT;. This avoids the runtime cost of two virtual method calls, and reduces code size (with the difference being effectively multiplied by inlined copies of the code).

There are a couple of minor style issues with the code:

• obj -> AddRef(); (spaces) inconsistent with obj->Release();.
• Spacing between functions (it's not entirely consistent across the whole project, but at least try to match the surrounding code).

---

It appears that var.Free(VAR_NEVER_FREE, true) would already handle things in the correct manner, applying VAR_ATTRIB_UNINITIALIZED and removing VAR_ATTRIB_IS_OBJECT before calling Release. Although this implementation of Uninitialize seems redundant, it does produce smaller code overall than delegating to Free, probably due to inlining.

For var := unset, we probably want VAR_ALWAYS_FREE or VAR_FREE_IF_LARGE rather than VAR_NEVER_FREE, which is what it was using (because the parameters of Assign were omitted). In v2.0-beta.12, var := "" is more likely to free the memory than var := unset, which I suppose is counter-intuitive. Other callers of Uninitialize might be better with VAR_FREE_IF_LARGE or as-is, favouring speed over memory usage.

var->Free(..., true) shouldn't be used directly for var := unset, because var can be an alias, in which case true would cause it to revert to a normal variable instead of "uninitializing" the target variable. Now that there is a need, the second parameter should probably be split into separate options (flags or parameters). Combining it with the first parameter as a set of flags might be best for code size.

if (result)

What do you expect to happen when Assign() fails? Ordinarily, it might retain its previous value. (Actually, it was generally making the variable blank, not unset.) In this case, maybe we want it to be marked as uninitialized regardless; but I don't think it's possible for Assign() to fail when the parameters are omitted. It can only fail for memory allocation failures.

When it's not possible for a function to fail, I prefer to remove the return value to show that there can only be one outcome. Sometimes this also results in smaller code.

---

There is a similar case that Uninitialize doesn't cover:

x := {__delete : t=>msgbox(x ?? "not set")}
x := "foo"

So this is really a more general problem that can't be solved by just adding a new method.

There's another similar case that AssignString didn't handle correctly: if __Delete assigned a new object to the variable, AssignString unconditionally removed VAR_ATTRIB_IS_OBJECT and continued with the assignment regardless. Any other type of value assigned by __Delete was also lost, but without leaking anything.

Actually, even Assign() followed by MarkUninitialized() can backfire, because Assign can cause __Delete to execute, which can assign a new value to the variable.

---

I think 67776fd covers everything.

[HelgeffegleH]

There are a couple of minor style issues with the code:

Sometimes VS makes some whitespace adjustments, for example, this was not intended. I accidently cut the code instead of copying it, then pasting it back again changed the whitespace, which I noticed after making the PR, intending to fix it later, but forgot. The x -> y inconstiencty is probably just a helgeffeeghel volatile finger error though, sorry.

What do you expect to happen when Assign() fails?

I just tried to preserve the return value as it would have been before the change, see previous lines L87-L90 and L46-L48

I don't think it's possible for Assign() to fail when the parameters are omitted. It can only fail for memory allocation failures.
When it's not possible for a function to fail, I prefer to remove the return value to show that there can only be one outcome. Sometimes this also results in smaller code.

I think it is more maintainable to treat it as if it can fail though, which I'd prefer over hypothetical code size optimisations. But as indicated above, I favoured not changing the previous ways the return value propagated.

Thank you very much for the feedback and the fixes 👍