Feature Type Safety & Visibilty Mega-thread

[Catch-22]

I'll post the attachments to some of the type safety and variable/class visibility changes here.

I'm keeping the patches for each .java file separate, so whoever decides to review them please let me know which ones you have committed and I'll remove the associated attachments to help keep track of things. I'll also bump this thread instead of creating a new one for future patches of the same variety.

Visibility changes are based on scope, but I tried not to make too many assumptions (there may be some incorrect assumptions though). For example, some classes have public methods/variables that aren't used in a public scope, but I've left them as public because it looks like there may be intention to use them as such in the future.

 

[Catch-22]

*

 

[Catch-22]

*

 

[Theraze]

Suggestion: Including version number in file name. I know that opening the patch file itself does let people know which version it's for, but including it in the file name helps if a post gets edited, since otherwise we're only able to guess which file has been examined based on how many times it's been downloaded. Which doesn't really have too much basis on how long it's been waiting.

 

[lostcalpolydude]

I don't have access to my primary computer now, and won't for a few weeks since I'm moving across the ocean (and my stuff is being shipped separately). I don't really feel comfortable making changes like that without an IDE, and I wouldn't want to run one from this old laptop, so I will probably take a look at the patches in a little while.

One thing I noticed is that a lot of type safety warnings are due to custom list types used for many things in KoLConstants, and those not taking parameters. Fixing those would either require updating the classes defining those list types, or possibly switching to some other list type if a suitable one happened to be introduced with 1.5. I don't really know how to make a good choice for what type of list to use in general, so I haven't looked into it much.

 

[Catch-22]

One thing I noticed is that a lot of type safety warnings are due to custom list types used for many things in KoLConstants, and those not taking parameters. Fixing those would either require updating the classes defining those list types, or possibly switching to some other list type if a suitable one happened to be introduced with 1.5. I don't really know how to make a good choice for what type of list to use in general, so I haven't looked into it much.

The KoLconstants is a different beast, I can't just fix the type safety or it causes a compile error, so yes it needs to be reworked, as well as a few other classes (eg. some of the SortedLists for AdventureResults, and another class used by the maximizer that I can't remember at the moment). The fixes above are meant to be standalone fixes that can be applied in any order that will still compile fine without errors.

If I were to refactor some of the more involved classes it would need to be more than just a matter adding type safety, or they wouldn't compile. I will probably submit those as a separate, stand alone patch that patches all the necessary classes at once.

Oh and I hope everything goes well with the move

 

[roippi]

Hilariously I also am moving, albeit within the same city. My dev box won't be up for another few days.

 

[Winterbay]

It's a good thing that nothing's happening in the game now then that may need supporting

 

[fronobulax]

It's a good thing that nothing's happening in the game now then that may need supporting

That's what the deep bench is for ;-)

 

[roippi]

r11718

I left in a couple of these:

		default:
			break;

that you had added. I don't know if it's just syntactic preference or what have you, but that seems to me like adding if( true ) statements.

edit: I'll leave this thread open for anyone who wants to post type-safety-only (or method visibility) patches.

 

[Catch-22]

Thanks!

I left in a couple of these:

		default:
			break;

that you had added. I don't know if it's just syntactic preference or what have you, but that seems to me like adding if( true ) statements.

My syntactic preference is to leave them out, but because those were switching on enum types, JLS 14.11 encourages a case label for each enum constant as well as a default case.

A Java compiler is encouraged (but not required) to provide a warning if a switch on an enum-valued expression lacks a default label and lacks case labels for one or more of the enum type's constants.

In reality, they will compile to the same bytecode (ie. no difference at run-time), but I think it's better to be clear on intent as well as keep compilers happy. Eclipse have a decent article on it, if you wanted to read more.

I'll leave this thread open for anyone who wants to post type-safety-only (or method visibility) patches.

That's the idea I plan on adding more to this thread anyway.