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

Added macro to create functions in PipelineNamespace #26606

Merged
merged 1 commit into from
May 23, 2020

Conversation

AbhishekSharma102
Copy link
Contributor

@AbhishekSharma102 AbhishekSharma102 commented May 21, 2020

Changes to constellation_msg.rs to generate namespace id using macros

@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 21, 2020
@jdm
Copy link
Member

jdm commented May 21, 2020

r? @gterzian

@highfive highfive assigned gterzian and unassigned Manishearth May 21, 2020
Copy link
Member

@gterzian gterzian left a comment

Choose a reason for hiding this comment

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

Thank you, looks likes a great start!

Would it be possible to additional functionality to the macro, maybe via a separate one, that would generate the various Id types, something like namespaced_id!(PipelineId)(which would then also generate the matching PipelineIndex)?

Perhaps this could be done with the same macro, but I am not sure, in that case it could be for example create_pipeline_namespace! {PipelineId, self} that would do what the current macro does, plus would also generate the PipelineId and PipelineIndex types.

@AbhishekSharma102
Copy link
Contributor Author

AbhishekSharma102 commented May 22, 2020

Hey!

I believe rust does not allow modification to variable names (like suffix and prefix). I was wondering if we could pass Pipeline to the macro, and it would append Id to get PipelineId and Index to get PipelineIndex . But rust does not allow it. So we would need to pass all the variable names.

We can definitely generate the Id and Index inside the macros. I am thinking of having two macros, one defined in the PR, and another one namespace_id which takes two args, first is id_name like PipelineId and second index_name like PipelineIndex and returns them.

Also I believe two macros would need to be called separately. Please correct if I'm wrong.

Let me know if this sounds good.

@gterzian
Copy link
Member

So we would need to pass all the variable names.

Ok, so you mean we'd have to pass PipelineId and PipelineIndex, sounds good to me, definitely better than the current situation.

I am thinking of having two macros, one defined in the PR, and another one namespace_id which takes two args, first is id_name like PipelineId and second index_name like PipelineIndex and returns them.

Sounds good.

Also I believe two macros would need to be called separately. Please correct if I'm wrong.

I'm not sure, let me know what you find out!

@AbhishekSharma102
Copy link
Contributor Author

Hey, I have added the macro. Let me know if this looks good.

Copy link
Member

@gterzian gterzian left a comment

Choose a reason for hiding this comment

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

Looks great! Just one question.

index: BlobIndex(self.next_index()),
}
}
create_pipeline_namespace_function! {next_pipeline_id, PipelineId, self, PipelineIndex}
Copy link
Member

Choose a reason for hiding this comment

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

nit: how about we name it a bit shorter, like namespace_id_method?

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 it

@AbhishekSharma102
Copy link
Contributor Author

Hey, updated the name. Let me know if anything else is needed.

@gterzian
Copy link
Member

Great! If you can please squash the third commit into the first, then it should be good to go!

@AbhishekSharma102
Copy link
Contributor Author

Squashed them into one.

@gterzian
Copy link
Member

Awesome, thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 16c3b89 has been approved by gterzian

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 23, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit 16c3b89 with merge 49d0055...

@bors-servo
Copy link
Contributor

☀️ Test successful - status-taskcluster
Approved by: gterzian
Pushing 49d0055 to master...

@bors-servo bors-servo merged commit 49d0055 into servo:master May 23, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 23, 2020
@AbhishekSharma102
Copy link
Contributor Author

@gterzian Can you please suggest another issue that I can take up?

@gterzian
Copy link
Member

I'm not sure, there are not many issues marked "less easy" any more, perhaps this one: #24684?

If you want something harder I think this one is interesting: #26571 (it might require some more guidance)

@AbhishekSharma102
Copy link
Contributor Author

I'd like to work on #26571 as I'm hoping there will be more learning. Could you please assign that to me? And please tell on what to read up for the same?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write a macro to generate namespaced Ids
6 participants