Fix this-escape warnings

[Larhzu]

There are a few this-escape warnings. For example, in DeltaOptions:

src/org/tukaani/xz/DeltaOptions.java:46: warning: [this-escape]
possible 'this' escape before subclass is fully initialized
    [javac]         setDistance(distance);
    [javac]                    ^

I fixed one this-escape warning by making a the internal class IndexDecoder final. I wonder what is the best way to fix the public classes: XZOutputStream, DeltaOptions, and LZMA2Options.

The fix to XZOutputStream in this PR feels fairly good already in sense that I don't really see any alternatives.

The filter options classes could be fixed in a few ways:

1. Make them final like currently in this PR. This is simple to do but will break things if someone has extended the options classes at all.

2. Make many (all?) public method in the options classes final. This will break less as the classes can still be extended if the functions aren't overridden. But, for example, someone could in theory have created a custom override that prevents setting a too high LZMA2 dictionary size. It sounds unlikely though.

3. Add private methods that can be called from the constructors and public methods. That is, make the public methods trivial wrappers for the private methods. This should break backward compatibility even less. It's still not perfect if there was a subclass with a constructor that relied on the current (somewhat broken) behavior.

   A downside is that this makes the code uglier. I would like to avoid this solution. It is OK in XZOutputStream but LZMA2Options would need many such functions.

4. Add static functions to do the argument verification, for example, void validateDistance(int distance). Use these in the constructors and public methods. The actual assignments of the validated values would be duplicated (done in both constructors and in the public setter methods).

   This makes the code even uglier than in (3).

I suppose any fix is at least a slight API/ABI breakage. I think even with (1) very few users of the package would be affected though.

@bokken: I would like to solve this fairly quickly (a few days) to get the 1.10 release out.

[Larhzu]

I pushed all four variants of the filter options change ideas. The version 3 is the latest one (without a number).

4 is indeed too ugly. I suppose it's about choosing between 1 and 3, assuming that this can be changed at all.

The benefit of 1 is that overriding can be done so that it messes up internals of the package. For example, if LZMA2Options.getPb returned 5 it would certainly break the encoder. Obvious there should be no reason to do stupid things like that, and preventing them by making classes final is silly as it can prevent useful subclasses too.

Once this is decided, I think 1.10 is ready. There is a draft of NEWS.md in the news_draft branch.

[bokken]

I think I lean towards making the classes final. Extending these classes and overriding methods allows for too many bad/non-sensical things to happen. Which is the same thing you point out. And I struggle to find a reasonable/useful thing to do with subclasses.

[JNightRider]

Hi @Larhzu

Just to help a little with the warnings (I hope I don't upset anyone)...

The warning on line 46 is something that the Java compiler frequently throws when an object calls a public method in its constructor, indicating that this method may be overridden in some inherited subclade and that the behavior may change.

There are several ways to solve this warning, although setting the class as 'final' solves it, you can also do the following trick:

public class DeltaOptions extends FilterOptions {
    ...
    public DeltaOptions(int distance) throws UnsupportedOptionsException {
        DeltaOptions.this.setDistance(distance);
    }
    ...
}

In this way we avoid creating unnecessary methods to encapsulate other public methods, which will only be called in the constructor.

The key is in this line of code:

Object.this.method(args)

[Larhzu]

@bokken:

I think I lean towards making the classes final. Extending these classes and overriding methods allows for too many bad/non-sensical things to happen. Which is the same thing you point out.

I cannot find code on the web that extends the options classes. So the likelyhood of breakage is pretty low if they are made final.

And I struggle to find a reasonable/useful thing to do with subclasses.

The main thing I can imagine is overriding setDictSize or setPreset to limit the settings to some value lower than the default maximums. Perhaps there could be some debugging use cases if getOutputStream or getInputStream can be overridden but that's getting very specific and then one perhaps can compile a custom version of the code too.

I tried to check what other situtations there are where public functions are called from other functions within the class:

• In output streams, close calls finish. This perhaps could be documented in the API docs.

• XZOutputStream.endBlock() is called in finish(). This too could be documented.

• SeekableXZInputStream.seekToBlock calls getBlockPos but there is no guarantee that it will do so in the future too. So I don't want to put that in the API docs. Perhaps such a call should be avoided.

I read that there is a recommendation that one should either design a class to be extensible or make it final. I get why but on the other hand sensible extensions are low risk, and preventing them feels a bit silly (it could even be really annoying in some situations).

[Larhzu]

@JNightRider:

DeltaOptions.this.setDistance(distance);

I tried it and it makes no difference. It doesn't fix the warning. If setDistance is overridden, the method in a subclass will be called. Sorry if I'm missing something.

[Larhzu]

@bokken:

I think I lean towards making the classes final.

I have done this. I hope the master branch is now about ready for 1.10. I updated the news_draft branch as well.

Earlier releases in Maven Central don't include debugging information. I noticed that at least some packages do though. I don't remember anyone requesting it but I wonder if it should be added. Building xz.jar with OpenJDK 22:

Size	Options
126 KiB	-g:none
147 KiB	-g:source,lines
165 KiB	-g or -g:source,lines,vars

Thoughts?

---

@JNightRider:

Just to help a little with the warnings (I hope I don't upset anyone)...

I forgot to say that it's great that more people comment and give feedback. Thanks!

[bokken]

Earlier releases in Maven Central don't include debugging information. I noticed that at least some packages do though. I don't remember anyone requesting it but I wonder if it should be added.

I have observed that including the debug info is more and more common (pretty sure this is because of the maven-compiler-plugin default behavior of -g).

[Larhzu]

Ah, so it hasn't always been so common. I suppose nowadays package sizes matter less and less. Having the debug info available without extra steps likely is convenient. I did enable debug info in 1.10 binaries in Maven Central.

While it's unclear if Ant will be used in this project a year from now, I played around a bit and got reproducible builds working. I used OpenJDK 21 for the binaries because that has long support, which makes it easier for people to verify the 1.10 binaries a year or two later.

I read that there's work going on in Maven plugins to make more builds reproducible. I feel it's a good goal. It seems that OpenJDK's compiler output doesn't often change outside of major releases, which helps a lot.