Re: [GSOC][PATCH] userdiff: add support for Scheme

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Atharva

On 28/03/2021 13:23, Atharva Raykar wrote:
> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
 > [...]
>>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>>> new file mode 100644
>>> index 0000000000..90e75dcce8
>>> --- /dev/null
>>> +++ b/t/t4018/scheme-local-define
>>> @@ -0,0 +1,4 @@
>>> +(define (higher-order)
>>> +  (define local-function RIGHT
>>
>> ... this one, which is also indented and *is* marked as RIGHT.
> 
> In this test case, I was explicitly testing for an indented '(define'
> whereas in the former, I was testing for the top-level '(define-syntax',
> which happened to have an internal define (which will inevitably show up
> in a lot of scheme code).

It would be nice to include indented define forms but including them 
means that any change to the body of a function is attributed to the 
last internal definition rather than the actual function. For example

(define (f arg)
   (define (g x)
     (+ 1 x))

   (some-func ...)
   ;;any change here will have '(define (g x)' in the hunk header, not 
'(define (f arg)'

I don't think this can be avoided as we rely on regexs rather than 
parsing the source so it is probably best to only match toplevel defines.

Best Wishes

Phillip

> 
>>> +    (lambda (x)
>>> +     (car "this is" "ChangeMe"))))
>>> \ No newline at end of file
>>
>>> diff --git a/userdiff.c b/userdiff.c
>>> index 3f81a2261c..c51a8c98ba 100644
>>> --- a/userdiff.c
>>> +++ b/userdiff.c
>>> @@ -191,6 +191,14 @@ PATTERNS("rust",
>>> 	 "[a-zA-Z_][a-zA-Z0-9_]*"
>>> 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>>> 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>>> +PATTERNS("scheme",
>>> +         "^[\t ]*(\\(define-?.*)$",
>>
>> This "optional hyphen followed by anything" in the regex is strange.
>> Wouldn't that also capture a line that looks like, e.g.,
>>
>>     (defined-foo bar)
>>
>> Perhaps we want "define[- \t].*" in the regex?
> 
> Yes, this is what I intended to do, thanks for correcting it.
>