Paths

Table of Contentst

Differential D870

Remove deepcopy clone
ClosedPublic
Actions

Authored by elexis on Sep 4 2017, 11:12 PM.

Details

Reviewers

• leper

Commits

rP20125: Unify deepcopy and clone.

Summary

rP15421 introduced a clone function in globalscripts/ which, besides not being able to handle a variety of types, does the same as the deepcopy function in ScriptInterface.cpp.
https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

IIRC, FeXoR and fatherbushido had been confused just like me which one to use. Since there is no relevant difference, we can nuke the copy.
Since the verb clone is widely used in the code, I'd vote to keep that name.
Notice we can't rename the original deepcopy function to clone because that name is reserved:
http://man7.org/linux/man-pages/man2/clone.2.html

Test Plan

Agree with the approach (Optionally notice that every JS use of clone copies an object or array).

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

elexis created this revision.Sep 4 2017, 11:12 PM

Notice we can't rename the original deepcopy function to clone because that name is reserved:

I'd be surprised if we actually pulled in the headers that define that one. So why not rename that too? (Not that we have to, but it'd make finding it slightly easier.)

Also 26 vs 15 uses of clone vs deepcopy, no real preference on what to use though. While there is there actually a need for Vector{2,3}D.prototype.clone?

Apart from that looks good. (Counting the number of lines changed in places that had deepcopy results in 2*15, which is the right amount.)

Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'tries'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/belgian_uplands.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/belgian_uplands.js
| 266| 266| 		if (maxMinDist > minDistBetweenPlayers)
| 267| 267| 		{
| 268| 268| 			goodStartPositionsFound = true;
| 269|    |-			log("Exiting giant while loop after " +  tries + " tries with a minimum player distance of " + maxMinDist);
|    | 269|+			log("Exiting giant while loop after " + tries + " tries with a minimum player distance of " + maxMinDist);
| 270| 270| 		}
| 271| 271| 		else
| 272| 272| 			log("maxMinDist <= " + minDistBetweenPlayers + ", maxMinDist = " + maxMinDist);

binaries/data/mods/public/maps/random/belgian_uplands.js
| 302| »   »   »   »   if·(i·==·0)·//·...deep·water
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/belgian_uplands.js
| 432| »   »   »   if·(j·%·2·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
| 128| »   »   »   if·(avoidPoints.every(ap·=>·getDistance(checkpoint.x,·checkpoint.y,·ap.x,·ap.y)·>·minDistance))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
| 144| »   »   if·(placements.every(p·=>·getDistance(p.x,·p.y,·checkPoint.x,·checkPoint.y)·>·minDistance))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
| 207| »   »   »   »   if·(x·%·2·==·0·&&·y·%·2·==·0)·//·Old·tile
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
| 207| »   »   »   »   if·(x·%·2·==·0·&&·y·%·2·==·0)·//·Old·tile
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
| 381| »   »   if·(placements.every(p·=>·getDistance(p.x,·p.y,·point.x,·point.y)·>·max(minDistance,·p.dist)))
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
| 386| »   »   if·(tries·!=·0·&&·tries·%·100·==·0)·//·Time·Check
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/maps/random/heightmap/heightmap.js
| 386| »   »   if·(tries·!=·0·&&·tries·%·100·==·0)·//·Time·Check
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'actor' to undefined.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/wild_lake.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/wild_lake.js
| 630| 630| 	{
| 631| 631| 		let x = areas[h][t].x;
| 632| 632| 		let y = areas[h][t].y;
| 633|    |-		let actor = undefined;
|    | 633|+		let actor;
| 634| 634| 		let texture = pickRandom(wildLakeBiome[h].texture);
| 635| 635| 
| 636| 636| 		if (slopeMap[x][y] < 0.5 * (minSlope[h] + maxSlope[h]))

binaries/data/mods/public/maps/random/wild_lake.js
| 247| »   groveActors·=·[g_Decoratives.grass,·g_Decoratives.rockMedium,·g_Decoratives.bushMedium],·groveTileClass·=·undefined,
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'groveActors' is already declared in the upper scope.

binaries/data/mods/public/maps/random/wild_lake.js
|  18| randomizeBiome()
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/maps/random/wild_lake.js
| 259| »   »   if·(i·%·3·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binari

http://jenkins-master:8080/job/phabricator_lint/481/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1966/ for more details.

Harbormaster completed remote builds in B3007: Diff 3489.Sep 5 2017, 2:49 AM

elexis mentioned this in D871: Deepfreeze entity and template in simulation components tests.Sep 6 2017, 8:52 PM

In D870#33970, @leper wrote:

surprised if ... pulled .. headers

Ack. Eclipse lead me to that header, it must be lying. I can compile with the complete rename. No occurrence of #include <sched.h>, so it likely won't fall apart.

no real preference

clone is well established and feels more sound to me than deepcopy

is there a need for Vector{2,3}D.prototype.clone?

Ack, amputating.
No occurrence of .clone and ["clone"] in JS files.
Could confirm that clone(someVector) works as advertized.

Sorry, I compiled before making the rename apparently.

../../../source/scriptinterface/ScriptInterface.cpp: In constructor ‘ScriptInterface_impl::ScriptInterface_impl(const char*, const std::shared_ptr<ScriptRuntime>&)’:
../../../source/scriptinterface/ScriptInterface.cpp:392:124: error: invalid conversion from ‘int (*)(int (*)(void*), void*, int, void*, ...) throw ()’ to ‘JSNative {aka bool (*)(JSContext*, unsigned int, JS::Value*)}’ [-fpermissive]
  JS_DefineFunction(m_cx, globalRootedVal, "clone", ::clone,        1, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);
                                                                                                                            ^
In file included from ../../../source/scriptinterface/ScriptTypes.h:61:0,
                 from ../../../source/scriptinterface/ScriptInterface.h:26,
                 from ../../../source/scriptinterface/ScriptInterface.cpp:20:
../../../libraries/source/spidermonkey/include-unix-release/jsapi.h:3146:1: note:   initializing argument 4 of ‘JSFunction* JS_DefineFunction(JSContext*, JS::Handle<JSObject*>, const char*, JSNative, unsigned int, unsigned int)’
 JS_DefineFunction(JSContext* cx, JS::Handle<JSObject*> obj, const char* name, JSNative call,
 ^~~~~~~~~~~~~~~~~
../../../source/scriptinterface/ScriptInterface.cpp: At global scope:
../../../source/scriptinterface/ScriptInterface.cpp:207:6: warning: ‘bool {anonymous}::clone(JSContext*, uint, JS::Value*)’ defined but not used [-Wunused-function]
 bool clone(JSContext* cx, uint argc, JS::Value* vp)
      ^~~~~

Not becoming smarter from those errors, so assuming it pulls the mentioned header:
int (*)(int (*)(void*), void*, int, void*, ...) throw ()

int clone(int (*fn)(void *), void *child_stack,
          int flags, void *arg, ...
          /* pid_t *ptid, void *newtls, pid_t *ctid */ );

Remove unused Vector clones

Would probably work with clone instead of ::clone, but meh.

This revision is now accepted and ready to land.Sep 6 2017, 10:45 PM

In D870#34347, @leper wrote:

Would probably work with clone instead of ::clone

Passes the compilation here.

Using ::deepfreeze instead of clone however keeps the JS_DefineFunction calls consistent and the resolution unambiguous.

Thanks for the review!

Closed by commit rP20125: Unify deepcopy and clone. (authored by elexis). · Explain WhySep 6 2017, 10:58 PM

This revision was automatically updated to reflect the committed changes.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1985/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/500/ for more details.

Harbormaster completed remote builds in B3044: Diff 3545.Sep 6 2017, 11:16 PM

clone(vector2d) does create a new object with the same x and y, but the functions are absent. So that change to the file was wrong too.
I do not recall what I tested (which is the reason why it should always be stated in the comments what exactly was tested).

In D870#37128, @elexis wrote:

clone(vector2d) does create a new object with the same x and y, but the functions are absent. So that change to the file was wrong too.
I do not recall what I tested (which is the reason why it should always be stated in the comments what exactly was tested).

Well, that'd be a reason for that to exist, feel free to propose a reversal of that part.