apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.58k stars 1.01k forks source link

Remove unneeded default value assignment [LUCENE-10074] #11112

Open asfimport opened 3 years ago

asfimport commented 3 years ago

This is a spin-off issue from discussion here https://github.com/apache/lucene/pull/128#discussion_r695669643, where we would like to see if there's any automatic checking mechanism (ecj ?) that can be enabled to detect and warn about unneeded default value assignments in future changes, as well as in the existing code.


Migrated from LUCENE-10074 by Zach Chen (@zacharymorn), updated Aug 28 2021

asfimport commented 3 years ago

Michael McCandless (@mikemccand) (migrated from JIRA)

This is maybe controversial, but it bugs me when I see declarations initializing a variable to their already default value, e.g. a class member int x = 0; or boolean foobarFlag = false.  Maybe ecj already has a check to detect such unnecessary default value initializations?

asfimport commented 3 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I looked thru the checks, don't think such a check exists.

IMO this is more style than a "problem". It is similar to adding extra unnecessary parentheses. Really if you initialize an integer to zero, it isn't doing any harm.

asfimport commented 3 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

I actually had to check whether it's not optimized away by javac... and it seems it's not. :)

> javap -c Test2.class
Compiled from "Test2.java"
public class Test2 {
  int field;

  public Test2();
    Code:
       0: aload_0
       1: invokespecial `#1`                  // Method java/lang/Object."<init>":()V
       4: return
}

>javap -c Test1.class
Compiled from "Test1.java"
public class Test1 {
  int field;

  public Test1();
    Code:
       0: aload_0
       1: invokespecial `#1`                  // Method java/lang/Object."<init>":()V
       4: aload_0
       5: iconst_0
       6: putfield      `#7`                  // Field field:I
       9: return
}
asfimport commented 3 years ago

Robert Muir (@rmuir) (migrated from JIRA)

Well that's just what javac does. if it is really important: say in a tight loop, then my first question would be: why are we creating zillions of objects? Then my second question would be: does c1/c2 take care?

asfimport commented 3 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

I agree with Robert. In general, I prefer final variables (and fields), in which case the initialization should be done differently. This mostly affects if/then/else chains, where initialization to a default before the chain is bad code practice, because is hard to follow when or if at all a value is assigned.

asfimport commented 3 years ago

Dawid Weiss (@dweiss) (migrated from JIRA)

Oh, I don't think it matters that much at runtime. I just wanted to check if javac would optimize this away (similar to how it treats string constants concatenation, for example). I understand javac-developers though: it doesn't make much sense to do such optimizations at javac-side given how much complex code reshuffling is going on at runtime (c2 in particular).