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.

Sign up for GitHub

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

Jump to bottom

Placement of the word virtual #754

Closed
josh11b opened this issue Aug 16, 2021 · 5 comments
Closed

Placement of the word virtual #754

josh11b opened this issue Aug 16, 2021 · 5 comments
Labels
leads question A question for the leads team

Comments

@josh11b
Copy link
Contributor

josh11b commented Aug 16, 2021

Context for this question is this Google doc proposing inheritance.

This option puts the word virtual after the signature, and is optimized for callers of the method:

base class MyBaseClass {
  fn Overridable[me: Self]() -> i32 virtual { return 7; }
}

This option puts the word virtual in a place familiar to C++ users:

base class MyBaseClass {
  virtual fn Overridable[me: Self]() -> i32 { return 7; }
}
@josh11b
Copy link
Contributor Author

josh11b commented Aug 19, 2021

Here is the rationale for putting the virtual keyword to the right, from the linked doc:

  • This is much less salient information than the name and signature of the function, particularly for callers of the API.
  • This choice allows the function names to line up vertically.
  • This keyword is about the implementation. For example, it says whether there is an implementation of this method in this class at all.

Unless you are extending the class, callers would not notice replacing a virtual function with a non-virtual function calling a private virtual function.

@jonmeow
Copy link
Contributor

jonmeow commented Aug 19, 2021 •
edited
Loading

To give a few thoughts:

  • Carbon will likely have other markers on functions too: for example, perhaps attribute, inline, must_use_result, or abstract. A consistent story for placement may help, particularly as functions may frequently have several of these.

  • So far it's been discussed to put visibility keywords such as protected up front, as in protected fn Overridable. These apply to other keywords too, such as protected var my_var. We could reserve this placement for keywords which only apply to multiple declarations (typically fn and var).

    <common keywords> fn <name>(...) -> ... <fn-specific keywords> { <body> }
<common keywords> var <name>: <type> <var-specific keywords> = <expr>;

  • fn virtual Overridable placement is technically an option too, but places the keyword where we might otherwise try to train developers to look for identifiers.

  • Another option would be to instead think of them as similar to Python's decorators, encouraging placement on a preceding line. For example:

    @protected
@virtual
@must_use_result
fn Overridable[me: Self]() -> i32 { return 7; }

@josh11b josh11b mentioned this issue Aug 25, 2021
Merged
@chandlerc
Copy link
Contributor

FWIW, I do think we should have a consistent story for placement. I think the most unsurprising story is to have a sequence of keywords before fn. Ideally we should pick a single ordering of these keywords that we think reads best and just insist on that order for consistency.

For example, given the concrete suggestions we currently have, I would suggest:

[private | protected] [abstract | virtual | impl] fn ...

@zygoloid
Copy link
Contributor

See also the discussion towards the end of #665; in particular, this comment provides some thoughts for when we should put annotations either before or after the fn and concludes that all of our current annotations (private / protected / abstract / virtual / impl) belong prior to fn.

@chandlerc
Copy link
Contributor

Ok, let's close this as consensus then. Discussion with Kate seemed aligned.

@zygoloid zygoloid mentioned this issue Sep 7, 2021
Closed
@jonmeow jonmeow added the leads question A question for the leads team label Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
leads question A question for the leads team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@zygoloid @chandlerc @josh11b @jonmeow