Related to #3403. This should fix style issues noted by Leper.
Details
Details
Code review and verification that behaviour isn't broken.
Diff Detail
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
Comment Actions
Looks a lot better already.
| source/gui/CChart.cpp | ||
|---|---|---|
| 143 ↗ | (On Diff #82) | Have you checked whether this actually works when we end up calling Draw more than once? |
| source/scriptinterface/ScriptConversions.cpp | ||
| 418 ↗ | (On Diff #82) | It might still be worth thinking about whether using this array representation is nicer than using Vector2D. |
| source/scriptinterface/ScriptConversions.h | ||
| 21 ↗ | (On Diff #82) | What is this doing in a header? |
| 24 ↗ | (On Diff #82) | Is this line really needed here? IIRC JS_IsTypedArrayObject is not defined in there, and we do not want to add lots of includes to headers. |
| 37 ↗ | (On Diff #82) | There should be a macro constant or std::numeric_limits template around that makes this a bit more obvious. |
Comment Actions
Build has FAILED
Updating workspaces.
Build (release)...
../../../source/scriptinterface/ScriptConversions.cpp:20:31: fatal error: ScriptConversions.h: No such file or directory
#include "ScriptConversions.h"
^
compilation terminated.
make[1]: *** [obj/scriptinterface_Release/ScriptConversions.o] Error 1
make: *** [scriptinterface] Error 2
make: *** Waiting for unfinished jobs....
../../../source/simulation2/scripting/EngineScriptConversions.cpp:20:47: fatal error: scriptinterface/ScriptConversions.h: No such file or directory
#include "scriptinterface/ScriptConversions.h"
^
compilation terminated.
make[1]: *** [obj/simulation2_Release/EngineScriptConversions.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [simulation2] Error 2Link to build: http://jw:8080/job/phabricator/52/
See console output for more information: http://jw:8080/job/phabricator/52/console
Comment Actions
Build is green
Updating workspaces.
Build (release)...
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
Status ret = ERR::FAIL;
^
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!http://jw:8080/job/phabricator/53/ for more details.
| source/gui/CChart.cpp | ||
|---|---|---|
| 143 ↗ | (On Diff #82) | On second thought, I think this wouldn't have worked, so I changed it to only call UpdateSeries when something changes, and put the copy back in. |
| source/scriptinterface/ScriptConversions.cpp | ||
| 418 ↗ | (On Diff #82) | the JS Vector2D you mean? |
| source/scriptinterface/ScriptConversions.h | ||
| 24 ↗ | (On Diff #82) | Needed also for Js::ToNumber so I kept it. |
Comment Actions
Build is green
Updating workspaces.
Build (release)...
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
Status ret = ERR::FAIL;
^
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!http://jw:8080/job/phabricator/73/ for more details.
| source/gui/CChart.cpp | ||
|---|---|---|
| 32 ↗ | (On Diff #108) | Not entirely sure that this is needed. |
| 44 ↗ | (On Diff #108) | That's not how we format switch statements. |
| source/scriptinterface/ScriptConversions.cpp | ||
| 418 ↗ | (On Diff #82) | Yes, though this one is probably a bit more memory efficent, though I don't think the code in question should have to care about that. (Not really requesting a change here, but maybe someone else has another opinion.) |
| source/scriptinterface/ScriptConversions.h | ||
| 26 ↗ | (On Diff #108) | This should not leak out of the header, undef it. While at it, you should probably move the definition closer to the usage. |
| 37 ↗ | (On Diff #108) | A blank line above this would be nice. |
| 77 ↗ | (On Diff #108) | Some indentation might be nice here. Same for the one right below. |
| 24 ↗ | (On Diff #82) | None of those is needed in this header AFAICS. |
Comment Actions
Fixes above.
| source/scriptinterface/ScriptConversions.h | ||
|---|---|---|
| 24 ↗ | (On Diff #82) | We do call JS_IsTypedArrayObject in this header, unless you mean something else? |
Comment Actions
Build is green
Updating workspaces.
Build (release)...
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
Status ret = ERR::FAIL;
^
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!http://jw:8080/job/phabricator/78/ for more details.
Comment Actions
I guess I'll have to actually test this soon. Actually is there any user for this code included?
| source/scriptinterface/ScriptConversions.cpp | ||
|---|---|---|
| 29 ↗ | (On Diff #115) | Why move this from the old position in this file? |
| source/scriptinterface/ScriptConversions.h | ||
| 24 ↗ | (On Diff #82) | Ah, that is indeed in jsfriendapi.h which is only in that header (as opposed to eg JS_IsArrayObject which is in jsapi.h), but then you should list JS_IsTypedArrayObject as the reason for this header, and only that (as none of the others is actually used in this header). Though I guess we could get away with a lot given that the below are templates and only insantiated later, so forward decls should work, but that might mean some more work for the few users of this. Your call. |
Comment Actions
Remove the Fail macro in the header, it was hardly that useful anyway. Fix comment.
As for the test case, there are JS files in the original ticket, but they weren't committed because they had no labels or anything.
Comment Actions
Build is green
Updating workspaces.
Build (release)...
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
Status ret = ERR::FAIL;
^
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!http://jw:8080/job/phabricator/82/ for more details.
Comment Actions
With the latter of these two fixed, your call on the former. Also update the copyright year.
| source/scriptinterface/ScriptConversions.cpp | ||
|---|---|---|
| 29 ↗ | (On Diff #115) | ... |
| source/scriptinterface/ScriptConversions.h | ||
| 50 ↗ | (On Diff #119) | This was IMO nicer to read with the macro, though I guess one should make the name a bit less generic as to not redefine something else (though that would cause warnings), and undef it after use. |