In D2205#91522, @vladislavbelov wrote:

we do not need to check whether the CGUI page exists or not.

Reference doesn't guarantee that.

Nor did I claim that its the reference guaranteeting that.

From my experience such kind of changes doesn't really improve a code and usually adds some difficulty.

Using a reference removes the difficulty here.

In more common words a pointer/reference to some global or relatively global object means lack of a complete design.

Not helpful

In D2205#91525, @elexis wrote:

From my experience such kind of changes doesn't really improve a code and usually adds some difficulty.

Using a reference removes the difficulty here.

I didn't find storying of reference in gui objects before. So it seems that you already implicitly added a strict property to GUI objects.

I'm talking about copying (and moving though, as it's not possible to clearly invalidate the object). The copying isn't useful here but you didn't add any comment nor special macro: NONCOPYABLE. And I call that difficulty of code understanding, as one may think that it's still possible to copy.

In more common words a pointer/reference to some global or relatively global object means lack of a complete design.

Not helpful

I mean that we need to remove that pGUI.

Or at least from all constructors. It has a lot of code duplication. I prefer to add GetGUI() method to IGUIObject and remove GUI from all constructors as it can be implemented more clear.

Another opinion:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

I'd agree with vlad that non-const references are probably trickier than non-const pointers. Const references are preferable to pointers however.
I also rather dislike keeping references as member variables.

Further - I don't see much the point of such a diff. It introduces a chance for errors (admittedly a really limited one in this instance), for very little gain, since it's purely syntactic.

In D2205#91536, @vladislavbelov wrote:

In D2205#91525, @elexis wrote:

From my experience such kind of changes doesn't really improve a code and usually adds some difficulty.

Using a reference removes the difficulty here.

I didn't find storying of reference in gui objects before.

The point is, if you operate a CGUI* then you are not sure if it's allocated or not. But if it always is, then adding safeguards is only adding more code and saving from something that never happens.
The reference expresses that the CGUI is always allocated during its lifetime, thus it is making it even impossible to test if (m_pGUI) or if (pGUI).
The m_pGUI pointer exists since rP74.
The commit mentioned in the summary changed it so that CGUI is allocated on init rather than some cycles later (which is even making it faster, since the value is directly assigned to the correct pointer instead of to nullpointer and then to something else), aside from making it easier for all the users to consider it.
If you look at that commit referenced in the summary, you will see the previous cases in which it was set.
SetGUI was called on AddScrollBar, on CGUIDummyObject creation, for every child object created in AddObject, and prior to RegisterScriptHandler.

So it seems that you already implicitly added a strict property to GUI objects.

It's explicit in the code, and it can't be expressed further than with a reference that the CGUI page exists for the lifetime of the IGUIObject.
Also it's I mentioned it explicitly in the summary of this revision proposal.

I'm talking about copying (and moving though, as it's not possible to clearly invalidate the object).

The copying isn't useful here but you didn't add any comment nor special macro: NONCOPYABLE.

You mean NONCOPYABLE(IGUIObject); and NONCOPYABLE(CGUI);?

And I call that difficulty of code understanding, as one may think that it's still possible to copy.

Difficulty of understanding depending on the amount of code actually read, and the people who work with that code read that part.

In more common words a pointer/reference to some global or relatively global object means lack of a complete design.

Not helpful

I mean that we need to remove that pGUI.

Or at least from all constructors. It has a lot of code duplication. I prefer to add GetGUI() method to IGUIObject and remove GUI from all constructors as it can be implemented more clear.

??
There is GetGUI and it returns this pointer, or reference.

The only way to remove m_pGUI from IGUIObject is to pass CGUI& in every function call. Not sure if this will make better code.

In D2205#91541, @wraitii wrote:

I'd agree with vlad that non-const references are probably trickier than non-const pointers. Const references are preferable to pointers however.

It's tricky to keep calm if people say that your patch is wrong without having read the affected code or brought fourth a single argument specific to the patch.

It's tricky to keep calm if people say that your patch is wrong without having read the affected code or brought fourth a single argument specific to the patch.

I've read your summary and I understand why you want to change it so. I don't think it's a necessary or particularly useful change however, and it introduces extremely tight coupling that might be annoying to change in the future, should we want to (a GUI Object _must_ have a GUI page)
The cost/benefit seems neutral or negative to me.

Further:

This patch now changes the pointer to a reference, so that it is absolutely clear to everyone that we do not need to check whether the CGUI page exists or not.

Technically this isn't quite true, the reference could be dangling at some point and we'd find ourselves in quite a bit of trouble.

The only way to remove m_pGUI from IGUIObject is to pass CGUI& in every function call. Not sure if this will make better code.

It might if that dependency doesn't actually exist everywhere, so the variable being passed means something. That would require reading all methods of all GUI objects though, which I don't care to do, so this is purely a nitpick.

In D2205#91549, @wraitii wrote:

I've read your summary and I understand why you want to change it so.

Evidently you dont understand why I want to change it if you say that it doesn't change anything and even more so if you say that it introduces something that it is already the case.

I don't think it's a necessary or particularly useful change however, and it introduces extremely tight coupling that might be annoying to change in the future, should we want to (a GUI Object _must_ have a GUI page)
The cost/benefit seems neutral or negative to me.

Sorry, but you don't get it.
Either you decide for a paradigm where CGUI is always allocated by design, and thus chose a reference,
or you decide for a design where CGUI is optional and thus chose a pointer.
In the first case we change some -> to ..
In the second case you add dozens of ENSURE(m_pGUI), if (m_pGUI) { bigscope }.
So unless you accept this patch changing pointer to ref, you have to accept the patch that adds lots of dead code.
Please don't make assertive statements if you have not read the involved code.

Further:

This patch now changes the pointer to a reference, so that it is absolutely clear to everyone that we do not need to check whether the CGUI page exists or not.

Technically this isn't quite true, the reference could be dangling at some point and we'd find ourselves in quite a bit of trouble.

No, it _can_ _not_ be dangling, that's the whole freaking point of using a reference.

The only way to remove m_pGUI from IGUIObject is to pass CGUI& in every function call. Not sure if this will make better code.

It might if that dependency doesn't actually exist everywhere, so the variable being passed means something. That would require reading all methods of all GUI objects though, which I don't care to do, so this is purely a nitpick.

Well you can get an idea of which code places use m_pGUI just by looking at the lines changed. The lines using m_pGUI are a superset.
It may be possible to pass m_pGUI in just about every function, but does it help? (And don't give me a "yes that helps" only because it's consistent with what you said before, we should decide code on actual arguments).

I've added 54 reasons not to use pointers in the inline comments.

In D2205#91544, @elexis wrote:

The point is, if you operate a CGUI* then you are not sure if it's allocated or not. But if it always is, then adding safeguards is only adding more code and saving from something that never happens.
The reference expresses that the CGUI is always allocated during its lifetime, thus it is making it even impossible to test if (m_pGUI) or if (pGUI).
The m_pGUI pointer exists since rP74.
The commit mentioned in the summary changed it so that CGUI is allocated on init rather than some cycles later (which is even making it faster, since the value is directly assigned to the correct pointer instead of to nullpointer and then to something else), aside from making it easier for all the users to consider it.
If you look at that commit referenced in the summary, you will see the previous cases in which it was set.
SetGUI was called on AddScrollBar, on CGUIDummyObject creation, for every child object created in AddObject, and prior to RegisterScriptHandler.

As far as I understand the patch the changes only shows to readers that the m_pGUI member can be used without the safeguard. It doesn't guarantee real lifetime of the CGUI object, as it's created on heap.

So it seems that you already implicitly added a strict property to GUI objects.

It's explicit in the code, and it can't be expressed further than with a reference that the CGUI page exists for the lifetime of the IGUIObject.

By property I mean not the reference but the abstract property to be copied.

Also it's I mentioned it explicitly in the summary of this revision proposal.

It's not visible from the code.

You mean NONCOPYABLE(IGUIObject); and NONCOPYABLE(CGUI);?

NONCOPYABLE(IGUIObject); is enough.

Or at least from all constructors. It has a lot of code duplication. I prefer to add GetGUI() method to IGUIObject and remove GUI from all constructors as it can be implemented more clear.

??
There is GetGUI and it returns this pointer, or reference.
The only way to remove m_pGUI from IGUIObject is to pass CGUI& in every function call. Not sure if this will make better code.

I mean to remove m_pGUI initialization and usage from all inherited objects and hide this property as much as possible behind a getter.

Also for ex: CButton::CButton(CGUI& pGUI) it's a duplication, as you need to add this argument for each constructor of each inherited object. I think it can be hidden easily.

I'd suggest to replace all m_pGUI by GetGUI(), which can return whatever you want. Fo ex:

CGUI& GetGUI()
{
    ENSURE(m_pGUI);
    return *m_pGUI;
}
// or
CGUI& GetGUI()
{
    ENSURE(IsGUIStillValid(m_pGUI));
    return m_pGUI;
}

In D2205#91609, @vladislavbelov wrote:

As far as I understand the patch the changes only shows to readers that the m_pGUI member can be used without the safeguard.

Contrary to a comment which is hidden in one of thousands of lines of code, if it is a reference then the reader cannot even have the thought to check about lifetime, whereas for the pointer he has to start searching.
Even further someone who thinks it may be better to test for non-null to error on the side of safety will be corrected by the compiler.

It doesn't guarantee real lifetime of the CGUI object, as it's created on heap.

I still didn't claim that this guarantees the lifetime of the CGUI object.
I did and do claim that the lifetime is guaranteed by the way the code is designed and written, namely that GUIObjects are created inside a CGUI and that the CGUI destroys its objects before it is destroyed and that CGUI objects are never moved or to be moved from one GUI to another.

So it seems that you already implicitly added a strict property to GUI objects.

It's explicit in the code, and it can't be expressed further than with a reference that the CGUI page exists for the lifetime of the IGUIObject.

By property I mean not the reference but the abstract property to be copied.

Ping me when the day comes where we want to copy a CGUI page.

Also it's I mentioned it explicitly in the summary of this revision proposal.

It's not visible from the code.

You mean NONCOPYABLE(IGUIObject); and NONCOPYABLE(CGUI);?

NONCOPYABLE(IGUIObject); is enough.

NONCOPYABLE(IGUIObject); and NONCOPYABLE(CGUI); are not visible in the code?

I mean to remove m_pGUI initialization and usage from all inherited objects

Just to understand what you mean - You want to keep m_pGUI but initialize it in some random places so that noone knows whether the thing was allocated or not and having to initialize it if it hasn't already been done, or do you want to remove m_pGUI and passing it as an argument in every function call?

hide this property as much as possible behind a getter.

To hide a private member from the class that it owns?
What does CGUI@ GetGUI() { return m_pGUI; } hide other than the information how GUI is obtained?
It's only making it less transparent what happens and the unaware reader will assume that the function does something.
I agree to use GetGUI for public access, I didn't change that.
But a class accessing it's own member isn't controversial.

Also for ex: CButton::CButton(CGUI& pGUI) it's a duplication, as you need to add this argument for each constructor of each inherited object.

I don't need to add this argument since it has been added already in the mentioned commit I.

I think it can be hidden easily.

I'd rather have one line with a constructor init per class than initializing something that is already known at the constructor time rather than null-ptr initializing it first and then rewriting the value later with the correct one, leaving 54 GetGUI / m_pGUI users to wonder if the thing has been allocated already.
The constructor shows you and wraitii that the lifetime, so if you hide that, you hide the fact that the lifetime is already assured at construction time.

In D2205#91610, @vladislavbelov wrote:

I'd suggest to replace all m_pGUI by GetGUI(), which can return whatever you want. Fo ex:

CGUI& GetGUI()
{
    ENSURE(m_pGUI);
    return *m_pGUI;
}
// or
CGUI& GetGUI()
{
    ENSURE(IsGUIStillValid(m_pGUI));
    return m_pGUI;
}

Did you see the inline comments?
ENSURE isn't worth anything in terms of code security, because if you click on continue it still dereferences null.
So you actually need If checks in all 54 places if you want to play the pointer game.
But the design of the GUI already says that the GUI will always exist, the IGUIObjects are created within the JS Context of the CGUI, even if they are not rooted (JS::Heap / JS::PersistentRooted).
You have the choice between accepting the current design and informing the reader that lifetime is assured and expressing that by ref or adding 54 if statements that have always been false for 15 years and will remain false until the GUI and ScriptInterface design changes. If the GUI and ScriptInterface design changes, you can call me.

ENSURE isn't worth anything in terms of code security, because if you click on continue it still dereferences null.

If you shoot yourself in the foot, your foot is going to be hurt. That's not an argument, it's completely irrelevant.

Sorry, but you don't get it.
Either you decide for a paradigm where CGUI is always allocated by design, and thus chose a reference,[...]
or you decide for a design where CGUI is optional and thus chose a pointer.

No actually I don't have to pick a "paradigm" here. Both the reference and the pointer code are completely valid and functional. Why you want to add 54 nullability checks is your problem, you're changing perfectlyy working code

In the first case we change some -> to ..
In the second case you add dozens of ENSURE(m_pGUI), if (m_pGUI) { bigscope }.

I argue for the case of doing nothing.

Please don't make assertive statements if you have not read the involved code.

Please don't make un-necessary changes.

Further:

This patch now changes the pointer to a reference, so that it is absolutely clear to everyone that we do not need to check whether the CGUI page exists or not.

Technically this isn't quite true, the reference could be dangling at some point and we'd find ourselves in quite a bit of trouble.

No, it _can_ _not_ be dangling, that's the whole freaking point of using a reference.

It totally can. Compile this and watch it crash.

struct des {
    des()
    {
        item = new int();
        *item = 0;
    }
    ~des()
    {
        delete item;
        item = nullptr;
    }
    int* item = 0;
};

struct example {
    example(des& a) : dangling(a) {};
    des& dangling;
};

int main()
{
    example* ex = nullptr;
    {
        des destructed;
        *destructed.item = 2;
        ex = new example(destructed);
    }
    // This dereferences a null pointer.
    return *ex->dangling.item;
}

In D2205#91634, @wraitii wrote:

Why you want to add 54 nullability checks is your problem

So you're making this a personal issue, okay, I'm always happy to go down that road.

In the first case we change some -> to ..
In the second case you add dozens of ENSURE(m_pGUI), if (m_pGUI) { bigscope }.

I argue for the case of doing nothing.

Why would you keep dead code? Yes I know you didn't read the patch completely becaues you said above that you don't care to read the code.

Further:

This patch now changes the pointer to a reference, so that it is absolutely clear to everyone that we do not need to check whether the CGUI page exists or not.

Technically this isn't quite true, the reference could be dangling at some point and we'd find ourselves in quite a bit of trouble.

No, it _can_ _not_ be dangling, that's the whole freaking point of using a reference.

It totally can. Compile this and watch it crash.

struct des {
    des()
    {
        item = new int();
        *item = 0;
    }
    ~des()
    {
        delete item;
        item = nullptr;
    }
    int* item = 0;
};

struct example {
    example(des& a) : dangling(a) {};
    des& dangling;
};

int main()
{
    example* ex = nullptr;
    {
        des destructed;
        *destructed.item = 2;
        ex = new example(destructed);
    }
    // This dereferences a null pointer.
    return *ex->dangling.item;
}

Thanks for the strawman, how often more do you want to tell me again that some other pointer or reference can crash that is not the code that we talk about?
How often will you tell me more that some other code that is not the code we speak about can crash?

I'm happy to discuss with you, but not if you just spill some random positions without knowing what you talk about, without having read the code in the patch or without even reading what I say.

So going back to

No, it _can_ _not_ be dangling, that's the whole freaking point of using a reference.

If there was someone actually looking at the code, he would ask whether that assumption is actually true. But there is no such person, so who cares.

Thanks for the strawman, how often more do you want to tell me again that some other pointer or reference can crash that is not the code that we talk about?
How often will you tell me more that some other code that is not the code we speak about can crash?

What other code? You're storing a reference to some class. That reference can become dangling. This is a fact of the language, C++ isn't rust and can't actually guarantee that a reference exists. I don't understand why you keep on harping about "this code" or "that code" or whatever.

If you are saying "this particular reference can't be dangling because CGUi constructs the object, always owns the object, and destructs the object before it itself is destroyed" then you can also say that for a pointer and I still don't understand why you want to change perfectly working code for purely syntactic reason.

In D2205#91624, @elexis wrote:

I still didn't claim that this guarantees the lifetime of the CGUI object.
I did and do claim that the lifetime is guaranteed by the way the code is designed and written, namely that GUIObjects are created inside a CGUI and that the CGUI destroys its objects before it is destroyed and that CGUI objects are never moved or to be moved from one GUI to another.

It's not the objection to your patch, it's just a note that reference isn't safe as it may seem.

Ping me when the day comes where we want to copy a CGUI page.

I didn't talk about CGUI, but about copying IGUIObject.

NONCOPYABLE(IGUIObject); and NONCOPYABLE(CGUI); are not visible in the code?

You're right, I took a look at my outdated repository.

Also for ex: CButton::CButton(CGUI& pGUI) it's a duplication, as you need to add this argument for each constructor of each inherited object.

I don't need to add this argument since it has been added already in the mentioned commit I.

It wasn't an objection to your patch, but to adding a new object.

ENSURE isn't worth anything in terms of code security, because if you click on continue it still dereferences null.
So you actually need If checks in all 54 places if you want to play the pointer game.

Nope, only 1 ENSURE.

My current understanding of your point: I think you think that the CGUI reference in a constructor of an object says to a reader that the CGUI is guaranteed (should be guaranteed) by creator to be valid during lifetime of the object. And a pointer or GetGUI() is less obvious for a reader.

In my opinion GetGUI() isn't worse. It might be guaranteed by a comment near its declaration and it might hide many duplications.

So question for me is: does this guarantee in each constructor really help to reader?

In D2205#91639, @wraitii wrote:

Thanks for the strawman, how often more do you want to tell me again that some other pointer or reference can crash that is not the code that we talk about?
How often will you tell me more that some other code that is not the code we speak about can crash?

What other code?

The code that you have posted??

You're storing a reference to some class. That reference can become dangling. This is a fact of the language, C++ isn't rust and can't actually guarantee that a reference exists. I don't understand why you keep on harping about "this code" or "that code" or whatever.

So you are actually saying I should use a pointer and check with if in all 54 places

If you are saying "this particular reference can't be dangling because CGUi constructs the object, always owns the object, and destructs the object before it itself is destroyed" then you can also say that for a pointer

Yes, that's what I have been repeating.
Yes, that property is true for the pointer as well, which is the reason why I wrote the patch to change the syntax to better reflect the semantics.

I still don't understand why you want to change perfectly working code for purely syntactic reason.

1. There are some if (m_pGUI) checks. There are maybe < 5 remaining, previous commits removed 10 or 15. This commit makes it consistent. Either we assume that m_pGUI can be null and check for it consistently, or we assume that its always given. Currently it is always given.

2. As already repeated several times: If you get a pointer, the first thing you do is to ask yourself if this can be null and whether you need an if safeguard. Then after you have noticed that it's always non-null, there is still someone coming along and saying you should add the safeguard regardless because it's a pointer and it's better to error on the side of caution.

In contrast to that, if you receive a reference, you are unable to check for null. And you even don't have to question yourself whether the reference is dangling or not, because if it would be dangling, the one writing the reference code would have made a mistake. But when it uses a pointer, the one using the pointer makes the mistake. So it means we take the responsibility in this commit once and it means that every future user of the variable does not need to consider this anymore whether it is dangling or not.

3. It is enforced on the compiler level that people don't add if safeguards or ENSUREs. (Yes I'm still not saying that a reference can't be dangling in principle, read my words carefully.)

Removing unused code, making code more consistent (by not having half the places check uselessly and other half of places rely on non-null seemingly recklessly), making the code easier to read (because one doesn't have to worry anymore about null), saving authors time, that's what you call not an improvement.

Yes, that property is true for the pointer as well, which is the reason why I wrote the patch to change the syntax to better reflect the semantics.

1. There are some if (m_pGUI) checks. There are maybe < 5 remaining, previous commits removed 10 or 15.

"Oh so it's not a big deal then", he said snarkily.

2. As already repeated several times: If you get a pointer, the first thing you do is to ask yourself if this can be null and whether you need an if safeguard.

Well, if your function doesn't make sense with a null pointer, no. Then you just crash. You could "ENSURE", you could let it crash, but the point is the error is not in your code but in the calling code, just like in the reference case. If something can't do its job with a null-pointer, it's the caller's responsibility to ensure that it is not called with a null pointer.

Thus I disagree with:

And you even don't have to question yourself whether the reference is dangling or not, because if it would be dangling, the one writing the reference code would have made a mistake. But when it uses a pointer, the one using the pointer makes the mistake.

I think there's no difference at all, caller makes the mistake in both cases when the function needs a valid object. If the object is optional, then one should use std::optional if you want semantics, not a pointer.

3. It is enforced on the compiler level that people don't add if safeguards or ENSUREs.

Yes but "people" weren't adding if safeguards anyways, so it seems to me you're protecting against... yourself?

Removing unused code, making code more consistent (by not having half the places check uselessly and other half of places rely on non-null seemingly recklessly), making the code easier to read (because one doesn't have to worry anymore about null), saving authors time, that's what you call not an improvement.

Well, yeah, I recognise this has some tangible improvements in terms of cognitive load, but I feel it's rather small and it was perhaps not worth writing, probably not worth debating and certainly not worth getting in an argument over.

In D2205#91643, @vladislavbelov wrote:

In D2205#91624, @elexis wrote:

I still didn't claim that this guarantees the lifetime of the CGUI object.
I did and do claim that the lifetime is guaranteed by the way the code is designed and written, namely that GUIObjects are created inside a CGUI and that the CGUI destroys its objects before it is destroyed and that CGUI objects are never moved or to be moved from one GUI to another.

It's not the objection to your patch, it's just a note that reference isn't safe as it may seem.

I know, it's not a shared_ptr.

ENSURE isn't worth anything in terms of code security, because if you click on continue it still dereferences null.
So you actually need If checks in all 54 places if you want to play the pointer game.

Nope, only 1 ENSURE.

ENSURE doesn't ENSURE.
It means the player gets a popup box, clicks on continue and after that the code continues and dereferences null.

And if you already ENSURE that its always != null then you can just as well use a reference to avoid ENSURE.

My current understanding of your point: I think you think that the CGUI reference in a constructor of an object says to a reader that the CGUI is guaranteed (should be guaranteed) by creator to be valid during lifetime of the object.

That is one of the points.

If you get a reference you don't even have the thought "Do I need to check for null", whereas it's the first thought if you receive a pointer.
Even more it makes it impossible to check if the reference is null, so you don't have half the code with null checks and the other code without and some other code with ENSURE.

In my opinion GetGUI() isn't worse. It might be guaranteed by a comment near its declaration and it might hide many duplications.

IMO GetGUI isn't adding anything, and it's not really the core of the issue, because the member variable still exists and GetGUI itself still has the ENSURE or IF or terminate / throw, so you didn't fix the issue, you just deduplicated the issue and still have the issue, whereas this patch removes any room for if/ENSURE/...

So question for me is: does this guarantee in each constructor really help to reader?

Yes, it informs the reader that he is getting that thing directly and forever.

Notice there is one IGUIObject m_BaseObject that is an edge case, it's constructed in the CGUI constructor, but the CGUI this pointer already exists and that IGUIObject is a dummy, not doing anything, so there is no issue with the base object. https://en.cppreference.com/w/cpp/language/this

In D2205#91645, @wraitii wrote:

Yes, that property is true for the pointer as well, which is the reason why I wrote the patch to change the syntax to better reflect the semantics.

1. There are some if (m_pGUI) checks. There are maybe < 5 remaining, previous commits removed 10 or 15.

"Oh so it's not a big deal then", he said snarkily.

Yeah, removing the checks is the first thing I did.
The second thing I did was making this a reference so that people don't repeat this in the future.
The third thing I did was finding even more of these null checks because the compiler is better at finding these things than someone searching manually.

2. As already repeated several times: If you get a pointer, the first thing you do is to ask yourself if this can be null and whether you need an if safeguard.

Well, if your function doesn't make sense with a null pointer, no. Then you just crash. You could "ENSURE", you could let it crash, but the point is the error is not in your code but in the calling code, just like in the reference case. If something can't do its job with a null-pointer, it's the caller's responsibility to ensure that it is not called with a null pointer.

Yes, it's ths callers responsibility to ensure that an IGUIObject isn't leaked and doesn't operate on a dangling CGUI.
But it doesn't change the fact that... well what I said about about pointers leading to people wondering whether they need to test for null, and some people adding the null checks because they are uncertain.
Using a reference makes them certain and anticipates the entire null or not null mystery on the user level (only the provider needs to care, and you can see the only provider is *this in this patch)
How about not writing code that crashes and adding exceptions and catching exceptions or safeguarding everything when we can avoid that by using one different character.

Thus I disagree with:

And you even don't have to question yourself whether the reference is dangling or not, because if it would be dangling, the one writing the reference code would have made a mistake. But when it uses a pointer, the one using the pointer makes the mistake.

I think there's no difference at all, caller makes the mistake in both cases when the function needs a valid object. If the object is optional, then one should use std::optional if you want semantics, not a pointer.

I give you
void foo(T* foo)
void foo(T& foo)

Add const if you like.
Now you tell me, for which of the two functions do you wonder whether you receive a null argument?

3. It is enforced on the compiler level that people don't add if safeguards or ENSUREs.

Yes but "people" weren't adding if safeguards anyways, so it seems to me you're protecting against... yourself?

I didn't add the if statements that are always false, so evidently I'm not protecting against something that is a pattern.
I'm adding this to protect from future people who don't have sufficient understanding of the codebase to know that this pointer is always allocated,
and to protect against those people who will error on the side of caution and prefer to catch null-deref even if it can supposedly never happen.

If you receive a pointer, you receive the seed of doubt that it may be null. You'll never get rid of this doubt each time you read the calls unless you use a reference.

Removing unused code, making code more consistent (by not having half the places check uselessly and other half of places rely on non-null seemingly recklessly), making the code easier to read (because one doesn't have to worry anymore about null), saving authors time, that's what you call not an improvement.

Well, yeah, I recognise this has some tangible improvements in terms of cognitive load, but I feel it's rather small and it was perhaps not worth writing, probably not worth debating and certainly not worth getting in an argument over.

In D2205#91647, @elexis wrote:

If you get a reference you don't even have the thought "Do I need to check for null", whereas it's the first thought if you receive a pointer.
Even more it makes it impossible to check if the reference is null, so you don't have half the code with null checks and the other code without and some other code with ENSURE.
IMO GetGUI isn't adding anything

It answers to the "Do I need to check for null" question as it returns a reference. I'm not talking about how it stores GUI inside.

Yes, it informs the reader that he is getting that thing directly and forever.

And it adds a lot of duplication in constructors. GetGUI comment also defines it directly and forever. In both cases you have a reference inplace, but for more details you need to go to another place (constructor/comment).

So more detailed question for me is (I skipped a pointer for a while): what's better a) constructor in the same file with a "smart" (as you have to understand a reference lifetime) guarantee and a lot of duplication or b) comment in other (one) file which describes the guarantee of GetGUI() clearly? (Both m_pGUI and GetGUI() return a reference without any ENSURE).

In D2205#91650, @vladislavbelov wrote:

In D2205#91647, @elexis wrote:

If you get a reference you don't even have the thought "Do I need to check for null", whereas it's the first thought if you receive a pointer.
Even more it makes it impossible to check if the reference is null, so you don't have half the code with null checks and the other code without and some other code with ENSURE.
IMO GetGUI isn't adding anything

It answers to the "Do I need to check for null" question as it returns a reference. I'm not talking about how it stores GUI inside.

Keeping CGUI* and only switching to GetGUI& puts the problem into GetGUI whereas this patch removes it from all places.
Also using GetGUI in all current places doesn't mean that the next best commit uses the member variable again. Fix the problem at the root and everything else follows along.

Yes, it informs the reader that he is getting that thing directly and forever.

And it adds a lot of duplication in constructors. GetGUI comment also defines it directly and forever. In both cases you have a reference inplace, but for more details you need to go to another place (constructor/comment).

So more detailed question for me is (I skipped a pointer for a while): what's better a) constructor in the same file with a "smart" (as you have to understand a reference lifetime) guarantee and a lot of duplication or b) comment in other (one) file which describes the guarantee of GetGUI() clearly? (Both m_pGUI and GetGUI() return a reference without any ENSURE).

The one m_pGUI(pGUI) line per class is not really adding any painful duplication either, it's not replicating and modifying some logic, it's just member initalization.
And this patch doesn't add anything, it only changes * to &.
I would say that is quite affordable given that the cost is a quarter line per file and the benefit is not having to worry about null anymore.
It is good practice to initialize all member variables with their correct value at construction time, that's the task description of a constructor.

I give you
void foo(T* foo)
void foo(T& foo)
Now you tell me, for which of the two functions do you wonder whether you receive a null argument?

Well, only the first, but you actually get no guarantee that foo exists with either. That's all I'm saying.
Indeed, you weren't arguing for that, indeed what you argued was that:

This patch now changes the pointer to a reference, so that it is absolutely clear to everyone that we do not need to check whether the CGUI page exists or not.

And as I've said, this feels like the smallest gain. I don't think it's worth changing so many lines and introducing a member reference (which has the highest likelihood of dangling, among references), over.

I'm adding this to protect from future people who don't have sufficient understanding of the codebase to know that this pointer is always allocated,
and to protect against those people who will error on the side of caution and prefer to catch null-deref even if it can supposedly never happen.

Though you're protecting against people writing correct, if a bit useless, code. That seems, in the big picture, not that important.

If you receive a pointer, you receive the seed of doubt that it may be null. You'll never get rid of this doubt each time you read the calls unless you use a reference.

We're not defusing a bomb though... I'm not losing sleep over this _ ever _. And If I ever get a dangling reference error, I'm more likely to guess it came from a pointer than a reference...

Edit:
Also you're again saying references can't be invalidated:

So question for me is: does this guarantee in each constructor really help to reader?

Yes, it informs the reader that he is getting that thing directly and forever.

NOT forever... If you mean that "this informs the reader that he can safely assume this object always exists"... Then a comment is better, and the best place for that would be GetGUI().

I'm saying I don't like the reference member variable. You could just say "Meh", and commit this, I don't care enough to actually try and stop you from doing this, but I think it's a bad idea.

The rest is your call.

In D2205#91651, @elexis wrote:

Fix the problem at the root and everything else follows along.

I can't say that the patch solves the problem at the root. It doesn't solve a real problem of GUI existence but it adds a visual confidence that the interface try/should to guarantee that.

The one m_pGUI(pGUI) line per class is not really adding any painful duplication either, it's not replicating and modifying some logic, it's just member initalization.

GetGUI is not really adding any painful duplication as well.

I would say that is quite affordable given that the cost is a quarter line per file and the benefit is not having to worry about null anymore.

I don't remember that someone (except you) whenever had serious worries about that.

It is good practice to initialize all member variables with their correct value at construction time, that's the task description of a constructor.

I don't disagree with that.

In D2205#91661, @vladislavbelov wrote:

In D2205#91651, @elexis wrote:

Fix the problem at the root and everything else follows along.

I can't say that the patch solves the problem at the root. It doesn't solve a real problem of GUI existence but it adds a visual confidence that the interface try/should to guarantee that.

The one m_pGUI(pGUI) line per class is not really adding any painful duplication either, it's not replicating and modifying some logic, it's just member initalization.

GetGUI is not really adding any painful duplication as well.

I would say that is quite affordable given that the cost is a quarter line per file and the benefit is not having to worry about null anymore.

I don't remember that someone (except you) whenever had serious worries about that.

Most of the code in source/gui/ has not been touched since 2004, so the sample size is limited.
And gandhi said "Even if you are a minority of one, the truth is the truth."
It doesn't matter whether it's me, you or anyone else, it's pattern, these things are true for everyone alike.
But I do remember precisely you recommending me to add if-safeguards for other places in the code even after I mentioned that it's always given, and then you even convinced me that it's a good idea to better be safe than sorry when it comes to null-deref.
So regardless, if anyone gets a pointer, he is forced to check whether it can be null or not, but by looking for comment stating that,
whereas with a reference it's impossible to even think if it can be dangling unless one questions the validity of the entire stack. At least in this code.
So I'm afraid I can't agree to change to prefer T* over T& if we know it's alwas non-null non-dangling.

The glooxwrapper code also showed me another problem with pointers.
Sometimes you get a pointer and the obligation to delete it.
And then you get a leak if you don't delete it!
If you get a reference, you know that you don't have that obligation.

We use words (in a programming language or not) because we want to express some meaning. Pointer and reference share some meaning, but they importantly differ.
The meaning of a reference is that it can never be dangling unless someone broke something hard, but the meaning of a pointer is that it is a freely settable address that may have been set to null.

It is good practice to initialize all member variables with their correct value at construction time, that's the task description of a constructor.

I don't disagree with that.

Thanks guys for discussing this at length, though it got quite heated in the end. It's important that we keep exchanging even when it seems that no one will ever be right.

If I may elexis, What is you current goal ? Do you plan to cleanup the whole code base ?

In D2205#91665, @elexis wrote:

But I do remember precisely you recommending me to add if-safeguards for other places in the code even after I mentioned that it's always given, and then you even convinced me that it's a good idea to better be safe than sorry when it comes to null-deref.

I suppose nothing was changed. You have to use safeguards if you don't have guarantees of a pointer lifetime.

So regardless, if anyone gets a pointer, he is forced to check whether it can be null or not, but by looking for comment stating that,
whereas with a reference it's impossible to even think if it can be dangling unless one questions the validity of the entire stack. At least in this code.
So I'm afraid I can't agree to change to prefer T* over T& if we know it's alwas non-null non-dangling.

My current point is more about T& in constructors with a direct member usage vs GetGUI.

And then you get a leak if you don't delete it!
If you get a reference, you know that you don't have that obligation.

It's true only for stack/member variables. But for heap objects it doesn't help.

I compare such kind of code:

With the reference member:

CList::CList(CGUI& pGUI)
	: IGUIObject(pGUI), IGUITextOwner(pGUI), IGUIScrollBarOwner(pGUI),
	  m_Modified(false), m_PrevSelectedItem(-1), m_LastItemClickTime(0)
{
    m_pGUI.Method();
}

With GetGUI:

CList::CList() : m_Modified(false), m_PrevSelectedItem(-1), m_LastItemClickTime(0)
{
    GetGUI().Method();
}

// IGUIObject.h
// It's guaranteed that CGUI exists during the GUI object lifetime.
CGUI& GetGUI();

rP22587 removed 21 of the if (GetGUI()) checks already, sometimes with a throw, sometimes with ENSURE, most often silent pass, so it is not only manifested in my personal brain.

The imagined reviewer said that CGUI pointers allow for tests to be written when a CGUI doesn't exist, such as the only GUI tests currently existing that test ParseString(CGUI*) (unchanged).
But my imagined answer to that reviewer would be that the IGUIObject widely depends on the ScriptInterface, so it would likely be required to create the CGUI and thus the ScriptInterface once during the tests, similar to what is done with the simulation tests and the uncommitted random map tests.
Even the only tested function ParseString gets the CGUI argument because it uses it in some cases, so passing nullptr there means that the function can't be tested fully and that it will require a CGUI eventually to achieve that.