Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistently use spaces in C in Python tutorial #1418

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Consistently use spaces in C in Python tutorial #1418

merged 1 commit into from
Nov 8, 2017

Conversation

zoidyzoidzoid
Copy link
Contributor

Sorry, nitpick, but it was confusing me a bit while going through it.

Copy link
Collaborator

@yonghong-song yonghong-song left a comment

Choose a reason for hiding this comment

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

LGTM.

@yonghong-song
Copy link
Collaborator

[buildbot, ok to test]

@brendangregg
Copy link
Member

Much of the C that we are tracing originates from the Linux kernel, which uses tabs as indentation. Switching that C code to look like Python, with 4 character indents, I do not think looks right to a kernel engineer.

The bcc repository has examples of both: some use tabs in their kernel code, and some uses 4 character indents. I'm fine with having examples of both, to appeal to different audiences. If I had to pick one, I'd switch the C code to tabs to be consistent with the kernel code that it is tracing, even though the remainder of the tool will be Python with 4 character indents. But I don't want to insist on one convention for all authors.

I'd accept this change as it makes one file consistent -- the tutorial. But I would not like to see all the other tools in bcc changed. If a kernel engineer uses tabs in their C code, we should let them. If a python developer writes a bcc tool and wants 4 character indents everywhere, we should let them too.

Copy link
Member

@4ast 4ast left a comment

Choose a reason for hiding this comment

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

I agree with Brendan.
Makes sense to keep C code kernel indented.

@zoidyzoidzoid
Copy link
Contributor Author

Sounds good to me. That makes sense. Will just change it to be standardised as kernel style.

I think following the kernel's style guide then is probably the best solution. Some of the snippets just had both spaces and tabs in a single string of C which got me started down the rabbit hole.

It might confuse people's IDEs if we have tabs and spaces together in Python, but the reasoning is sane and I'm happy with whichever cuz I'm a noob to C, as long as it's consistent within snippets, and ideally a file, but the mixing of C and Python in a single file is confusing and only for simple cases a lot of the time anyway.

24653324.561322998 node 24728 path:/index.html
24653335.343401998 node 24728 path:/images/welcome.png
24653340.510164998 node 24728 path:/images/favicon.png
TIME(s) COMM PID ARGS
Copy link
Member

Choose a reason for hiding this comment

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

This is what the tool actually outputs, we should change this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I didin't mean to edit any tool outputs.

@palmtenor
Copy link
Member

LGTM. Could you collapse the commits to one?

@zoidyzoidzoid
Copy link
Contributor Author

Awesome, thanks. Done

@yonghong-song
Copy link
Collaborator

@4ast could you take a look?

@4ast 4ast merged commit 447ad50 into iovisor:master Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants