Skip to content
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

Defines/Typos #557

Merged
merged 13 commits into from
Nov 20, 2013
Merged

Defines/Typos #557

merged 13 commits into from
Nov 20, 2013

Conversation

ZobyTwo
Copy link
Contributor

@ZobyTwo ZobyTwo commented Nov 17, 2013

These commits get rid of a few define groups and replaces them with enums (in class scopes).
There was also a typo in ActionLayer::receiveEventPlus(...) and two unused variables in Interruptible which are now gone.

Defines do not follow scope rules, but enums do. Enum classes
even utilize strong type checking (but i am not sure about compiler
support for c++11 on all target platforms).
This method is rarely used, however it returned 0 after adding stuff.
It now returns the sum.
Merge cleanup codings style commit as i accidentally used that branch as
the base.
@ZobyTwo
Copy link
Contributor Author

ZobyTwo commented Nov 18, 2013

I dont get why f2314ca compiles and 82e3ab3 does not.

@pankdm
Copy link
Member

pankdm commented Nov 18, 2013

Due to #558 I can't see the whole output. So it's difficult to say for sure what's wrong. Probably something with TestSuite.
I hope to fix #558 soon.

for(int w = 0; w < int(abilities.size());w++)
{
currentAction = (ActionElement*)abilities[w];
ActionElement *currentAction = (ActionElement*)abilities[w];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use ActionElement* currentAction or ActionElement * currentAction

@pankdm
Copy link
Member

pankdm commented Nov 19, 2013

Nice changes! I like when the code is getting cleaner.
However, I would prefer to merge the "green" build.
So I would suggest for you to wait until #559 is merged into master then rebase the changes over it and see the Travis result.

pankdm added a commit that referenced this pull request Nov 20, 2013
@pankdm pankdm merged commit a37af17 into WagicProject:master Nov 20, 2013
@ZobyTwo ZobyTwo deleted the defines branch November 20, 2013 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants