Re: [PATCH 2/2] add csharp userdiff tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 27 Apr 2014, at 19:19, Johannes Sixt <j6t@xxxxxxxx> wrote:

> Am 27.04.2014 15:47, schrieb Marius Ungureanu:
>> 
>> ---
> 
> Thanks. Please signed off your patch.
> 

Ah, yes, forgot to do that.

> When you re-send, please place [PATCH v3 n/m] in the subject (and drop
> the "Re:") and note what you changed compared to the previous (or all
> earlier) rounds. The place for this note is here, after the "---" marker.
> 
> Have a look at Documentation/SubmittingPatches.
> 

Will do so.

>> t/t4018/csharp-constructor | 4 ++++
>> t/t4018/csharp-destructor  | 4 ++++
>> t/t4018/csharp-function    | 4 ++++
>> t/t4018/csharp-member      | 4 ++++
>> t/t4018/csharp-namespace   | 4 ++++
>> t/t4018/csharp-operator    | 4 ++++
>> t/t4018/csharp-property    | 4 ++++
>> t/t4018/csharp-skip-call   | 5 +++++
>> 8 files changed, 33 insertions(+)
>> create mode 100644 t/t4018/csharp-constructor
>> create mode 100644 t/t4018/csharp-destructor
>> create mode 100644 t/t4018/csharp-function
>> create mode 100644 t/t4018/csharp-member
>> create mode 100644 t/t4018/csharp-namespace
>> create mode 100644 t/t4018/csharp-operator
>> create mode 100644 t/t4018/csharp-property
>> create mode 100644 t/t4018/csharp-skip-call
>> 
> 
> Unfortunately, I think you have reduced the test cases too far. One of
> the main properties of C# code is that usually member and property
> definitions are indented and there is a class definition around them:
> 
>  class Foo {
>     Foo(X) {}
>     virtual void DoStuff() {}
>     public int X;
>  };
> 
> In your examples, you omitted the surrounding class definition and did
> not indent the member definitions. By doing so, the test cases do not
> demonstrate that the csharp userdiff patterns are significantly
> different from the default userdiff pattern: in the examples you
> present, the default pattern would have picked the same hunk headers as
> the csharp patterns!
> 

Ah, I think I over judged minimal sample here. I’ll do so.

Is it okay though if I add a few tests to show what is broken?

I think this can’t be solved at a regex level.

> For a reviewer who is not (or only marginally) familiar with C# (like
> myself), it would have been very instructive to present patches with
> test cases that demonstrate weaknesses of the old patterns before
> patches that fix them. For example, you say that you fix the constructor
> pattern. But I am unable to judge what is wrong and how you fix it. The
> commit message is the right place to add text that helps reviewers.
> 

Well, the previous pattern didn’t match constructors as they should be
at a logical level. That means, it considered the constructor name
as being the return type. It’s just a logical change that helped with
writing operator function parsing.

> You can mark a userdiff test case that demonstrates a breakage by
> including the work "broken" somewhere in the file. See
> http://www.repo.or.cz/w/alt-git.git/commitdiff/9cc444f0570b196f1c51664ce2de1d8e1dee6046
> 
>> diff --git a/t/t4018/csharp-constructor b/t/t4018/csharp-constructor
>> new file mode 100644
>> index 0000000..a39cffb
>> --- /dev/null
>> +++ b/t/t4018/csharp-constructor
>> @@ -0,0 +1,4 @@
>> +MyClass(RIGHT)
>> +{
>> +	ChangeMe;
>> +}
>> diff --git a/t/t4018/csharp-destructor b/t/t4018/csharp-destructor
>> new file mode 100644
>> index 0000000..55106d9
>> --- /dev/null
>> +++ b/t/t4018/csharp-destructor
>> @@ -0,0 +1,4 @@
>> +~MyClass(RIGHT)
>> +{
>> +	ChangeMe;
>> +}
>> diff --git a/t/t4018/csharp-function b/t/t4018/csharp-function
>> new file mode 100644
>> index 0000000..a5d59a3
>> --- /dev/null
>> +++ b/t/t4018/csharp-function
>> @@ -0,0 +1,4 @@
>> +virtual DoStuff(RIGHT)
>> +{
>> +	ChangeMe;
>> +}
>> diff --git a/t/t4018/csharp-member b/t/t4018/csharp-member
>> new file mode 100644
>> index 0000000..4939d53
>> --- /dev/null
>> +++ b/t/t4018/csharp-member
>> @@ -0,0 +1,4 @@
>> +unsafe class RIGHT
>> +{
>> +	int ChangeMe;
>> +}
>> diff --git a/t/t4018/csharp-namespace b/t/t4018/csharp-namespace
>> new file mode 100644
>> index 0000000..6749980
>> --- /dev/null
>> +++ b/t/t4018/csharp-namespace
>> @@ -0,0 +1,4 @@
>> +namespace RIGHT
>> +{
>> +	ChangeMe;
>> +}
>> diff --git a/t/t4018/csharp-operator b/t/t4018/csharp-operator
>> new file mode 100644
>> index 0000000..5b00263
>> --- /dev/null
>> +++ b/t/t4018/csharp-operator
> 
> "csharp-user-defined-operator" would more precisely describe the case. I
> wouldn't mind seening other file names being a bit more elaborate, but I
> find this one particularly ambiguous.
> 

Okay, I’ll name them better. Also improve them.

>> @@ -0,0 +1,4 @@
>> +A operator+(RIGHT)
>> +{
>> +	ChangeMe;
>> +}
>> diff --git a/t/t4018/csharp-property b/t/t4018/csharp-property
>> new file mode 100644
>> index 0000000..82d67fc
>> --- /dev/null
>> +++ b/t/t4018/csharp-property
>> @@ -0,0 +1,4 @@
>> +public virtual int RIGHT
>> +{
>> +	get { ChangeMe; }
>> +}
>> diff --git a/t/t4018/csharp-skip-call b/t/t4018/csharp-skip-call
>> new file mode 100644
>> index 0000000..e89d083
>> --- /dev/null
>> +++ b/t/t4018/csharp-skip-call
>> @@ -0,0 +1,5 @@
>> +virtual void RIGHT()
>> +{
>> +	call();
>> +	ChangeMe;
>> +}
> 
> In this last test case, you want to demonstrate that the line "call()"
> is not picked as hunk header. As written, the line would never be picked
> as hunk header, even if it would match some pattern, because it is too
> close to "ChangeMe". You must have at least one more line between
> "call()" and "ChangeMe”.
> 

Oh, forgot about that.

> BTW, what is the expected hunk header in a diff like the following where
> "class Foo" is at line 1 in the file (just above the hunk)?
> 
> @@ -2,3 +2,3 @@
> {
> -   // old comment
> +   // new comment
>    public whatever()
> 
> That is, when the class definition is undecorated (no "unsafe" etc.)

The class name should still get matched in that case even if it isn’t decorated.

> 
> -- Hannes

Marius--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]