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

Comments #198

Merged
merged 18 commits into from
Mar 1, 2021
Merged

Comments #198

merged 18 commits into from
Mar 1, 2021

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Nov 18, 2020

@zygoloid zygoloid added WIP proposal A proposal labels Nov 18, 2020
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Nov 18, 2020
@zygoloid zygoloid changed the title comments Comments Nov 19, 2020
@zygoloid zygoloid added proposal rfc Proposal with request-for-comment sent out and removed WIP labels Nov 19, 2020
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Trying to avoid style comments since it's just a proposal doc, let me know if you want style comments...

proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
Comment on lines 124 to 126
extend a comment to subsequent lines. There shall be no text other than
horizontal whitespace before the `//` characters introducing a comment. (Either
all of a line is a comment, or none of it.)
Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to have agreed to try this last sentence out, but it seems more likely to change than other parts of this proposal -- is there a need to mark this as provisional or experimental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good idea. Done.

Comment on lines 175 to 176
**Open question:** Should we accept only one or the other kind of documentation
comment?
Copy link
Contributor

Choose a reason for hiding this comment

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

My answer: yes.
To me, picking either one is better than choosing both.
I'd rather pick one and switch if we decide that we picked the wrong one than have both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll run a poll in #syntax to gauge consensus.

Copy link
Contributor Author

@zygoloid zygoloid Dec 9, 2020

Choose a reason for hiding this comment

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

Based on subsequent discussion, I'm no longer proposing to handle documentation via comments. I think that resolves this comment thread.

Carbon code is expected to include documentation comments. Specifying such
comments as part of the language definition allows a consistent interpretation
of the comments, and a consistent attachment of the comments to entities, to be
provided for Carbon programs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should go further and specify a format for doc comments? I saw this recent thread on HN where people were pretty happy with Rust's approach of using markdown with Rust-specific extensions, such as the ability to link to language symbols, not just URLs: https://news.ycombinator.com/item?id=25149969

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree and support this direction, but it might be better to leave details to a separate proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an open question for doc comment formatting.

// It's OK to include a //} in the middle of this comment; it's not a
// comment introducer so doesn't end the block comment.

//} is not a closing block comment line, so doesn't end the comment.
Copy link
Contributor

Choose a reason for hiding this comment

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

because it doesn't start at the beginning of the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please clarify that in the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by removal of this section.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

As I went through commenting, I decided I really want a clear proposal on whether block comments or repeated line comments should predominantly be used for long comments. There are a few comments touching on this, and I made the comment on line 104 at the end to be unambiguous.

proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
- In favor of `//!`: it is less likely to be confused with `//`; writing `//`
where `///` is intended is a common error in C++ code using Doxygen.

#### Documentation comments rationale
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a use case that seems to fall into the cracks between doc comments and block comments: Sometimes it's useful to provide at least part of the documentation in the form of a block comment, so that it can be cut and pasted without having to manually remove // from the beginning of each line, e.g.

/*
For example, this program can be run with:

a.out --flag --other_flag --yet_another_flag \
  --flag_name_that_pushes_us_over_80_characters
*/

Any thoughts on whether/how Carbon could support this use case? It seems like it might work better if we use string literal attributes instead of comments for documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on prior discussion, my expectation is that we'll have a string-literal syntax for documentation, including support for block string literals. I'd expect that such support could be used here. Eg:

#"""
This program can be run with:

```
a.out --flag --other_flag --yet_another_flag \
  --flag_name_that_pushes_us_over_80_characters
```
"""
package Foo impl;
...

proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
zygoloid and others added 2 commits December 8, 2020 18:46
Co-authored-by: Geoff Romer <gromer@google.com>
proposals/p0198.md Outdated Show resolved Hide resolved
The intent is that by omitting them, we'll get better information on how
important (or unimportant) it would be to include such a facility.
proposals/p0198.md Outdated Show resolved Hide resolved
Comment on lines +138 to +140
> _Experimental:_ No support for block comments is provided. Commenting out
> larger regions of human-readable text or code is accomplished by commenting
> out every line in the region.
Copy link
Contributor

Choose a reason for hiding this comment

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

What other languages that don't provide block comments? It looks like Visual Basic and R don't, although VB has IDE assistance for block commenting code and is essentially an IDE-only language. I'm very wary of this experiment -- I don't think we should pursue it, it feels like it will create a barrier for Carbon adoption by C++ developers due to poor ergonomics.

Copy link
Contributor Author

@zygoloid zygoloid Jan 26, 2021

Choose a reason for hiding this comment

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

Python has no block comments.

The rationale for including block comments was incredibly weak. Consensus seemed to be that we didn't want block comments for human-readable text, and the only remaining use cases were for commenting out code transiently during development and for bisecting compiler test cases. (It seems the Go folks have come to the same conclusion: https://www.digitalocean.com/community/tutorials/how-to-write-comments-in-go) So we're left needing to justify the inclusion of dedicated language syntax for that purpose, which is challenging when the existing comment syntax can be used to achieve the same thing.

Regarding IDE assistance, I think there's a large spectrum between IDE-only languages and languages whose code can be effectively maintained in notepad, and while I agree that we don't want an IDE-only language, I don't think this pushes us too far in that direction. But I don't know -- that's why I consider this experimental. If there is a barrier, we can find that out and adapt, probably long before there's anyone writing Carbon code other than ourselves. If there isn't, we'll have added a feature we didn't need, and we probably won't ever find that out.

In general, given that it's much easier to add language features later than it would be to remove them later, presenting a more minimal feature set initially supports our language evolution goal.

Copy link
Contributor

@jonmeow jonmeow Jan 27, 2021

Choose a reason for hiding this comment

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

While Python doesn't technically do block comments, it does allow """ string literals in places where it's a no-op (something I would assume we'd treat as a compile error due to the lack of side-effects). (and I have had issues with Python due to this, it makes it a nuisance to comment out large amounts of code with docstrings)

Regarding Go, I think the best reference is the official "Effective Go" docs: "Line comments are the norm; block comments appear mostly as package comments, but are useful within an expression or to disable large swaths of code." So they do note use for human-readable text.

Note, I'm fine if we disagree with this stance, I just thought I'd speak up with trepidation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. If nothing else, this is something we should make sure the core team explicitly considers as part of their decision.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly nervous about leaving out block comments too, just because they're present in so many language. (Including C++ and all of the languages anyone talks about as potential replacements for it.) On the other hand, the comment from zygoloid earlier is pretty persuasive: this is an easy decision to reverse if anyone finds it too annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged. Would it improve this proposal if I made the question "do we want to run the no-block-comments experiment?" an explicit open question for the core team? I expect the core team will consider and discuss this part of the proposal -- probably more than anything else -- either way.

proposals/p0198.md Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

Just wanted to say this proposal generally LGTM. I'm very nervous about not including block comments at all, but I also don't think it's really worth trying to figure out right now and it does have the advantage of being wonderfully simple.

proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
zygoloid and others added 3 commits February 1, 2021 14:00
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

I think essentially all of my questions are addressed. Some more editorial suggestions below - mostly I find the term "maintenance comments" not working well for me. I've listed a potential alternative, but maybe thats only better for me...

Comments serve a variety of purposes in existing programming languages. The
primary use cases are:

- _Documentation_: human-readable commentary explaining to users and future
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this a bit more precise:

Suggested change
- _Documentation_: human-readable commentary explaining to users and future
- _API documentation_: human-readable commentary explaining to users and future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(See below.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Will resolve either way (based on the resolution below).

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 believe this is addressed now.

};
```

- _Maintenance comments_: human-readable commentary explaining intent and
Copy link
Contributor

Choose a reason for hiding this comment

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

With the above, maybe:

Suggested change
- _Maintenance comments_: human-readable commentary explaining intent and
- _Implementation documentation_: human-readable commentary explaining intent and

It doesn't seem exclusively about maintenance, but about documenting the behavior / logic / rationale / etc of the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line I'm trying to draw would classify (for example) comments on private members of a class or a function that's an implementation detail as "documentation" but (for example) comments in a function body as "maintenance comments". Broadly, if we imagine an automatic documentation generation tool in a "generate internal documentation" mode, or an IDE that displays documentation, I'm trying to classify the things that such a tool would care about as "documentation" and the other comments as "maintenance comments". I think renaming the latter category to "implementation documentation" would make the distinction less clear, because those are comments that you would not translate into machine-generated code documentation.

Part of this is framing the background in terms that fit the proposal: I'm trying to position the terminology here so the things we write as @"..." (straw-person syntax) are always called documentation and never called comments, and the things we write as // ... are always called comments and never called documentation. That's how I'd like us to describe things going forward, if this proposal is accepted.

That being said, yes, you need to take quite a broad interpretation of "maintenance" (including, for example, reading the code with no intent to modify it) for this to really describe the entire purpose of such comments. Would "implementation comments" work better here?

I have no major concerns renaming "documentation" to "API documentation", except that it seems redundant if we don't also rename this one to "<something> documentation", which I'm reluctant to do for framing/pedagogy reasons. I'd also be a little concerned that people will see "API" and think exclusively "public interface".

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy with "implementation comments", and that makes me not care whether you include "API" above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to "implementation comments".

proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
proposals/p0198.md Outdated Show resolved Hide resolved
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Comment on lines +138 to +140
> _Experimental:_ No support for block comments is provided. Commenting out
> larger regions of human-readable text or code is accomplished by commenting
> out every line in the region.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly nervous about leaving out block comments too, just because they're present in so many language. (Including C++ and all of the languages anyone talks about as potential replacements for it.) On the other hand, the comment from zygoloid earlier is pretty persuasive: this is an easy decision to reverse if anyone finds it too annoying.

> the `//` characters introducing a comment. Either all of a line is a comment,
> or none of it.

The character after the `//` is required to be a whitespace character. Newline
Copy link
Contributor

Choose a reason for hiding this comment

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

I also have misgivings about requiring whitespace after "//", since it's a seemingly unnecessary difference from C++. But again, it should be relatively easy to fix if it turns out to be too annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we've abandoned all of our past uses of this lexical space, and don't have any immediate plans to add new uses, I don't think there's a pressing need to keep it reserved. But if we think there's any chance we might want new comment flavors in the future, keeping this syntax reserved seems marginally useful. Do we think this is going to frustrate people in practice? I could imagine people wanting // with no following whitespace for commenting out code in particular.

Perhaps we should describe this restriction as experimental too?

Copy link
Contributor

Choose a reason for hiding this comment

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

That probably makes sense. This comment, too, was motivated by the question of automatic C++-to-Carbon translation. I'm thinking of cases where people use ASCII art for comments, in which case the requirement to add a ' ' at the beginning of some lines would lead to a lower fidelity translation than we might otherwise have.

to the end of the line. We have no mechanism for physical line continuation, so
a trailing `\` does not extend a comment to subsequent lines.

> _Experimental:_ There can be no text other than horizontal whitespace before
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth saying something about what an automatic C++-to-Carbon translator should do with comments under this proposal, given the more restrictive placement. (Or is the idea that we shouldn't expect C++ comments to get transferred to the Carbon version verbatim?)

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 don't think I have a solid answer here, but I do think it's important that we translate comments in the common case.

Comments at the start of a line seem straightforward to translate, and I'd imagine we'd make a policy decision that we produce documentation whenever a comment appears at a suitable position. Trailing comments seem slightly harder, because there's the risk that they aren't directly associated with the line to the left:

int left;  // The location of the widget relative
int top;  // to its closest ancestor.

But I think we can get a 95+% conversion by moving these comments before their declarations.

Comments in unusual places (specifically, neither at the beginning nor at the end of a statement) seem likely to cause problems for translation irrespective of whether we permit intra-line / block / trailing comments in Carbon. I don't really have a good answer there. Tracking where they are in the C++ AST and mapping that into the Carbon AST seems like it may be infeasible in general. I expect such comments would also be a problem when migrating between versions of Carbon, although likely less so, because we'll hopefully mostly be doing localized rewrites rather than full-source-file rewrites.

I'm happy to include something along these lines in the proposal, though I wonder how much detail we should go into here versus in a proposal more targeted at migration. Maybe this is something we should be systematically asking of all proposals involving language syntax?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're probably right that this is something we should be systematically asking of all proposals involving language syntax. Or to make a slightly weaker claim: this is something we should be systematically asking of proposals involving language syntax in all cases where there's reason to think translation might be problematic.

I asked in this case because the proposed Carbon comment syntax is more restrictive than the existing C++ comment syntax, which makes me wonder if there would always be a good Carbon translation for C++ comments. I think there's a chance, once we start writing a C++-to-Carbon translator, that we'll find it's easiest to allow Carbon comments in all of the places where C++ comments are allowed.

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

FWIW, I'm pretty happy here (if, like others, nervous about block comments).

};
```

- _Maintenance comments_: human-readable commentary explaining intent and
Copy link
Contributor

Choose a reason for hiding this comment

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

Happy with "implementation comments", and that makes me not care whether you include "API" above.

Comments serve a variety of purposes in existing programming languages. The
primary use cases are:

- _Documentation_: human-readable commentary explaining to users and future
Copy link
Contributor

Choose a reason for hiding this comment

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

Will resolve either way (based on the resolution below).

@jonmeow jonmeow added proposal accepted Decision made, proposal accepted and removed needs decision labels Feb 19, 2021
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Approving per responses at https://forums.carbon-lang.dev/t/request-for-decision-comments-198, working on decision...

@jonmeow jonmeow removed the proposal rfc Proposal with request-for-comment sent out label Feb 23, 2021
zygoloid added a commit that referenced this pull request Feb 26, 2021
Require a space after a `//`, and recognize but reject a `//` that follows non-whitespace text in the same line. Remove DocComment token kind that ended up not being part of the design.
@zygoloid zygoloid merged commit 15c8cf1 into carbon-language:trunk Mar 1, 2021
jonmeow added a commit that referenced this pull request Mar 15, 2021
@zygoloid zygoloid deleted the proposal-comments branch March 11, 2022 00:55
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Require a space after a `//`, and recognize but reject a `//` that follows non-whitespace text in the same line. Remove DocComment token kind that ended up not being part of the design.
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
Proposal as accepted by core team on 2021-02-19.
chandlerc pushed a commit that referenced this pull request Jun 28, 2022
jonmeow pushed a commit that referenced this pull request Mar 30, 2023
The [Comments #198](#198) got accepted but the details were not updated in the design docs. Added `comments.md` file to add the details of the proposal and its discussion.

Closes #1994
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot. proposal accepted Decision made, proposal accepted proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants