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

More New API changes for MapChooser, Nominations, RockTheVote, RandomCycle, and NextMap #226

Merged
merged 18 commits into from Jun 12, 2015
Merged

Conversation

powerlord
Copy link
Contributor

This depends on pull request 225 and mapchooser will not compile without it.

Changes include things like:

  • Converting old style variable declaration to new style
  • Converting old-style casts to view_as
  • Converting from Handle to ArrayList where adt_arrays are being used.

There are also misc cleanups that were missed prior to this:

  • Converting hard-coded iength for map names to be PLATFORM_MAX_PATH instead of 32, 33, or 64.
  • Converting hard-coded lengths for client names to MAX_NAME_LENGTH+1 (yay null terminator).

Note: This is only the first upgrade I have planned for MapChooser. I also have plans for introducing fixes for various issues with different games, but those need more testing.

@powerlord
Copy link
Contributor Author

OK, question time to those who are looking at this code.

Is it better to continue passing INVALID_HANDLE to CreateTimer, or should I be passing view_as<DataPack>(null) when the Timer function is expecting a DataPack?

@dvander
Copy link
Member

dvander commented Dec 16, 2014

I'd be okay with allowing an implicit coercion from null to any, if that is the problem.

@powerlord
Copy link
Contributor Author

@dvander Well, I guess the question is... do we want to get rid of the INVALID_HANDLE constant wherever possible? If we do, then we may want to allow an implicit conversion of null to any to allow us to use null in places like CreateTimer.

@powerlord
Copy link
Contributor Author

I realized I missed converting nextmap.inc despite converting nextmap.sp.

Also, updated the copyright years to 2014 to the files I've edited.

@powerlord
Copy link
Contributor Author

er... ignore the newdecls warning commit (f500e20). I'm going to roll that back and make a new pull request for it.

@powerlord powerlord closed this Jan 19, 2015
@powerlord powerlord deleted the mapchooser-updates branch January 19, 2015 21:33
@powerlord powerlord restored the mapchooser-updates branch March 30, 2015 20:51
@powerlord powerlord reopened this Mar 30, 2015
@powerlord
Copy link
Contributor Author

Side note: Since this hasn't been mentioned yet, this pull request also fixes a few locations that were still hard-coding 32, 64, or 65 for the map name string.

@psychonic
Copy link
Member

Whoops! I completely forgot about this PR.

Side note: Since this hasn't been mentioned yet, this pull request also fixes a few locations that were still hard-coding 32, 64, or 65 for the map name string.

I took care of this yesterday in #343, landing shortly, but there shouldn't be a conflict since both use PLATFORM_MAX_PATH for the sizes now.

I do see a couple of error reported on this though, due to the added "#pragma newdecls required":

C:\projects\sourcemod\plugins\mapchooser.sp(562) : error 240: 'MenuAction:' is an old-style tag operation; use view_as(expression) instead
C:\projects\sourcemod\plugins\mapchooser.sp(794) : error 240: 'MenuAction:' is an old-style tag operation; use view_as(expression) instead

Do you mind fixing the errors and merging master back into your branch since it's been quite some time (our fault)?

@powerlord
Copy link
Contributor Author

@psychonic The errors you're seeing are actually a bug in menus.inc and were fixed in master by PR #229.

@psychonic
Copy link
Member

Touché.

@powerlord
Copy link
Contributor Author

Urgh, attempting to merge master into this is introducing all sorts of... fun... conflicts.

@powerlord
Copy link
Contributor Author

I seriously hope this didn't mess up the sourcepawn submodule. I had to Google how to resolve it. Which was to git revert sourcepawn it, then git rm sourcepawn.

Edit: Also, I didn't edit the commit message, which apparently listed the conflicts I had just manually resolved as not being resolved. :/

@powerlord
Copy link
Contributor Author

sigh of course it f'ed it up. Time to rollback and try again.

@@ -51,7 +82,7 @@ native bool:RemoveNominationByOwner(owner);
* @param array An ADT array handle to add the map strings to.
* @noreturn
Copy link
Member

Choose a reason for hiding this comment

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

You can ditch these noreturn tags for void returns if you want. They were necessary before it was clear whether a call even returned a value or not.

@psychonic
Copy link
Member

All of the newdecl and buffer size updates look good. I still need to review the actual functionality changes, which is fortunately only a small minority of this.

@psychonic
Copy link
Member

I misread the PR and some of the code I guess. "New API" is referring to the new-style syntax and methodmaps, not new mapchooser API. I thought that I had spotted some changes to the nominations API. Will take another look.

@psychonic
Copy link
Member

I thought that I had spotted some changes to the nominations API. Will take another look.

It was the comments on the native impls that were throwing me off, making it look like changes that were really just updating a comment to match the prototype in the inc.

All looks good. Thanks for updating these.

psychonic added a commit that referenced this pull request Jun 12, 2015
More New API changes for MapChooser, Nominations, RockTheVote, RandomCycle, and NextMap.
@psychonic psychonic merged commit d0574db into alliedmodders:master Jun 12, 2015
@powerlord powerlord mentioned this pull request Jun 24, 2015
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.

None yet

3 participants