On 27 Apr 2014, at 19:19, Johannes Sixt <j...@kdbg.org> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
