Page MenuHomeWildfire Games

Simple cleanup of Shapes
ClosedPublic

Authored by vladislavbelov on Apr 1 2019, 8:49 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22213: Simple cleanup of Shapes, removes old style format.
Summary

I removed useless comments, made the file more near to CC and removed useless C styles.

Test Plan
  1. Apply the patch and compile the game
  2. Run tests
  3. Run the game

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7080
Build 11561: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.Apr 1 2019, 8:49 PM
Vulcan added a subscriber: Vulcan.Apr 1 2019, 9:02 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Shapes.cpp
| 238| »   return·x·==·a.x·&&·y·==·a.y);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character (() when these macros are defined: ''.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1150/display/redirect

Vulcan added a comment.Apr 1 2019, 9:26 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1151/display/redirect

elexis added a subscriber: elexis.Apr 1 2019, 10:04 PM

You did a good write up on the logic change, not the syntax candy change at https://wildfiregames.com/forum/index.php?/topic/25594-c-tip-1-constant-references/&tab=comments#comment-371946
Should mention it here too, at least that there is such a logic change at all.

Tests for this logic change?
If not, we might consider adding a comment that explains in which cases const ref would trigger the possibly less expected behavior.

vladislavbelov added a comment.EditedApr 1 2019, 10:18 PM
In D1809#73829, @elexis wrote:

Tests?

I plan to add it later. I have a big queue. But there is no logic change.

If not, we might consider adding a comment that explains in which cases const ref would trigger the possibly less expected behavior.

True.

source/ps/Shapes.h
190

We pass basic types by value. Also the previous version leads to bug with aliasing:

CSize size(2560, 1440);
// Normalize the size.
if (size.cx)
    size /= size.cx;
debug_printf("%f %f", size.cx, size.cy);

Expected value is 1.0 1.78, but the real output will be 1.0 1440.0, because the const float& a references to cx, and the first dividing assign 1.0 to it. So the second dividing is useless in such cases.

So it's good that we publish the version history of our source, so people can find this information.
But even better might be adding the comment to the code itself, so that people stubmle upon it while reading the code.

In D1809#73904, @elexis wrote:

But even better might be adding the comment to the code itself, so that people stubmle upon it while reading the code.

About what? It's too common thing and it's related to many places in the code. So I'd prefer to add comments to our CC.

In D1809#73904, @elexis wrote:

But even better might be adding the comment to the code itself, so that people stubmle upon it while reading the code.

About what? It's too common thing and it's related to many places in the code. So I'd prefer to add comments to our CC.

This commit is not a cleanup, but a bugfix with some additional cleanup.
So it makes sense to add a comment so that people don't reintroduce the bug.
It doesn't cost much, just one line.

// Do not use const references here to allow passing cx and cy to the operators!
Could be added at the top of the file.

Also one may consider splitting CRect, CSize, CPos to different files before the fourth and fifth class is added.
If one splits in the same commit, every line will be touched only once, so minimizing lines of code changed a bit, only one commit to audit instead of two.
(The comment could be added once per file if split)

In D1809#73924, @elexis wrote:

This commit is not a cleanup, but a bugfix with some additional cleanup.
So it makes sense to add a comment so that people don't reintroduce the bug.

Then add it to CC.

// Do not use const references here to allow passing cx and cy to the operators!
Could be added at the top of the file.

Then it should be added in each file in the project. Because the bug may appear in each file.

Also one may consider splitting CRect, CSize, CPos to different files before the fourth and fifth class is added.
If one splits in the same commit, every line will be touched only once, so minimizing lines of code changed a bit, only one commit to audit instead of two.
(The comment could be added once per file if split)

I don't see a strong reason to split. Since all these classes are grouped.

elexis added a comment.Apr 3 2019, 2:42 PM

Then add it to CC.

CC is to allow people to chose between multiple syntactically and semantically correct solutions.
It's not a convention that we avoid bugs.
It could be added under a different chapter or different document: Common Pitfalls or so.

Then it should be added in each file in the project. Because the bug may appear in each file.

Not really in every file, it can only happen for few files where the argument can be a variable that is written to and read afterwards again. For most classes with private/public distinction should protect from that. Perhaps it's more common than I think though.
The difference between this file and other files is that this file already had the bug, and that it's not so obvious as it only happens when one passes cx or cy.
It's not unlikely someone comes along and thinks he might optimize it or add the reference for consistency without thinking about this case.

But yes, a good developer shouldn't need the comment. Makes me wonder who added the defect.
Seems like rP288.

I don't see a strong reason to split. Since all these classes are grouped.

Indeed, good reasons to split would be to make headers more clear, but here there are none. And the file is short. And you don't change half the lines. If one prefers one file per class while it's something that could be done while at it, or not.

In D1809#73950, @elexis wrote:

Then add it to CC.

CC is to allow people to chose between multiple syntactically and semantically correct solutions.
It's not a convention that we avoid bugs.

But CC helps to make less mistakes.

It could be added under a different chapter or different document: Common Pitfalls or so.

True.

Not really in every file, it can only happen for few files where the argument can be a variable that is written to and read afterwards again. For most classes with private/public distinction should protect from that.

But there are also classes that return just a reference, structs where all fields are public.

The difference between this file and other files is that this file already had the bug, and that it's not so obvious as it only happens when one passes cx or cy.
It's not unlikely someone comes along and thinks he might optimize it or add the reference for consistency without thinking about this case.
But yes, a good developer shouldn't need the comment. Makes me wonder who added the defect.

The problem here is you don't know when you need to draw a line. Someone may pass (float&) or (CSize size), forget ) const and so on. Do you need to add a comment about these too?

Why I drew a line to not add a comment, because the problem is independent of the current file. It may happen for every file.

Indeed, good reasons to split would be to make headers more clear, but here there are none. And the file is short.

I don't think that the header would be more clear, only smaller.

And you don't change half the lines.

I prefer to not move and change lines at the same step. It's harder to review and easier to make a mistake.

elexis added a comment.Apr 3 2019, 3:11 PM

I don't think that the header would be more clear, only smaller.

(I meant the includes, not the header. Sometimes we find something like 30 includes in a file with 3 classes, 10 of them for one class, 10 for another, 10 for the last class, then it's hard to tell what is forgotton, what is unused and what include is used by which class (here are no includes, so there's that))

wraitii accepted this revision.Apr 13 2019, 11:11 AM
wraitii added a subscriber: wraitii.

This is a sneaky bug lol. i guess it's more of a logic issue (as a can indeed refer to our very own variable and we don't check for that). I haven't actually encountered this as a "classic mistake you might make in C++" when it feels like it might be.

I think as a rule we don't pass trivial types by reference as it's generally useless - the rare few exceptions should be made explicit - so the value of adding a comment above the specific functions feels low. Up to you imo.

This revision is now accepted and ready to land.Apr 13 2019, 11:11 AM
This revision was automatically updated to reflect the committed changes.