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

On 27 Apr 2014, at 19:19, Johannes Sixt [off-list ref] wrote:

Am 27.04.2014 15:47, schrieb Marius Ungureanu:

quoted

---

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.

quoted

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

quoted

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.

quoted

@@ -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.

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

-- Hannes

Marius