New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced a ton of ConsoleMethods with the DefineConsoleMethod Macro. #887

Closed
wants to merge 4 commits into
base: development
from

Conversation

Projects
None yet
4 participants
@Winterleaf
Contributor

Winterleaf commented Nov 4, 2014

Cleaned up the ConsoleMethods and converted them to DefineConsoleMethods. Mainly the ConsoleMethods left are the ones that take a variable amount of parameters. They will need to be cleaned up in a later pass.

@crabmusket crabmusket added the Defect label Nov 4, 2014

@crabmusket crabmusket added this to the 3.7 milestone Nov 4, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 4, 2014

Contributor

Awesome work, and thanks for the PR! This is a big step forwards.

Ubuntu fails because of including engineApi.h from telnetDebugger.cpp. Correct include is engineAPI.h.

Contributor

crabmusket commented Nov 4, 2014

Awesome work, and thanks for the PR! This is a big step forwards.

Ubuntu fails because of including engineApi.h from telnetDebugger.cpp. Correct include is engineAPI.h.

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Nov 4, 2014

This should probably use getName() on object. Otherwise it might make it difficult to figure out which UndoManager the error is coming from.

jamesu commented on Engine/source/util/undo.cpp in acb192e Nov 4, 2014

This should probably use getName() on object. Otherwise it might make it difficult to figure out which UndoManager the error is coming from.

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Nov 4, 2014

Not my favourite API in the world, but generally good.

Reaction

jamesu commented on acb192e Nov 4, 2014

Not my favourite API in the world, but generally good.

Reaction

@@ -0,0 +1,39 @@
#include "platform/platform.h"

This comment has been minimized.

@dottools

dottools Nov 4, 2014

Seems like an out of place change, moved out of Engine/source/gui/game/GuiChunkedBitmapCtrl.cpp, in this pull request.

@dottools

dottools Nov 4, 2014

Seems like an out of place change, moved out of Engine/source/gui/game/GuiChunkedBitmapCtrl.cpp, in this pull request.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 4, 2014

Contributor

Thanks @dottools for the detailed review! Hadn't gotten around to it myself. I'm curious whether all those == ""s actually work because of the StringTable. But they're still bad style and I agree with changing them. dStrcmp is probably faster as in this case it is O(1) not O(n), I believe.

Contributor

crabmusket commented Nov 4, 2014

Thanks @dottools for the detailed review! Hadn't gotten around to it myself. I'm curious whether all those == ""s actually work because of the StringTable. But they're still bad style and I agree with changing them. dStrcmp is probably faster as in this case it is O(1) not O(n), I believe.

@dottools

This comment has been minimized.

Show comment
Hide comment
@dottools

dottools Nov 4, 2014

Yeah... I wouldn't trust "" to point to the same memory location at all times since it is per compiler and compiler settings specific as to how that'll work. Not to mention with optimizations you just can't trust "" to be in the same memory location as another empty string ("") used in another source file.

An idea to tackle this situation, Andrew Mac suggested introducing a new dStrcmpEmpty() function to make it more uniform to test "" against const char* C strings. Both him and James Urquhart suggested a more simpler and faster approach to checking an empty string. So I took those ideas and came up with a C macro for comparing against empty strings:

#define dStrcmpEmpty(_var)  (*_var == '\0')

// Usage examples
if(dStrcmpEmpty(MissionAreaName))
{
   // MissionAreaName is an empty string
}
if(!dStrcmpEmpty(MissionAreaName))
{
   // MissionAreaName is not an empty string
}

I'm sure somebody could also come up with a C++ inlined template variant with type safety to ensure only variables of type const char* are used with it. I'm not that in-depth with C++ templates myself to come up with one. :)

Anyway, there's probably other problems I haven't spotted that hopefully others will take the initiative to review this PR as well, so we can make sure no other problems are left unnoticed.

dottools commented Nov 4, 2014

Yeah... I wouldn't trust "" to point to the same memory location at all times since it is per compiler and compiler settings specific as to how that'll work. Not to mention with optimizations you just can't trust "" to be in the same memory location as another empty string ("") used in another source file.

An idea to tackle this situation, Andrew Mac suggested introducing a new dStrcmpEmpty() function to make it more uniform to test "" against const char* C strings. Both him and James Urquhart suggested a more simpler and faster approach to checking an empty string. So I took those ideas and came up with a C macro for comparing against empty strings:

#define dStrcmpEmpty(_var)  (*_var == '\0')

// Usage examples
if(dStrcmpEmpty(MissionAreaName))
{
   // MissionAreaName is an empty string
}
if(!dStrcmpEmpty(MissionAreaName))
{
   // MissionAreaName is not an empty string
}

I'm sure somebody could also come up with a C++ inlined template variant with type safety to ensure only variables of type const char* are used with it. I'm not that in-depth with C++ templates myself to come up with one. :)

Anyway, there's probably other problems I haven't spotted that hopefully others will take the initiative to review this PR as well, so we can make sure no other problems are left unnoticed.

@Winterleaf

This comment has been minimized.

Show comment
Hide comment
@Winterleaf

Winterleaf Nov 4, 2014

Contributor

I’m going to be updating the pr here in a bit

(I removed some quoted text. @eightyeight)

Contributor

Winterleaf commented Nov 4, 2014

I’m going to be updating the pr here in a bit

(I removed some quoted text. @eightyeight)

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 5, 2014

Contributor

@dottools if it's only applicable to const char*... isn't that the opposite of when to use a template :P? Good idea on the empty cmp function though.

@Winterleaf seems like WaypointTesm has slipped in there, and there are still some instances of == "".

Contributor

crabmusket commented Nov 5, 2014

@dottools if it's only applicable to const char*... isn't that the opposite of when to use a template :P? Good idea on the empty cmp function though.

@Winterleaf seems like WaypointTesm has slipped in there, and there are still some instances of == "".

@Winterleaf

This comment has been minimized.

Show comment
Hide comment
@Winterleaf

Winterleaf Nov 5, 2014

Contributor

So what failed on this in linux? I have no idea how to read the log.

Contributor

Winterleaf commented Nov 5, 2014

So what failed on this in linux? I have no idea how to read the log.

@Winterleaf

This comment has been minimized.

Show comment
Hide comment
@Winterleaf

Winterleaf Nov 5, 2014

Contributor

All fixed now! Let me know if you all find anything else with it.

Contributor

Winterleaf commented Nov 5, 2014

All fixed now! Let me know if you all find anything else with it.

if(argc == 3)
dSscanf(argv[2], "%f %f %f", &pos.x, &pos.y, &pos.z);
else if(argc == 5)
if(ptOrX != "" && Y == "" && Z == "")

This comment has been minimized.

@crabmusket

crabmusket Nov 5, 2014

Contributor

Should Y and Z even be const char* type? Seems like they're used as floats. But either way, remove != "".

@crabmusket

crabmusket Nov 5, 2014

Contributor

Should Y and Z even be const char* type? Seems like they're used as floats. But either way, remove != "".

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Nov 5, 2014

Contributor

@Winterleaf note that I'd prefer all these strinc comparisons to use dStrcmp. Which, I think you did for the last batch :).

Contributor

crabmusket commented Nov 5, 2014

@Winterleaf note that I'd prefer all these strinc comparisons to use dStrcmp. Which, I think you did for the last batch :).

@Winterleaf

This comment has been minimized.

Show comment
Hide comment
@Winterleaf

Winterleaf Nov 6, 2014

Contributor

Fixed

Contributor

Winterleaf commented Nov 6, 2014

Fixed

DefineEngineFunction(calcExplosionCoverage, F32, (Point3F pos, S32 id, U32 covMask),(Point3F(0.0f,0.0f,0.0f), NULL, NULL),
//I could possible see a use with passing in a null covMask, but even that
//sounds flaky because it will be 100 percent if your saying not to take
//any thing in consideration for coverage. So I'm removing these defaults they are just bad.

This comment has been minimized.

@crabmusket

crabmusket Nov 29, 2014

Contributor

Please remove comment and add it to the GitHub issue (either in the description or as a comment). It will be recoverable using git blame if someone notices that the defaults have changed, and if nobody notices then they don't need to know that there used to be defaults.

@crabmusket

crabmusket Nov 29, 2014

Contributor

Please remove comment and add it to the GitHub issue (either in the description or as a comment). It will be recoverable using git blame if someone notices that the defaults have changed, and if nobody notices then they don't need to know that there used to be defaults.

This comment has been minimized.

@Winterleaf

Winterleaf Nov 30, 2014

Contributor

How exactly do you do that?

@Winterleaf

Winterleaf Nov 30, 2014

Contributor

How exactly do you do that?

This comment has been minimized.

@crabmusket

crabmusket Dec 1, 2014

Contributor

Use git blame?

@crabmusket

crabmusket Dec 1, 2014

Contributor

Use git blame?

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Dec 21, 2014

Contributor

Resubmitted with merge and fixes.

Contributor

crabmusket commented Dec 21, 2014

Resubmitted with merge and fixes.

@crabmusket crabmusket closed this Dec 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment