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

Add parsing (only) support for class prefix and 'extends' #1880

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Add parsing (only) support for class prefix and 'extends' #1880

merged 2 commits into from
Aug 3, 2022

Conversation

Pixep
Copy link
Contributor

@Pixep Pixep commented Aug 2, 2022 •
edited
Loading

Provides a first implementation iteration towards class prefix and extension, along with user-friendly error regarding missing implementation for #1881

Explorer behavior
For class prefix base and abstract:

Class prefixes `base` and `abstract` are not supported yet

For extension with extends:

Class extension with `extends` is not supported yet

Motivation

  • Provides user-friendly error for these unsupported features
  • First implementation increment in supporting class prefix and extension.

Pixep
Pixep commented Aug 2, 2022
View reviewed changes
explorer/syntax/parser.ypp Outdated
@@ -1059,14 +1064,28 @@ type_params:
| tuple_pattern
{ $$ = $1; }
;
class_declaration_prefix:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to get the job done (tests passing), but I may be missing some subtleties of the parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this approach seems reasonable to me. I'm pretty sure these are mutually exclusive.

@Pixep
Add parsing (only) support for class prefix and 'extends'
5d9a669 
Parsing only support, gracefully fails at compilation indicating
that 'base', 'abstract', and 'extends' are not yet supported.
@Pixep Pixep force-pushed the feature/class-prefix-extends-parsing branch from 6b4cc14 to 5d9a669 Compare August 2, 2022 03:15
jonmeow
jonmeow reviewed Aug 2, 2022
View reviewed changes
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.

Thanks! I think it's good to put in an error like this, and the change looks correct structurally. I'd just like to think about better names for the ClassPrefix enum, because I think the name doesn't quite capture the choice being made.

explorer/syntax/parser.ypp Outdated
@@ -1059,14 +1064,28 @@ type_params:
| tuple_pattern
{ $$ = $1; }
;
class_declaration_prefix:
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this approach seems reasonable to me. I'm pretty sure these are mutually exclusive.

explorer/ast/declaration.h Outdated
@@ -179,31 +180,40 @@ class SelfDeclaration : public Declaration {
auto value_category() const -> ValueCategory { return ValueCategory::Let; }
};

enum class ClassPrefix { StandardClass, BaseClass, AbstractClass };
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
enum class ClassPrefix { StandardClass, BaseClass, AbstractClass };
enum class ClassExtensibility { None, Base, Abstract };

I think this should probably be "None, Base, Abstract" because (with enum class) the "Class" suffix feels a little repetitive. But I also think "Prefix" isn't quite capturing the state: looking at the design wording, maybe "Extensibility" capture this better?

Suggestion is merging these two thoughts, though will obviously require more code edits.

Copy link
Contributor Author

@Pixep Pixep Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with that. By merging these two thoughts I understand renaming the enum plus updating its values, is that what you meant? Because it doesn't look like a lot of code edits too me, still super quick :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I meant, thanks. :)

jonmeow
jonmeow requested changes Aug 2, 2022
View reviewed changes
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.

(just explicitly requesting changes)

@Pixep Pixep requested a review from jonmeow August 3, 2022 02:33
@jonmeow jonmeow merged commit 0943111 into carbon-language:trunk Aug 3, 2022
@jonmeow
Copy link
Contributor

jonmeow commented Aug 3, 2022

Thanks for the contribution! And merged.

@Pixep Pixep deleted the feature/class-prefix-extends-parsing branch August 4, 2022 05:10
@chandlerc chandlerc added the explorer Action items related to Carbon explorer code label Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonmeow jonmeow jonmeow approved these changes

Assignees
No one assigned
Labels
explorer Action items related to Carbon explorer code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Pixep @jonmeow @chandlerc