Removes not needed method and refactors classes.
Details
- Reviewers
wraitii - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22755: Cleanup Camera and CGameView, removes a not needed method and refactors classes.
- Apply the patch and compile the game
- Run tests and make sure that all are passed
- Run the game and atlas and make sure that everything works as before
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/graphics/Camera.h | 41| class·CCamera | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classCCamera{' is invalid C code. Use --std or --language to configure the language. source/graphics/GameView.h | 35| class·CGameView·:·private·Scene | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classCGameView:' is invalid C code. Use --std or --language to configure the language. source/tools/atlas/GameInterface/View.cpp | 211| » » //·not·in·every·call·to·g_Game->Update | | [MAJOR] CPPCheckBear (syntaxError): | | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/431/display/redirect
From CPPCheck, that you might want to fix.
Member variable 'CCamera::m_NearPlane' is not initialized in the constructor. Member variable 'CCamera::m_FarPlane' is not initialized in the constructor. Member variable 'CCamera::m_FOV' is not initialized in the constructor.
source/graphics/Camera.cpp | ||
---|---|---|
49 ↗ | (On Diff #9408) | Why not use a pointer here? Just using a reference doesn't guarantee that it will exist for the entire function call. |
source/graphics/Camera.cpp | ||
---|---|---|
49 ↗ | (On Diff #9408) | We don't store it, so it's not a problem since we're on the same stack. References aren't so simple as it may seem. For ex: // int can be replaced by a class type. int& foo() { int n = 42; return n; } int& x = foo(); std::cout << x << std::endl; // <- that's not correct, it may crash or print a wrong value. int foo() { int n = 42; return n; } const int& x = foo(); std::cout << x << std::endl; // <- that's correct, as a constant reference extends a lifetime of its object. // But that works only for local variables (not for an object members). |
source/graphics/Camera.cpp | ||
---|---|---|
49 ↗ | (On Diff #9408) | So according to your advice you should use a pointer here:
|
source/graphics/Camera.cpp | ||
---|---|---|
49 ↗ | (On Diff #9408) | We are not in C are we ? |
source/graphics/Camera.cpp | ||
---|---|---|
49 ↗ | (On Diff #9408) | Sorry, wrong example since its const, but I hope you get the point regardless. |
source/graphics/Camera.cpp | ||
---|---|---|
49 ↗ | (On Diff #9408) | In short my point about pointers/references:
|
source/graphics/Camera.cpp | ||
---|---|---|
145 ↗ | (On Diff #9408) | So this should use a pointer |
164 ↗ | (On Diff #9408) | So this should use a pointer |
189 ↗ | (On Diff #9408) | So this should use a pointer |
source/graphics/GameView.cpp | ||
386 ↗ | (On Diff #9408) | So this should return a pointer |
409 ↗ | (On Diff #9408) | So this should return a pointer^ |
612 ↗ | (On Diff #9408) | So this should use a pointer |
@elexis In my opinion yes to mostly all your So this should *. I think it's more clear. But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.
source/graphics/GameView.cpp | ||
---|---|---|
612 ↗ | (On Diff #9408) | I think it should be const CCamera& camera. |
I don't necessarily agree that pointers are always preferable to non-const refs (I think for function arguments it's fine, I dislike reference member variables though), but I strongly agree that there is little reason to modify the code only for reference <> pointer transformation.
So for T* it's obvious that it can be modified and for T& it is not obvious that it can be modified?
And for T* it is obvious that it cannot be null, because some comment in another file from X years ago says so?
First - I don't understand why you're getting so worked up about this, nor going at this so disingenuously.
The point is more that usually, pointers aren't constant, and usually, references are constant. Doing something different is unexpected, and thus ever so slightly more dangerous. One is probably more likely to forget that a reference isn't constant than a pointer. Now as I've said, I am OK with non-const references in function arguments, since I agree their non-nullability is nice there...
And for T* it is obvious that it cannot be null, because some comment in another file from X years ago says so?
Nobody ever said that. A pointer is obviously nullable. Checking for nullability everywhere doesn't make sense, though - and switching to a reference just says "don't check here". Cause you can't. But it does nothing to actually guarantee that the lifetime is being extended if some other code is doing something stupid. So the bug can still be there.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/graphics/Camera.h | 41| class·CCamera | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classCCamera{' is invalid C code. Use --std or --language to configure the language. source/graphics/GameView.h | 35| class·CGameView·:·private·Scene | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classCGameView:' is invalid C code. Use --std or --language to configure the language. source/tools/atlas/GameInterface/View.cpp | 211| » » //·not·in·every·call·to·g_Game->Update | | [MAJOR] CPPCheckBear (syntaxError): | | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/446/display/redirect
Because you have wasted several hours of my lifetime, telling me that my patch is bad because it changes code without you understanding the point of it, accusing me of not understanding references, accusing me of being disingenous, making strawmans and proposing to change the coding conventions to use pointers when it can use references.
nor going at this so disingenuously.
I find this accusation quite worrying and I will remember it.
The point is more that usually, pointers aren't constant, and usually, references are constant.
If you remove the word "usually" the sentence is completely false.
So what does "usually" mean? The Google code that Vladislav linked? If they have one consistent codebase I would add code consistently. Or our codebase? Or the ideal codebase to which we should move?
Vladislav is actually arguing to prefer pointers when references are just as valid, and this has objective disadvantages.
Doing something different is unexpected, and thus ever so slightly more dangerous. One is probably more likely to forget that a reference isn't constant than a pointer.
So T* appears changeable, T& appears constant, no I don't follow. One could also confuse a pointer or a reference with a copy.
But one thing that cannot happen is that one confuses a pointer that can be null with a pointer that cannot be null if one uses a reference.
And for T* it is obvious that it cannot be null, because some comment in another file from X years ago says so?
Nobody ever said that. A pointer is obviously nullable. Checking for nullability everywhere doesn't make sense, though - and switching to a reference just says "don't check here". Cause you can't
Yes a pointer can assume null. A reference cannot. That is the point of prefering a reference if non-null is given.
The reason to prefer a reference is that you don't have to look at a function comment or at the callstack to determine whether you have to check for null or not.
But it does nothing to actually guarantee that the lifetime is being extended if some other code is doing something stupid. So the bug can still be there.
Nor did I ever claim that using a reference changes the lifetime of the referred value for the fifth time in the last 24 hours.
Patch looks cool.
Because you have wasted several hours of my lifetime, telling me that my patch is bad because it changes code without you understanding the point of it
I think I've perfectly understood your patch and I still think it's bad (or rather, not worth committing). I have to guess at this point that you believe this to be so unthinkable that you must assume I've misunderstood your patch to reconcile that with reality somehow?
You still haven't acknowledged this remark:
But currently I don't see a strong reason to modify the code only for reference <> pointer transformation.
I would have accepted "well, I believe the gain here of explicitly not needing to check for nulls is worth it." I would disagree, but you know that's a thing that happens in life, and it wouldn't be a big deal.
For some reason which I can't fathom you made a whole pataquès out of this, as if we were debating some universal law of something. And then when both I and Vlad said we disagreed with your patch (perhaps for different reasons), you quoted Gandhi and said you were right anyways and didn't care for our opinon. It's a bit hard to take you seriously.
proposing to change the coding conventions to use pointers when it can use references.
-_-
I find this accusation quite worrying and I will remember it.
¯\_(ツ)_/¯
The point is more that usually, pointers aren't constant, and usually, references are constant.
If you remove the word "usually" the sentence is completely false.
Why even write that? Yes, if you change words, sentences change meaning? This is exactly what I mean with "disingenusouly" above.
So what does "usually" mean?
Well if you do want some more details, I would ask you to fetch all c++ LOC from GitHub, check how many times a reference is const vs non-const, and do the same for pointers. I have no doubt that my "usually" above would stand.
So T* appears changeable, T& appears constant, no I don't follow.
Well that's where you should stop discussing this, because we obviously speak from different understandings of the universe, and they seem completely impossible to reconcile.
As for me, I enjoy trolling too much to stop just now.
The reason to prefer a reference is that you don't have to look at a function comment or at the callstack to determine whether you have to check for null or not.
And (as I've said several times too if you want to play this game), I think this 'pro' of references is not worth the cost of having a non-const member-reference in IGuiObject. But again, you've not addressed that comment, I guess you don't see the point of it.