| Message ID | 20191213173902.71541-1-emaste@FreeBSD.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | userdiff: remove empty subexpression from elixir regex | expand |
On Fri, 13 Dec 2019 at 12:45, Jeff King <peff@peff.net> wrote: > > And that is the right thing, since these strings are the funcname and > word_regex patterns, respectively. > > So I think this is the correct fix. Many of the other regexes in this > list use "/* -- */" to seperate the two for readability. Maybe worth > doing here, too? Yeah, this elixir set seems to be the only one with comments on the individual subexpressions in the second set but the extra /* -- */ does make it a bit more clear. Patch v2 sent.
On Fri, Dec 13, 2019 at 05:39:02PM +0000, Ed Maste wrote: > diff --git a/userdiff.c b/userdiff.c > index 324916f20f..165d7e8653 100644 > --- a/userdiff.c > +++ b/userdiff.c > @@ -35,7 +35,7 @@ PATTERNS("dts", > PATTERNS("elixir", > "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$", > /* Atoms, names, and module attributes */ > - "|[@:]?[a-zA-Z0-9@_?!]+" > + "[@:]?[a-zA-Z0-9@_?!]+" > /* Numbers with specific base */ > "|[-+]?0[xob][0-9a-fA-F]+" > /* Numbers */ It took me a minute to see why this was different than the similar "Numbers" line below. The issue is the comma at the end of the previous line; this is starting a new string, whereas the "Numbers" line is pasting to the existing string. And that is the right thing, since these strings are the funcname and word_regex patterns, respectively. So I think this is the correct fix. Many of the other regexes in this list use "/* -- */" to seperate the two for readability. Maybe worth doing here, too? -Peff
Nothing to do with the patch from Ed, but the regex following his correction matches a lot of things that decidedly are not "Numbers with specific bases" as it claims to do in the comment. Ed Maste writes: > PATTERNS("elixir", > "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$", > /* Atoms, names, and module attributes */ > - "|[@:]?[a-zA-Z0-9@_?!]+" > + "[@:]?[a-zA-Z0-9@_?!]+" > /* Numbers with specific base */ > "|[-+]?0[xob][0-9a-fA-F]+" Here, things like "+0bad" would match as a base 2 number, which doesn't seem right. If it's intended to match that broadly, I'd have expected a comment to that effect. Maybe something like "|[-+]?0b[01]+|[-+]?0o[0-7]+|[-+]?0x[0-9a-fA-F]+" or (if the resulting group is not a problem someplace else) "|[-+]?0(b[01]+|o[0-7]+|x[0-9a-fA-F]+)" to more specifically match only what the comment says? Regards, Achim.
Achim Gratz <Stromeko@nexgo.de> writes: > Nothing to do with the patch from Ed, but the regex following his > correction matches a lot of things that decidedly are not "Numbers with > specific bases" as it claims to do in the comment. > > Ed Maste writes: >> PATTERNS("elixir", >> "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$", >> /* Atoms, names, and module attributes */ >> - "|[@:]?[a-zA-Z0-9@_?!]+" >> + "[@:]?[a-zA-Z0-9@_?!]+" >> /* Numbers with specific base */ >> "|[-+]?0[xob][0-9a-fA-F]+" > > Here, things like "+0bad" would match as a base 2 number, which doesn't > seem right. If it's intended to match that broadly, I'd have expected a > comment to that effect. No need for such a comment, as it is implicit that we assume the user writes reasonable text that our patterns try to match.
diff --git a/userdiff.c b/userdiff.c index 324916f20f..165d7e8653 100644 --- a/userdiff.c +++ b/userdiff.c @@ -35,7 +35,7 @@ PATTERNS("dts", PATTERNS("elixir", "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$", /* Atoms, names, and module attributes */ - "|[@:]?[a-zA-Z0-9@_?!]+" + "[@:]?[a-zA-Z0-9@_?!]+" /* Numbers with specific base */ "|[-+]?0[xob][0-9a-fA-F]+" /* Numbers */
The regex failed to compile on FreeBSD. Fixes: a807200f67588f6e Signed-off-by: Ed Maste <emaste@FreeBSD.org> --- userdiff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)