@RequiredArgsConstructor varargs (...) option

[Francis Lalonde]

Hello,

Here's a suggestion for a smal improvement. I use the
@RequiredArgsConstructor where I can, but also have a number of
classes that have final Object[] members that can be initialized with
zero..n elements. I still prefer to define explicit constructors for
those classes so I can use varargs, which make for cleaner calling
code. It would be nice if @RequiredArgsConstructor had an option to
generate varargs constructors for those array fields.

thus this :

class ZugZug {
final Integer[] functors;

public ZugZug(Integer... f) {
functors = f;
}
}

could be turned into this

@RequiredArgsConstructor(useVarArgs=true)
class ZugZug {
final Integer[] functors;
}

Obviously, only one array field could be initialized this way, and it
should be the last defined in class (to respect param order that
requires varags to be last in signature).

Francis

[Francis Lalonde]

Following Roel's interest in my proposition, I've been toiling away on
Lombok's code. I have successfully changed the Javac handler to
generate varargs when the last parameter is an array, but the Eclipse
Handler is giving me a hard time. Here is my Javac (working, tested)
code :

lombok.javac.handlers.HandleConstructor.createConstructor(AccessLevel,
JavacNode, List<JavacNode>, boolean, JCTree)

...
Iterator<JavacNode> i = fields.iterator();
while (i.hasNext()) {
JavacNode fieldNode = i.next();

...
long flags = Flags.FINAL;
if (field.vartype.getTag() == JCTree.TYPEARRAY && !i.hasNext())
flags |= Flags.VARARGS;
JCVariableDecl param = maker.VarDef(maker.Modifiers(flags,
nonNulls.appendList(nullables)), field.name, field.vartype, null);
...

and here is what I have now in Eclipse, which seems like it could work
(but doesnt) :

lombok.javac.handlers.HandleConstructor.createConstructor(AccessLevel,
JavacNode, List<JavacNode>, boolean, JCTree)
...

Iterator<EclipseNode> i = fields.iterator();
while (i.hasNext()) {
EclipseNode fieldNode = i.next();

...
Argument parameter = new Argument(field.name, fieldPos,
copyType(field.type, source), Modifier.FINAL);

if (field.binding != null) {
if ((field.binding.tagBits & TagBits.IsArrayType) != 0 && !
i.hasNext()) {
parameter.type.bits |= ASTNode.IsVarArgs;
}
}

setGeneratedBy(parameter, source);
...

I've taken the "parameter.type.bits |= ASTNode.IsVarArgs" bit from
lombok's own PatchDelegate class, but I do not get the expected
effect. The generated constructor only takes a regular array and
introspecting it does not return isVarArgs() == true.

Any Eclipse AST masters out there to help me out?

Francis

[Reinier Zwitserloot]

Well, this is a great start. We've had a discussion on this feature and concluded that your plan:

IF Lombok is going to generate a constructor, AND the last parameter of this constructor would be an array, THEN use varargs instead.

Is not a good idea. Instead, we're going to force you to annotate this field with @Varargs or some such (and, in passing, this also means the setter will be generating in that fashion). We ran into these problems:

(A) Arrays are extremely awkward in modern java, due to their clashes with generics, and storing the result of a varargs call in an array field is extremely bad, because this causes invisible heap corruption; it's what the @SafeVarargs hubbub was all about. Basically you should NEVER do ANYTHING with a varargs array except ask for its size and read elements from it (usually by just foreaching through it). Storing it is specifically something you should NOT DO with a varargs. Nor should you attempt to clone one, again, generics being the primary reason why not. This leads to...

(B) The right way to go about this is to have a List or Collection and allow filling of this list/collection via varargs, but it would be exceedingly surprising if you have a List<T> as a field in your class and all of a sudden lombok decides to make a varargs method for you. Our current intention is that you can stick @Varargs on a List or Collection and that will work. We still have many open questions in this regard: Should this be a simple Arrays.asList call, resulting in a hybrid list which is neither fully mutable (you can't clear, grow, or shrink it) but not fully immutable either (.set does work). This is pretty silly but is the fastest. Should be wrap this in Collections.unmodifiableList, making a true immutable? Should we wrap this in a newly created arraylist making it fully mutable? Should this depend on whether or nor the field is final? (That's awkward, as final doesn't actually mean that the list is mutable or immutable, so we're really repurposing an existing keyword for a different concept and that's not usually a good idea). I'm leaning towards saying that this means you get an immutable list (After all, presumably the list is small; rarely do you pass an array with hundreds of thousands of objects to a varargs method), and then immutable lists make a little more sense.

(C) It is not always obvious what's going to be 'the last parameter', and it is bad that it's not immediately clear if your method will be varargs or not. With a @Varargs annotation that part will at least be clear.

[Marius Kruger]

On 30 May 2012 16:01, Reinier Zwitserloot <rein...@gmail.com> wrote:
...

> (B) The right way to go about this is to have a List or Collection and allow

> filling of this list/collection via varargs, .... Our current

> intention is that you can stick @Varargs on a List or Collection and that
> will work.

AWESOME AWESOME AWESOME
In my api's I often have this headache which causes boilerplate, this
will be so cool, can't wait :)

> We still have many open questions in this regard: Should this be
> a simple Arrays.asList call, resulting in a hybrid list which is neither
> fully mutable (you can't clear, grow, or shrink it) but not fully immutable
> either (.set does work). This is pretty silly but is the fastest. Should be
> wrap this in Collections.unmodifiableList, making a true immutable? Should
> we wrap this in a newly created arraylist making it fully mutable?

my first thought, make it immutable by default but have the option eg.
@Varargs(mutable=true)

--
✝ Marius

[Fabrizio Giudici]

On Thu, 31 May 2012 00:59:56 +0200, Marius Kruger <ama...@gmail.com> wrote:


>> We still have many open questions in this regard: Should this be
>> a simple Arrays.asList call, resulting in a hybrid list which is neither
>> fully mutable (you can't clear, grow, or shrink it) but not fully
>> immutable
>> either (.set does work). This is pretty silly but is the fastest.
>> Should be
>> wrap this in Collections.unmodifiableList, making a true immutable?
>> Should
>> we wrap this in a newly created arraylist making it fully mutable?
>
> my first thought, make it immutable by default but have the option eg.
> @Varargs(mutable=true)

+1

--
Fabrizio Giudici - Java Architect, Project Manager
Tidalwave s.a.s. - "We make Java work. Everywhere."
fabrizio...@tidalwave.it
http://tidalwave.it - http://fabriziogiudici.it

[Reinier Zwitserloot]

Works for me as a first implementation. This work will enter .experimental first, which gives us the excuse to take it out if we later feel like it's a mistake to ship this as a 'we pledge to support this unless the reasons are really REALLY overwhelming not to'. Normally we err on the side of caution.

So, plan is: @Varargs, which has 1 param named 'mutable' of type boolean, default false.

[foal]

And? As I can see it was not be implemented for some reason. Why?

Best,

Stas