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

WIP: node templates #1

Closed
wants to merge 28 commits into from
Closed

WIP: node templates #1

wants to merge 28 commits into from

Conversation

Morriar
Copy link

@Morriar Morriar commented Oct 29, 2024

No description provided.

Morriar added 28 commits October 29, 2024 17:21
@Morriar
Bump Steep version
2a3f9a1 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Modify extconf to allow files under src/ and include/ directory
aac8232 
Ultimately we want to separate the pure C implementation files
(under `src/` and `include/`) from the Ruby extension files
(under `ext/rbs_extension`).

This change allow us to build the Ruby extension using files outside of
the `ext/rbs_extension` directory.

Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Add templating mechanism to generate .c/.h files
8d07698 
Generate the c/h code for constants definitions and objects initialization.
The generated files are generated under `src/` and `include/` as to not
conflict with the files from `ext/rbs_extension` and will soon replace
`constants.{h,c}` and `ruby_objs.{h,c}`.

We make sure the Rake task builds the required templates so we can compile
the extension.

Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Annotation node
32af832 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Comment node
1ae7f86 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generte RBS::AST::Declarations::Class node
32e11a1 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::Class::Super node
6dd9c98 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::ClassAlias node
61a1f63 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::Constant node
0e6b38d 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::Global node
2e5e5f4 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::Interface node
34a1968 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::Module node
1144f2c 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::Module::Self node
9f08089 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::ModuleAlias node
e0766f9 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Declarations::TypeAlias node
9443326 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Directives nodes
d849551 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Members::Alias node
014ecf0 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Members::Attr* nodes
ba88c63 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Members::*Variable nodes
e1c02b5 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Members::Mixin nodes
8e85b2f 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Members::MethodDefinition* nodes
6666193 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::Members::Visibility nodes
dd89b5d 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::AST::TypeParam node
c1fdd3f 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::MethodType node
91cd080 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::Namespace node
0247b48 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::TypeName node
3fce45c 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Generate RBS::Types::* nodes
0a7c45d 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar
Clean unnessary files
ec2f417 
Signed-off-by: Alexandre Terrasa <[email protected]>
@Morriar Morriar self-assigned this Oct 29, 2024
amomchilov
amomchilov reviewed Oct 30, 2024
View reviewed changes
ext/rbs_extension/parser.c
Comment on lines +1224 to +1226
if (unchecked) {
rb_funcall(param, rb_intern("unchecked!"), 0);
}
Copy link

Choose a reason for hiding this comment

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

What's this about?

Copy link
Author

Choose a reason for hiding this comment

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

I needed to extract it from the ruby_objs code since we now generate it

ext/rbs_extension/parser.c
Comment on lines +1788 to +1793
enum TokenType type;

member_range.start = state->current_token.range.start;
comment_pos = nonnull_pos_or(comment_pos, member_range.start);

type = state->current_token.type;
Copy link

Choose a reason for hiding this comment

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

Can we drop the forward declaration?

Suggested change
enum TokenType type;
member_range.start = state->current_token.range.start;
comment_pos = nonnull_pos_or(comment_pos, member_range.start);
type = state->current_token.type;
enum TokenType type = state->current_token.type;
Suggested change
enum TokenType type;
member_range.start = state->current_token.range.start;
comment_pos = nonnull_pos_or(comment_pos, member_range.start);
type = state->current_token.type;
member_range.start = state->current_token.range.start;
comment_pos = nonnull_pos_or(comment_pos, member_range.start);
enum TokenType type = state->current_token.type;

Copy link
Author

Choose a reason for hiding this comment

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

It's the style of the original code, I'd prefer to follow it for now to avoid inconsistencies. We can clean all of it later 👍

ext/rbs_extension/parser.c
Comment on lines +1842 to +1843
VALUE comment = get_comment(state, comment_pos.line);
switch (type)
Copy link

Choose a reason for hiding this comment

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

Is this just an inlining of rbs_ast_members_mixin()?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, since we generate the specialized methods

templates/include/rbs/constants.h.erb
extern VALUE <%= node.c_constant_name %>;
<%- end -%>

void rbs__init_constants();
Copy link

Choose a reason for hiding this comment

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

This is subtly incorrect: Prior to C23 (which we aren't using), this means "takes unspecified params", not "takes no parameters".

Suggested change
void rbs__init_constants();
void rbs__init_constants(void);

https://stackoverflow.com/a/51080

Copy link
Author

Choose a reason for hiding this comment

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

Kept the original definition, not sure if we want to change it right now?

Copy link

Choose a reason for hiding this comment

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

Yep, I'll keep it "as-is" in our first PR, and add this to our second

@amomchilov
Copy link

Merged upstream in ruby#2098

@amomchilov amomchilov closed this Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amomchilov amomchilov amomchilov left review comments

Assignees

@Morriar Morriar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Morriar @amomchilov