Create script to apply style to diffs #11261

ret2libc · 2018-08-26T15:18:12Z

I copied the clang-format-diff.py script from the clang project, as it provides a way to apply clang-format to diffs only. The license is University of Illinois Open Source. Is that ok?

Moreover, this patch restores the tab size to 8 char.

ret2libc · 2018-08-26T15:18:33Z

Fixes #11170

Maijin · 2018-08-26T15:20:53Z

So what about existing one here:

https://github.com/radare/radare2/blob/master/doc/clang-format

ret2libc · 2018-08-26T15:23:03Z

@Maijin maybe I should remove that? I just copied it directly in .clang-format, since at least it will be parsed by editors plugins.

Maijin · 2018-08-26T15:24:08Z

Needs to have only one yes.

ret2libc · 2018-08-26T15:26:21Z

removed

Maijin · 2018-08-26T15:30:21Z

Don't forget to update scripts like indent.sh that uses this clang-format

ret2libc · 2018-08-26T15:33:16Z

Thanks, done.

ret2libc · 2018-08-26T17:11:07Z

I slightly modified clang-format-diff.py to handle function declaration/definitions which do not require the space before the parenthesis.

ret2libc · 2018-08-26T18:58:44Z

I removed the indent-diff.sh script and just handled everything in the clang-format-diff.py script

ret2libc · 2018-08-27T08:29:22Z

Even though @radare said clang-format doesn't work, after testing some files I think it does a pretty good job. There are some things that are not well indented (like break in a switch case), but the good thing about this script is that it works at the "patch level" so we can more easily manually check those cases. Most of the stuff will be done automatically (e.g. indentation, spaces around functions, operators, etc.)

I believe as @radare said there are cases that clang-format does not handle well, but I do think we can either adapt to what the tools expect (instead of having our own particular coding style) or just consider them from case to case, manually.

ret2libc · 2018-08-28T11:58:34Z

doc/indent-example.c

+	"Usage:", "aa[0*?]", " # see also 'af' and 'afna'",
+	"aa", " ", "alias for 'af@@ sym.*;af@entry0;afva'", //;.afna @@ fcn.*'",
+	"aa*", "", "analyze all flags starting with sym. (af @@ sym.*)",
+	NULL};


this thing is different from what it is now, but... do we really care? I think we can adapt to this style as long as the automatic tool is able to correctly indent this.

the } should be in the next line

} shuld be in the next line

As said, is this really important? What I want is one way to automatically indent/fix style in my code. I don't care whether there's a space before/after a given word, as long as there's a tool that automagically does that.

Is there such a tool?
If yes, perfect, please show me.
If not, I think it's time we change a bit our coding style to adapt to those existing tools.

well it looks bad to me, and i dont know if there's any tool to do this unless my sys/indent.sh was doing it, which i didnt checked. i dont think we should change our coding style because we need to make code beautiful to read and not adjust to the bugs of the indentation tools. again, i didnt had time to test this pr yet, just speaking out loud

The } is probably attached because there is no , after NULL. Maybe we could programmatically insert colons at the end of lists like this to force the } to go to the next line.

On the other hand, I agree with retlibc that the attached-closing-curly-bracket-style is also OK, you would just need to get used to it.

ret2libc · 2018-08-28T11:59:32Z

doc/indent-example.c

+	r_anal_esil_set_pc (core->anal->esil, fcn ? fcn->addr : core->offset);
+	switch (*input) {
+	case 'a': // "afta"
+	{


I've seen so many different styles in r2 code base related to this... Again, who cares as long as we have an automatic tool?

ret2libc · 2018-08-28T12:02:12Z

doc/indent-example.c

+	r_anal_esil_set_pc (core->anal->esil, fcn ? fcn->addr : core->offset);
+	switch (*input) {
+	case 'a': // "afta"
+	{


I've seen so many different styles related to switch/cases in r2 codebase that I think we can adapt to this, since at least it is supported by automatic tools.

i think we should refactor this out and not allow such blocks in case statements. just move the code in a separate function or define variables outside will make it more readable. so im not sure if i want to define a syntax rule for those cases

I do agree, but still, there's so much code in r2codebase that has these code blocks in the switch-cases... And refacatoring is something the formatter can't do, we should ask for it during reviews.

coccinelle can do that

Can it extract functions from switch-case blocks?

Anyway, I don't think we do want such a behaviour. That's something that should be reviewed manually by us.

ret2libc · 2018-08-28T12:02:58Z

doc/indent-example.c

+		r_anal_esil_set_pc (core->anal->esil, fcn ? fcn->addr : core->offset);
+		r_core_anal_type_match (core, fcn);
+		r_core_seek (core, seek, true);
+	} break;


NOTE: this is the only case which I think deviates a lot from our current style... I think we can check this manually for the moment and just ask people to put the break inside the braces, so it will be correctly indented as part of the block.

ret2libc · 2018-08-28T12:04:52Z

@radare I've put an example with clang-format in doc/indent-example.c and added some comments on some different things that apply there. I'd like your comments on that. Also, by applying clang-format on entire files I obtain reasonable results.

nsajko · 2018-08-29T08:30:02Z

Have you reported how "break;" after a block in a switch statement is formatted to the Clang project? Seems like a bug.

But on the other hand, why not include the "break;" in the block in the first place? Seems like that would be a prettier style? The change is even easy to do programmatically if you decide to use clang-format for radare2 (at least for the cases where the "break;" precedes a case keyword).

ret2libc · 2018-08-29T08:34:21Z

@nsajko I din't report the "break;" thing to Clang yet. I will double-check and do it if necessary.
I also think that including the break; in the block is better anyway.

nsajko · 2018-08-29T08:39:58Z

I think you should add "IndentPPDirectives: AfterHash" to .clang_format.

ret2libc · 2018-08-29T08:49:08Z

@nsajko thanks for the help! have you seen indented #if in the current code? I'd like to introduce as less changes to the style as possible for the moment.

nsajko · 2018-08-29T09:12:21Z

I did a quick search in libr:

util/thread_sem.c util/sys.c util/regex/regexec.c util/regex/regcomp.c util/regex/regex2.h fs/fs.c include/sdb/types.h include/r_types.h debug/p/debug_native.c

etc.

ret2libc · 2018-08-29T09:15:52Z

yeah but as you can probably see most of the code doesn't do that. I guess those parts were copied from other code which used the indentation. Anyway, let's see what @radare thinks about it.

nsajko · 2018-08-29T09:17:24Z

OK

nsajko · 2018-08-29T20:04:31Z

The radare2 .clang-format should probably also have something like

ForEachMacros: ['r_list_foreach', 'ls_foreach', 'fcn_tree_foreach_intersect', 'r_skiplist_foreach', 'graph_foreach_anode']

With the other foreach macros, too.

Maijin · 2018-08-29T22:48:44Z

Maybe merge pre-commit-indent in the python script and probably indent.sh in it to have only one consolidated script instead of 3 scattered?

ret2libc · 2018-08-30T06:18:49Z

@Maijin I think I could merge indent.sh in it, but I'm not sure about pre-commit-indent. I think I will keep it separate, it's just a script that calls the .py and prints an error messag.

@nsajko thanks! That's useful, if you find anything else please share!

radare · 2018-09-21T09:56:01Z

obviously this is something to be done by a human

…

On 21 Sep 2018, at 11:51, Riccardo Schirone ***@***.***> wrote: @ret2libc commented on this pull request. In doc/indent-example.c <#11261 (comment)>: > +static const char *help_msg_aa[] = { + "Usage:", "aa[0*?]", " # see also 'af' and 'afna'", + "aa", " ", "alias for 'af@@ ***@***.***;afva'", //;.afna @@ fcn.*'", + "aa*", "", "analyze all flags starting with sym. (af @@ sym.*)", + NULL}; + +static int cmpaddr(const void *_a, const void *_b) { + const RAnalFunction *a = _a, *b = _b; + return a->addr - b->addr; +} + +int main(int argc, char **argv) { + r_anal_esil_set_pc (core->anal->esil, fcn ? fcn->addr : core->offset); + switch (*input) { + case 'a': // "afta" + { Can it extract functions from switch-case blocks? Anyway, I don't think we do want such a behaviour. That's something that should be reviewed manually by us. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3-lkdDLmFdOQBndI1WgIyIRgS4o9Vjks5udLawgaJpZM4WM0iH>.

codecov-io · 2018-09-21T14:16:19Z

Codecov Report

Merging #11261 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11261   +/-   ##
=======================================
  Coverage   37.47%   37.47%           
=======================================
  Files         899      899           
  Lines      284166   284166           
=======================================
  Hits       106492   106492           
  Misses     177674   177674

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d04e888...0efaa0e. Read the comment docs.

nsajko · 2018-09-21T16:34:25Z

sys/clang-format-diff.py

@@ -0,0 +1,153 @@
+#!/usr/bin/env python


Could you be explicit about which version of python you are invoking? Eg. do #! /usr/bin/python2. This is needed for example on Archlinux.

EDIT: to clarify, /bin/python is usually a symbolic link to /bin/python2, but on Archlinux /bin/python instead links to /bin/python3. So using python as a command is not a good idea.

Actually, it would be great if you could port this to the latest Python version, it is probably not a big effort.

nsajko · 2018-09-21T16:59:39Z

sys/clang-format-diff.py

@@ -0,0 +1,153 @@
+#!/usr/bin/env python2


Don't know if you are interested in porting to Python 3, but I am pretty sure you would just need to change this "shebang" line, import functools, and change reduce to functools.reduce

already done

ret2libc · 2018-09-21T20:09:03Z

@radare ping I think this is now ready to be (at least) tested more.

radare · 2018-09-21T23:39:23Z

sys/pre-commit-indent

@@ -0,0 +1,34 @@
+#!/bin/sh


rename to sys/pre-commit-indent.sh

radare · 2018-09-21T23:40:37Z

sys/pre-commit-indent

+
+tmpfile="$(mktemp)"
+unstaged_diff="$(mktemp)"
+git diff > "${unstaged_diff}"


can be "", chk if mktemp is not failing and tmpfile and unsaged_diff are not. ""

radare · 2018-09-21T23:40:51Z

sys/pre-commit-indent

+tmpfile="$(mktemp)"
+unstaged_diff="$(mktemp)"
+git diff > "${unstaged_diff}"
+git checkout -- .


radare · 2018-09-21T23:41:01Z

sys/pre-commit-indent

+
+restore_exit() {
+	git apply < "${unstaged_diff}" 2>/dev/null
+	rm "${tmpfile}"


radare · 2018-09-21T23:41:07Z

sys/pre-commit-indent

+restore_exit() {
+	git apply < "${unstaged_diff}" 2>/dev/null
+	rm "${tmpfile}"
+	rm "${unstaged_diff}"


radare · 2018-09-21T23:41:42Z

sys/pre-commit-indent

+#
+# To enable this hook, move it to ".git/hooks/pre-commit".
+
+tmpfile="$(mktemp)"


uppercase variable names?

radare

lgtm

ret2libc · 2018-09-23T15:15:48Z

I think I've done everything requested.

ret2libc requested review from radare and Maijin August 26, 2018 15:18

ret2libc changed the title ~~Create script to apply style to diffs~~ WIP: Create script to apply style to diffs Aug 26, 2018

ret2libc changed the title ~~WIP: Create script to apply style to diffs~~ Create script to apply style to diffs Aug 26, 2018

ret2libc changed the title ~~Create script to apply style to diffs~~ WIP: Create script to apply style to diffs Aug 26, 2018

ret2libc changed the title ~~WIP: Create script to apply style to diffs~~ Create script to apply style to diffs Aug 26, 2018

ret2libc requested a review from lowlyw August 27, 2018 12:25

ret2libc mentioned this pull request Aug 27, 2018

Provide/Document script to fix style in patches #11170

Closed

ret2libc commented Aug 28, 2018

View reviewed changes

ret2libc added 2 commits September 21, 2018 11:37

some fixes from PR comments

6980e34

Merge remote-tracking branch 'origin/master' into indent-diff

ee2adc8

ret2libc added 2 commits September 21, 2018 11:59

fix sys/pre-commit-indent and add , in list of strings

d05428b

sys/clang-format-diff.py: remove temp file at the end

1901a7b

nsajko reviewed Sep 21, 2018

View reviewed changes

ret2libc added 3 commits September 21, 2018 18:54

sys/clang-format-diff: specify python2

ff4448c

clang-format-diff: use python3 and make it python2/3 compatible

42e9fb5

clang-format-diff: wrap filename in "

f0d34f7

nsajko reviewed Sep 21, 2018

View reviewed changes

clang-format-diff: fix f.write for python3

72759d2

ret2libc changed the title ~~WIP: Create script to apply style to diffs~~ Create script to apply style to diffs Sep 21, 2018

radare reviewed Sep 21, 2018

View reviewed changes

sys/pre-commit-indent Outdated

@@ -0,0 +1,34 @@

#!/bin/sh

Copy link

Collaborator

radare Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to sys/pre-commit-indent.sh

radare reviewed Sep 21, 2018

View reviewed changes

sys/pre-commit-indent Outdated

restore_exit() {

git apply < "${unstaged_diff}" 2>/dev/null

rm "${tmpfile}"

Copy link

Collaborator

radare Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add -f

radare reviewed Sep 21, 2018

View reviewed changes

ret2libc added 2 commits September 23, 2018 17:10

rename sys/pre-commit-indent file

eda81ab

sys/pre-commit-indent: clean the script

28ea2fb

ret2libc added 4 commits September 25, 2018 12:05

Merge remote-tracking branch 'origin/master' into indent-diff

c138854

fix pre-commit-indent name

b87d47d

pre-commit-indent: use -s instead of computing the size with wc|cut

0efaa0e

Merge remote-tracking branch 'origin/master' into indent-diff

939d3cf

ret2libc merged commit 58ae116 into radareorg:master Sep 26, 2018

Refactorings automation moved this from IN PROGRESS to DONE Sep 26, 2018

Create script to apply style to diffs #11261

Create script to apply style to diffs #11261

Conversation

ret2libc commented Aug 26, 2018

ret2libc commented Aug 26, 2018

Maijin commented Aug 26, 2018

ret2libc commented Aug 26, 2018

Maijin commented Aug 26, 2018

ret2libc commented Aug 26, 2018

Maijin commented Aug 26, 2018

ret2libc commented Aug 26, 2018

ret2libc commented Aug 26, 2018

ret2libc commented Aug 26, 2018

ret2libc commented Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ret2libc commented Aug 28, 2018

nsajko commented Aug 29, 2018 • edited

ret2libc commented Aug 29, 2018

nsajko commented Aug 29, 2018

ret2libc commented Aug 29, 2018

nsajko commented Aug 29, 2018

ret2libc commented Aug 29, 2018

nsajko commented Aug 29, 2018

nsajko commented Aug 29, 2018 • edited

Maijin commented Aug 29, 2018

ret2libc commented Aug 30, 2018

radare commented Sep 21, 2018 via email

codecov-io commented Sep 21, 2018 • edited

Codecov Report

nsajko Sep 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ret2libc commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radare left a comment

Choose a reason for hiding this comment

ret2libc commented Sep 23, 2018

ret2libc commented Aug 27, 2018 •

edited

nsajko commented Aug 29, 2018 •

edited

nsajko commented Aug 29, 2018 •

edited

codecov-io commented Sep 21, 2018 •

edited

nsajko Sep 21, 2018 •

edited