Re: [PATCH v4] userdiff: improve java hunk header regex
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Aug 11, 2021, 07:34 UTC
- Message-ID
- <95ebb2cf-2e6e-912e-7d80-3947a8e3d9e4@kdbg.org>
- In-Reply-To
- <87zgtoh6bm.fsf@gnu.org>
Am 11.08.21 um 07:22 schrieb Tassilo Horn:
Show 9 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes: >> These new tests are very much appreciated. You do not have to go wild >> with that many return type tests; IMO, the simple one and the most >> complicated one should do it. (And btw, s/cart/card/) > > Well, they appeared naturally as a result during development and made it > easier to spot errors when you know up to which level of complexity it > still worked. Is there a stronger reason to remove tests which might > not be needed, e.g., runtime cost on some CI machines?
I totally understand how the test cases evolved. Having many of them is not a big deal. It's just the disproportion of tests of this new feature vs. the existing tests that your patch creates, in particular, when earlier of the new tests are subsumed by later new tests.
Show 16 quoted lines
> Another thing I've noticed (with my suggested patch) is that I should
> not try to match constructor signatures. I think that's impossible
> because they are indistinguishable from method calls, e.g., in
>
> public class MyClass {
> MyClass(String RIGHT) {
> someMethodCall();
> someOtherMethod(17)
> .doThat();
> // Whatever
> // ChangeMe
> }
> }
>
> there is no regex way to prefer MyClass(String RIGHT) over
> someOtherMethod().Good find.
Show 18 quoted lines
> So all in all, I'd propose this version in the next patch version:
>
> --8<---------------cut here---------------start------------->8---
> PATTERNS("java",
> "!^[ \t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\n"
> "^[ \t]*("
> /* Class, enum, and interface declarations */
> "(([a-z]+[ \t]+)*(class|enum|interface)[ \t]+[A-Za-z][A-Za-z0-9_$]*[ \t]+.*)"
> /* Method definitions; note that constructor signatures are not */
> /* matched because they are indistinguishable from method calls. */
> "|(([A-Za-z_<>&][][?&<>.,A-Za-z_0-9]*[ \t]+)+[A-Za-z_][A-Za-z_0-9]*[ \t]*\\([^;]*)"
> ")$",
> /* -- */
> "[a-zA-Z_][a-zA-Z0-9_]*"
> "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
> "|[-+*/<>%&^|=!]="
> "|--|\\+\\+|<<=?|>>>?=?|&&|\\|\\|"),
> --8<---------------cut here---------------end--------------->8---That looks fine.
One suggestion, though. You do not have to have all positive patterns ("class, enum, interface" and "method definitions") in a single pattern separated by "|". You can place them on different "lines" (note the "\n" at the end of the first pattern):
/* Class, enum, and interface declarations */ "^[ \t]*(...(class|enum|interface)...)$\n" /* * Method definitions; note that constructor signatures are not * matched because they are indistinguishable from method calls. */ "^[ \t]*(...[A-Za-z_][A-Za-z_0-9]*[ \t]*\\([^;]*))$",
I don't think there is a technical difference, but I find this form easier to understand because fewer open parentheses have to be tracked.
-- Hannes