Re: [PATCH] userdiff: add support for Swift

Johannes Sixt <[email protected]> writes:
>>  - attributes, with or without arguments, whether on their own line
>>    ("@objc" above a "func") or inline with the declaration
> AFAIC, the regular expression does not match attributes on their own
> line. What relevance does this statement have?

You are right.  The pattern only matches attributes that are inline with
the declaration.  An attribute on its own line is not matched, and does
not need to be, because the declaration line below it matches on its own.
I have reworded this.

> This test contains "RIGHT" twice. This is not good, because we do not
> know which one is picked.
[...]
> Again "RIGHT" twice in a harmful way.

Fixed in a coming v2: swift-init, swift-failable-init and
swift-generic-subscript now contain "RIGHT" only once, on the
declaration line.

> It may be worth considering to enumerate all keywords and permit any
> run of them:
> 	(public|final|etc.|func|init|...|actor)[ \t(?!<]+)+

Noted, and thanks for the follow-up on this one.  I did check it anyway
out of curiosity: with that shape, a line that is only modifiers and
never reaches a real declaration keyword, such as

	public var counter = 0

would still match, because it merges modifiers and declaration keywords
into one interchangeable run.  The current pattern requires a real
keyword at the end, so that line correctly gets no header.  I will keep
the current form for now, and can revisit if the backtracking turns out
to matter in practice.

> You could just throw all of them into a single pattern like this:
> 	0[xXoObB][0-9a-fA-F_]+
> except when, for example, 0b1_abc

Right -- that is why I kept them as three separate patterns, so the
digit ranges stay correct (binary [01], octal [0-7]); merging would
mis-tokenize "0b1_abc".

> Is ".5" a correct floating-point number?

No -- Swift requires a leading digit, so ".5" is a syntax error (one must
write "0.5").  Tokenizing it as "." and "5" is therefore fine, and it
does not occur in valid Swift.

> You do not have to account for single-character operators; they are
> automatic. Drop the "?" from the first "=?".

Done in a coming v2, thanks; I had not realized PATTERNS appends
"|[^[:space:]]".  It is a nice simplification, and it only touches the
word regex, not the funcname pattern.

Since neither of us speaks Swift, for your ease of judgement I have also
put together some coverage numbers, which the coming v2 cover note will
include:

 - Grammar: I went through every declaration form listed in the "Summary
   of the Grammar" in Swift's own language reference (func, init incl.
   failable/generic, deinit, subscript incl. generic, class, struct,
   enum, protocol, extension, actor, operator methods, stacked
   modifiers, attributes with and without arguments, "where" clauses,
   multi-line signatures -- 26 forms total) and wrote a case for each.
   All 26 get the correct header.

 - Real-world code: I ran the driver over the last 200 commits touching
   *.swift in seven different Swift projects -- Alamofire,
   apple/swift-argument-parser, vapor, Kingfisher, RxSwift, SnapKit, and
   pointfreeco/swift-composable-architecture -- and checked every hunk
   header by hand. Out of 20454 hunks, 15310 got a header, and 15296 of
   those (99.9%) named a real declaration. None of the empty-header
   hunks turned out to be a real miss (they were things like file
   comment blocks, imports, or Package.swift, which have nothing to
   attach a header to).

These numbers are unaffected by the changes in this reply: the funcname
pattern is identical in v1 and v2 (only the word regex and the test
files changed), and both measurements are of hunk headers, which come
from the funcname pattern alone. So the coverage above still holds for
v2.

Besides the fixes above, v2 will also carry the reworded attribute
description and the changelog explaining what changed since v1, so the
full picture is in one place when you look at it.

Thanks for the careful review.
Shlok