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

Zero-length array allocation inspection (idea) #504

Open
wants to merge 103 commits into
base: master-MC1.12
Choose a base branch
from

Conversation

RusTit
Copy link
Contributor

@RusTit RusTit commented Apr 5, 2018

Text:
Reports on allocations of arrays with known lengths of zero. Since array lengths in Java are non-modifiable, it is almost always possible to share zero-length arrays, rather than repeatedly allocating new zero-length arrays. Such sharing may provide useful optimizations in program runtime or footprint. Note that this inspection does not report zero-length arrays allocated as static final fields, as it is assumed that those arrays are being used to implement array sharing.
And also I removed two methods. They are the same as those of the parent class.

@RusTit RusTit mentioned this pull request Apr 6, 2018
@Nedelosk Nedelosk requested a review from mezz April 6, 2018 13:53
@RusTit
Copy link
Contributor Author

RusTit commented Apr 7, 2018

@Nedelosk please review.

@RusTit
Copy link
Contributor Author

RusTit commented Apr 12, 2018

No problem, but it can take some time.

@@ -40,8 +40,10 @@ public static ItemStack getItemStack(final Block block, final int damage) {
return itemStack;
}

private static final int MAX_ITEM_DAMAGE = 16387;
Copy link
Member

Choose a reason for hiding this comment

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

Any idea what the significance of this number is?
I thought it might be a power of 2 minus 1, but the nearest power of 2 is 2^14 (16384) which is also a weird number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, I was trying to find this value (binnie, forestry, forge) but no successed

Copy link
Contributor

@temp1011 temp1011 Apr 13, 2018

Choose a reason for hiding this comment

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

Max seems to be OreDictionary.WILDCARD_VALUE at 32767 so this doesn't even have the right number of bits.

@Nullable
private static Pattern PATTERN;

private static Pattern GetPattern() {
Copy link
Member

Choose a reason for hiding this comment

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

Method names should start with a lowercase letter.

Actually I think this should just be created statically instead of using a singleton getter, since it is not very expensive to create and does not pull in lots of other classes.

private static Pattern PATTERN = Pattern.compile("_", Pattern.LITERAL);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it is not. At first version I do like you describe. But after mc run, it crushed with NullPointException in

@Override
	public String toString() {
		return GetPattern().matcher(super.toString().toLowerCase(Locale.ENGLISH)).replaceAll(StringUtils.EMPTY);
}

Where is some issue with object creating (initial). At moment then toString invoked PATTERN will be null.

Copy link
Member

Choose a reason for hiding this comment

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

Hm enums are weird, I guess they are being constructed before static. This is fine then.

@@ -57,11 +57,13 @@ public String getText() {
@Override
@SideOnly(Side.CLIENT)
public void onRenderBackground(int guiWidth, int guiHeight) {
Object texture = CraftGUITexture.BUTTON_DISABLED;
Object texture;
Copy link
Member

Choose a reason for hiding this comment

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

One nice use of final is for cases like this. You can make final Object texture; and the static analyzer will complain "variable 'texture' might not have been initialized" if there is a case where it is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced the proposed changes, but it seems that the static analyzer worked correctly without them.

@@ -42,15 +42,15 @@ public void onValueChanged(final IClassification branch) {
}
this.branchDescription.clear();
String desc = branch.getDescription();
if (desc == null || Objects.equals(desc, "") || desc.contains("for.")) {
if (desc == null || desc.length() == 0 || desc.contains("for.")) {
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 use org.apache.commons.lang3.StringUtils#isEmpty to check desc

for (int row = 0; row < rowCount; ++row) {
for (int column = 0; column < columnCount; ++column) {
final ControlSlot slot = new ControlSlot.Builder(this, column * 18, row * 18)
.assign(InventoryType.PLAYER,column + row * columnCount);
Copy link
Member

Choose a reason for hiding this comment

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

this line is using spaces for indentation, the one above is tabs

@@ -193,11 +193,11 @@ public void getHelpTooltip(final Tooltip tooltip, ITooltipFlag tooltipFlag) {
if (slot != null) {
tooltip.add(slot.getName());
if (tooltipFlag.isAdvanced()) {
Collection<EnumFacing> inputSides = slot.getInputSides();
Collection<EnumFacing> inputSides = slot.getInputSides();
Copy link
Member

Choose a reason for hiding this comment

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

this line indents with spaces

if (inputSides.size() > 0) {
tooltip.add(TextFormatting.GRAY + I18N.localise(ModId.CORE, "gui.side.insert", MachineSide.asString(inputSides)));
}
Collection<EnumFacing> outputSides = slot.getOutputSides();
Collection<EnumFacing> outputSides = slot.getOutputSides();
Copy link
Member

Choose a reason for hiding this comment

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

this line indents with spaces

@@ -53,7 +52,7 @@ public ItemStack getStack(IItemMiscProvider type, int size) {
@SideOnly(Side.CLIENT)
public void addInformation(ItemStack stack, @Nullable World worldIn, List<String> tooltip, ITooltipFlag flagIn) {
super.addInformation(stack, worldIn, tooltip, flagIn);
IItemMiscProvider item = this.getItem(stack.getItemDamage());
IItemMiscProvider item = this.getItem(stack.getItemDamage());
Copy link
Member

Choose a reason for hiding this comment

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

this line indents with spaces

@@ -67,7 +66,7 @@ public void registerModel(Item item, IModelManager manager) {

@Override
public String getItemStackDisplayName(final ItemStack stack) {
IItemMiscProvider item = this.getItem(stack.getItemDamage());
IItemMiscProvider item = this.getItem(stack.getItemDamage());
Copy link
Member

Choose a reason for hiding this comment

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

this line indents with spaces

if ((!FluidRegistry.registerFluid(bFluid)) || (FluidRegistry.addBucketForFluid(bFluid))) {
Log.error("Liquid registered incorrectly - {} ", fluid.getIdentifier());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

this line indents with spaces

Copy link
Member

@mezz mezz left a comment

Choose a reason for hiding this comment

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

there are indentation issues in the latest commits

@mezz
Copy link
Member

mezz commented Apr 16, 2018

Please do not add more commits on to this PR, it needs to be reviewed and merged but that gets harder with more commits.
You can create a new PR for more changes.

@RusTit
Copy link
Contributor Author

RusTit commented Apr 16, 2018

Ok, but to make clear, where is still some bugs:

  1. Item icons painting on second layer. (Some description is above).
  2. Event key passing in ComponentCompartmentInventory.java and WindowCompartment.java (read my dialog with Nedelosk).

@RusTit
Copy link
Contributor Author

RusTit commented Apr 16, 2018

Item icon painting bug appears not only in the search window.
You can also see it in the slider (in which custom tab settings are set).

@mezz
Copy link
Member

mezz commented Apr 16, 2018

If you have specific fixes for the changes made in this PR, I think that is fine.
I am just worried that this PR is going out of control, it seems to be fixing too many different things.

@RusTit
Copy link
Contributor Author

RusTit commented Apr 16, 2018

Not yet. I do not know how to fix a bug with rendering. I will fix the error with the key a little later with separate commits (will be last in this PR)

@RusTit
Copy link
Contributor Author

RusTit commented Apr 16, 2018

Apparently there is no bug with keys.
I did debugging by checking the passed string-keys through the events, in everything is ok.
P.S. but the bug with the renderer I can not solve myself.

@KorDum
Copy link
Contributor

KorDum commented Apr 16, 2018

Tabs indent or spaces?

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.

5 participants