Re: [PATCH v2 0/3] userdiff: Java updates

From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Feb 5, 2023, 19:27 UTC
Message-ID: <6ca6ebf0-b357-e1d0-4866-dd04a5f987ad@gmail.com>
In-Reply-To: <45830cf4-41c1-0bc1-3e4e-26b9f713f452@kdbg.org>

On 2023-02-05T11:09, Johannes Sixt wrote:

Show 19 quoted lines

> Am 04.02.23 um 14:43 schrieb Andrei Rybak:
>> On 04/02/2023 10:22, Tassilo Horn wrote:
>>> Thanks for including me being the last contributor to java userdiff.
>>> The patches look good from my POV and are safe-guarded with tests, so
>>> I'm all for it.
>>
>> Thank you for review!
>>
>> I've realized that I've been writing modifiers "abstract" and "sealed" in a
>> technically correct, but not the conventional order.  Here's a reroll with the
>> order of modifiers following the style of original authors of
>> https://openjdk.org/jeps/409.  It doesn't matter for the purposes of the test,
>> but it will be less annoying to any future readers :-)
> 
> I've looked through the patches and run the tests, and they all make
> sense to me. By just looking at the patch text I noted that no
> whitespace between the identifier and the opening angle bracket is
> permitted and whether it should be allowed, but the commit messages make
> quite clear that whitespace is not allowed in this position.

There is some kind of misunderstanding. I guess the wording in commit messages of the first and second patches could have been clearer.

In Java, whitespace is allowed between type name and the brackets. It is permitted both for angle brackets of type parameters:

	class SpacesBeforeTypeParameters         <A, B> {
	}

and for round brackets of components in records:

	record SpacesBeforeComponents      (String comp1, int comp2) {
	}

The common convention, is however, to omit the whitespace before the brackets.

The regular expression on branch master already allows for whitespace after the name of the type:

	"^[ \t]*(([a-z]+[ \t]+)*(class|enum|interface)[ \t]+[A-Za-z][A-Za-z0-9_$]*[ \t]+.*)$\n"
	                                                                          ^^^^^^
so I didn't need to cover this case.  Note that it requires a non-zero
amount of whitespace. This part of the regular expression was left as
is (v2 after patch 3/3):

	"^[ \t]*(([a-z-]+[ \t]+)*(class|enum|interface|record)[ \t]+[A-Za-z][A-Za-z0-9_$]*([ \t]+|[<(]).*)$\n"
	                                                                                   ^^^^^^

That being said, I guess it would be an improvement to also allow the name of the type be followed by the end of the line, for users with fairly common code style that puts braces on separate lines:

	class WithLineBreakBeforeOpeningBrace
	{
	}

or `extends` and `implements` clauses after a line break:

	class ExtendsOnSeparateLine
		extends Number
		implements Serializable
	{
	}

even type parameters:

	class TypeParametersOnSeparateLine
		<A, B>
	{
	}

Something like the following:

	"^[ \t]*(([a-z-]+[ \t]+)*(class|enum|interface|record)[ \t]+[A-Za-z][A-Za-z0-9_$]*(([ \t]+|[<(]).*)?)$\n"
	                                                                                  ^               ^^
perhaps? Technically, the following is also valid Java:

	class WithComment//comment immediately after class name
	{
	}

but I'm not sure if allowing it is needed.  If so, we might as well just do this:

	"^[ \t]*(([a-z-]+[ \t]+)*(class|enum|interface|record)[ \t]+[A-Za-z][A-Za-z0-9_$]*.*)$\n"
	                                                                                  ^^