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

Fix #15438, Add to General Formatting Guidelines for C and Julia #15439

Closed
wants to merge 1 commit into from
Closed

Fix #15438, Add to General Formatting Guidelines for C and Julia #15439

wants to merge 1 commit into from

Conversation

thepulkitagarwal
Copy link
Contributor

refer to #15438

@@ -154,6 +154,7 @@ Make sure that [Travis](http://www.travis-ci.org) greenlights the pull request w
- use lower case with underscores for method names
- it is generally preferred to use ASCII operators and identifiers over
Unicode equivalents whenever possible
- function signature should not be followed by an empty line
Copy link
Contributor

Choose a reason for hiding this comment

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

specify end of functions too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something like no empty line before the end of a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or something like: no empty line before end in julia and no empty line before } in c?

Copy link

Choose a reason for hiding this comment

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

It would be good if the word end is highlighted, in order to make it clearer.

Even this looks good enough too, however highlighting it would make it clearer IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it, please check!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Dawny33! I didn't read your note earlier. Highlighting wasn't added to the other lines, and so I assumed that I shouldn't add it.

Copy link

Choose a reason for hiding this comment

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

Yeah, I was skeptical with the suggestion too.

The current diff LGTM, can go ahead with it 👍

@stevengj
Copy link
Member

This is not quite correct. You can and should indent nested preprocessor macros (see e.g. this example in src/init.c), but the indentation comes after the # and not before. The # itself is always in the first column.

@@ -154,6 +154,8 @@ Make sure that [Travis](http://www.travis-ci.org) greenlights the pull request w
- use lower case with underscores for method names
- it is generally preferred to use ASCII operators and identifiers over
Unicode equivalents whenever possible
- no newline after function signature
- no newline before function end
Copy link
Member

Choose a reason for hiding this comment

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

Are these really necessary to spell out?

Copy link
Member

Choose a reason for hiding this comment

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

Occasionally, I've found it more readable with a newline after long function signatures so I don't think we should add this rule.

Copy link
Member

Choose a reason for hiding this comment

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

This came up in #15418.

@stevengj
Copy link
Member

In general, I think we should limit the style guide to things where there is significant disagreement among different programmers. More concretely, we only need to put rules in the style guide if we find that contributors are violating them. (Including too many rules makes it less likely that people will read the rules in the first place.)

e.g. we write if (cond), not if         (cond), but we don't need a "one space after if" item in the style guide because pretty much every C programmer spaces if statements this way.

Similarly for the "no newline before function end" rule; it's not something that people seem to violate, so we hardly need to spell it out. And have we had a problem with contributors indenting cpp macros?

@tkelman
Copy link
Contributor

tkelman commented Mar 10, 2016

People do add excess newlines or other nits that contributors who spend lots of time in the code end up cleaning up. I think the style guide should expand to include various things that we would want an auto formatter to enforce if we had one. Yes the current codebase has a few places in codegen and elsewhere that are inconsistent about indenting preprocessor directives, especially when code gets copy pasted from LLVM or libunwind or other projects with different style conventions.

@samoconnor
Copy link
Contributor

FWIW, I strongly disagree with "no newline after function signature".
IMHO it directly contradicts "use whitespace to make the code more readable" is some cases as @andreasnoack points out.

I make no claim to have better taste that anyone else, and at the end of the day, attempting to apply rationale to style is silly, so I'm not going to debate the issue.
However, I do think that those proposing "no newline after function signature" should copy/paste the examples that "violate" the rule here (along with a "corrected" version) so that readers of this PR can see before/after and arrive at their own opinion.

I was recently chastised for this blank line: https://github.com/JuliaLang/julia/blob/master/base/mapiterator.jl#L64.
I see this not as a "blank line after a function signature", but as a "blank line before a comment line" in a function where each step of the algorithm is spaced out and explained by a comment.

(Also I think the proposer means "no blank line" rather than no "newline")

@samoconnor
Copy link
Contributor

See also @tbreloff: #15418 (comment)

@tkelman
Copy link
Contributor

tkelman commented Mar 12, 2016

Quoting your C# StyleCop link:

An exception to this rule occurs when the single-line comment is the first item within its scope. In this case, the comment should not be preceded by a blank line.

@samoconnor
Copy link
Contributor

Re StyleCop's "exception to this rule"...

True, but that is because in C# there is a curly brace line at the start of the scope, so there is already whitespace above the comment. See the example from the referenced page below. The point is for the comment to have whitespace above it on the page so that it stands out as a "start reading here" point like the start of a paragraph or a heading.

Not allowed:
    public bool Enabled
    {
        get
        {
            Console.WriteLine("Getting the enabled flag.");
            // Return the value of the 'enabled' field.
            return this.enabled;  
        }
    }
Allowed:
    public bool Enabled
    {
        get
        {
            // Return the value of the 'enabled' field.
            return this.enabled;  
        }
    }

@nalimilan
Copy link
Member

Concretely, the vast majority of the existing code base does not have blank lines after signature, even when the body starts with a comment. Therefore, if we allow them we lose consistency an leave room for discussions, which wastes time when reviewing PRs. A simple rule without exceptions is better.

@nalimilan
Copy link
Member

Could we agree on a systematic rule here? Discussions regarding spacing in PRs are wasting everybody's time. See e.g. #15409.

@kshyatt kshyatt added docs This change adds or pertains to documentation needs decision A decision on this change is needed labels May 10, 2016
@StefanKarpinski StefanKarpinski added design Design of APIs or of the language itself and removed needs decision A decision on this change is needed labels Sep 13, 2016
@StefanKarpinski StefanKarpinski added this to the 0.5.x milestone Sep 13, 2016
@StefanKarpinski StefanKarpinski added help wanted Indicates that a maintainer wants help on an issue or pull request and removed help wanted Indicates that a maintainer wants help on an issue or pull request labels Oct 27, 2016
@JeffBezanson JeffBezanson removed this from the 0.5.x milestone May 21, 2018
@ViralBShah ViralBShah closed this Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design of APIs or of the language itself docs This change adds or pertains to documentation help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.