From: Marius Ungureanu <hidden> Date: 2016-06-15 23:00:50
New keywords: foreach, break, in, try, finally, as, is, typeof, var,
default, fixed, checked, unchecked, this, lock, readonly, unsafe,
ref, out, base, null, delegate, continue.
Removed keywords: instanceof. It's only in Java.
Moved keywords to happen before modifier parsing, as matching a keyword
will stop modifiers from being matched.
Added method modifiers: extern, abstract.
Added properties modifiers: abstract.
Added parsing of events and delegates, which are like properties, but
take an extra keyword.
The reasoning behind adding unsafe to keywords is being also a
statement that can happen inline in code to mention blocks which are
unsafe. Also, delegates are not necessarily declared in class bodies,
but can also happen inline in other functions.
Keywords are based on MSDN docs.
Signed-off-by: Marius Ungureanu <redacted>
---
userdiff.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 23:00:50
Am 26.04.2014 01:25, schrieb Marius Ungureanu:
New keywords: foreach, break, in, try, finally, as, is, typeof, var,
default, fixed, checked, unchecked, this, lock, readonly, unsafe,
ref, out, base, null, delegate, continue.
Removed keywords: instanceof. It's only in Java.
Moved keywords to happen before modifier parsing, as matching a keyword
will stop modifiers from being matched.
Added method modifiers: extern, abstract.
Added properties modifiers: abstract.
Added parsing of events and delegates, which are like properties, but
take an extra keyword.
The reasoning behind adding unsafe to keywords is being also a
statement that can happen inline in code to mention blocks which are
unsafe. Also, delegates are not necessarily declared in class bodies,
but can also happen inline in other functions.
Keywords are based on MSDN docs.
Signed-off-by: Marius Ungureanu <redacted>
Thanks for your contribution.
Please write the commit message in imperative mood, and use full
sentences, not just fragments and avoid contractions ("it's"). Also,
don't capitalize the subject line and drop the full-stop:
update C# userdiff patterns
Add new keywords: foreach, break, ...
Remove keyword instanceof because it is only in Java. ...
BTW, it is now dead easy to add test cases for userdiff patterns. Just
drop files with content like this into t/t4018:
---- t/t4018/csharp-ignore-statement-keywords -----
class Foo {
public int RIGHT()
{
if (x)
else
try
catch (y)
...
ChangeMe;
}
}
-----------------------------------------
(This I just invented, I don't do C#.) See the README file in that
directory.
Here, you are moving keywords down, but in the commit message you say
that you "moved keywords to happen before modifier parsing". Aren't you
moving keywords *after* something? (Where the "modifiers" are here is
not obvious, but that can be attributed to that I don't do C#.)
BTW, I appreciate that you re-arrange keywords alphabetically. Could you
do that in the commit message, too?
-- Hannes
From: Marius Ungureanu <hidden> Date: 2016-06-15 23:00:50
On 26 Apr 2014, at 10:10, Johannes Sixt [off-list ref] wrote:
Am 26.04.2014 01:25, schrieb Marius Ungureanu:
quoted
New keywords: foreach, break, in, try, finally, as, is, typeof, var,
default, fixed, checked, unchecked, this, lock, readonly, unsafe,
ref, out, base, null, delegate, continue.
Removed keywords: instanceof. It's only in Java.
Moved keywords to happen before modifier parsing, as matching a keyword
will stop modifiers from being matched.
Added method modifiers: extern, abstract.
Added properties modifiers: abstract.
Added parsing of events and delegates, which are like properties, but
take an extra keyword.
The reasoning behind adding unsafe to keywords is being also a
statement that can happen inline in code to mention blocks which are
unsafe. Also, delegates are not necessarily declared in class bodies,
but can also happen inline in other functions.
Keywords are based on MSDN docs.
Signed-off-by: Marius Ungureanu <redacted>
Thanks for your contribution.
Please write the commit message in imperative mood, and use full
sentences, not just fragments and avoid contractions ("it's"). Also,
don't capitalize the subject line and drop the full-stop:
update C# userdiff patterns
Add new keywords: foreach, break, ...
Remove keyword instanceof because it is only in Java. …
Hey!
I’ll fix the commit message and description.
BTW, it is now dead easy to add test cases for userdiff patterns. Just
drop files with content like this into t/t4018:
---- t/t4018/csharp-ignore-statement-keywords -----
class Foo {
public int RIGHT()
{
if (x)
else
try
catch (y)
...
ChangeMe;
}
}
————————————————————
Great, I’ll make another commit with adding unit tests. Thanks!
(This I just invented, I don't do C#.) See the README file in that
directory.
Here, you are moving keywords down, but in the commit message you say
that you "moved keywords to happen before modifier parsing". Aren't you
moving keywords *after* something? (Where the "modifiers" are here is
not obvious, but that can be attributed to that I don't do C#.)
It was a typo because I was sleepy. It was intentional to move them *after*.
Modifier parsing can contain keywords, so just to be sure, I moved the
keywords after modifier parsing, so it uses the keywords as a fallback.
If this is not what should happen, please tell.
Modifiers are prefixes to methods/properties. (the pipe separated lists)
BTW, I appreciate that you re-arrange keywords alphabetically. Could you
do that in the commit message, too?
— Hannes
From: Marius Ungureanu <hidden> Date: 2016-06-15 23:00:50
On a side note, I noticed some of the keywords I added shouldn’t be there.
I just realised that simple statements have no reason to be there, but only
block definitions. I’ll reduce the size of this patch on the keywords part.
Thanks,
Marius
...
Modifier parsing can contain keywords, so just to be sure, I moved the
keywords after modifier parsing, so it uses the keywords as a fallback.
If this is not what should happen, please tell.
For each line, patterns are are scanned in order, and the first match
determines the outcome: If it is a negative pattern (i.e., it begins
with an exclamation mark), the line is not a hunk header; if it is a
positive pattern, the line is a hunk header. If no pattern matches, the
line is not a hunk header, either; it is as if the list were terminated
by a negative catch-all pattern.
Due to these rules, negative patterns in the list are only necessary
when you want to make an exception to a positive pattern in the list,
and then the negative pattern must be listed before the positive pattern.
In the csharp case, I do not see a pattern of which the keyword pattern
would make an exception (neither the old version nor your new version).
Therefore, you could drop the keyword pattern entirely.
-- Hannes
...
Modifier parsing can contain keywords, so just to be sure, I moved the
keywords after modifier parsing, so it uses the keywords as a fallback.
If this is not what should happen, please tell.
For each line, patterns are are scanned in order, and the first match
determines the outcome: If it is a negative pattern (i.e., it begins
with an exclamation mark), the line is not a hunk header; if it is a
positive pattern, the line is a hunk header. If no pattern matches, the
line is not a hunk header, either; it is as if the list were terminated
by a negative catch-all pattern.
Due to these rules, negative patterns in the list are only necessary
when you want to make an exception to a positive pattern in the list,
and then the negative pattern must be listed before the positive pattern.
In the csharp case, I do not see a pattern of which the keyword pattern
would make an exception (neither the old version nor your new version).
Therefore, you could drop the keyword pattern entirely.
-- Hannes
Hey!
I’ll remove them and add as many unit tests I can. I’ve been side tracked
today and I couldn’t get to look at it. I’ll start a new thread with the new
patch as soon as I’m done with it.
Thanks for all the tips until now. You’ve been of great help.
Marius
From: Marius Ungureanu <hidden> Date: 2016-06-15 23:00:51
Heya, so I’ll add the patches in the next 2 emails.
I’ve changed a bit the main body of the methods/constructors regex.
Basically, I’ve made the first item after the modifiers optional. That’s
the return type and it’s not used in any case by operator overloads
or constructors/destructors.
I also added lots of symbols to the name of the function. Those are
the symbols of the operators that the language allows you to overload.
Thanks in advance,
Marius.
From: Johannes Sixt <hidden> Date: 2016-06-15 23:00:51
Am 27.04.2014 15:47, schrieb Marius Ungureanu:
---
Thanks. Please signed off your patch.
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.
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!
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.
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
"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.
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".
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.)
-- Hannes
From: Marius Ungureanu <hidden> Date: 2016-06-15 23:00:51
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.
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.
"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.
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.
quoted hunk
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.