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

Implementation Code Cleanup #1932

Merged
merged 7 commits into from
Jun 13, 2018
Merged

Implementation Code Cleanup #1932

merged 7 commits into from
Jun 13, 2018

Conversation

gabizou
Copy link
Member

@gabizou gabizou commented Jun 5, 2018

Overall code cleanup for a bunch of compiler warnings. This is a WIP and of course, the cleanup is intended only for cleanup, no actual functionality changes.

This should improve however some corner cases that can occur as nullability and validation checks can be better written for certain areas of code.

…sion to clean up some compiler warnings.

Signed-off-by: Gabriel Harris-Rouquette <[email protected]>
@phit
Copy link
Contributor

phit commented Jun 6, 2018

nice, there's a few little things here that could be nice to include #23

@@ -1007,7 +1007,7 @@ public static EntityItem entityOnDropItem(Entity entity, ItemStack itemStack, fl
* @param offsetY The offset y coordinate
* @return The item entity
*/
@SuppressWarnings({"unchecked", "rawType"})
@SuppressWarnings({"unchecked", "rawType", "rawtypes"})
Copy link
Member

Choose a reason for hiding this comment

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

Isnt the old rawType a typo and thus can be removed?

* @param random The random instance
* @return The motion vector
*/
@SuppressWarnings("unused")
Copy link
Member

Choose a reason for hiding this comment

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

Why should we keep the unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

Item pre-merging will be reintroduced, just not right now.

Signed-off-by: Gabriel Harris-Rouquette <[email protected]>
.buildAndSwitch()) {
handler.getTimingsHandler().startTimingIfSync();
.buildAndSwitch();
final Timing timings = handler.getTimingsHandler()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as here, note how it lines up with CauseStackManager

Copy link
Member

Choose a reason for hiding this comment

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

I guess he referred to it being 5 spaces instead of 4 or 8. IIRC 8 is a common standard for this. Aligning it as it is here is also used but less common. As always: styles are just personal preferences.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, looked really odd to me

@@ -411,16 +414,18 @@ private boolean post(Event event, List<RegisteredListener<?>> handlers) {
try (CauseStackManager.StackFrame frame = Sponge.getCauseStackManager().pushCauseFrame();
final PhaseContext<?> context = PluginPhase.Listener.GENERAL_LISTENER.createPhaseContext()
Copy link
Contributor

Choose a reason for hiding this comment

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

extra leading space?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the proper indentation since it's a multiple try with resources.

…sed as necessary.

Signed-off-by: Gabriel Harris-Rouquette <[email protected]>
Signed-off-by: Gabriel Harris-Rouquette <[email protected]>
@gabizou gabizou merged commit 3682d25 into stable-7 Jun 13, 2018
@gabizou gabizou deleted the cleanup/compile-warnings branch June 13, 2018 17:00
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