Ticket #1215 (closed cage: fixed)

Opened 5 years ago

Last modified 4 years ago

Factor out Duplicated Code in vivify/fetch opcodes

Reported by: chromatic Owned by: soh_cah_toa
Priority: minor Milestone:
Component: core Version: 1.7.0
Severity: low Keywords: ops newbie
Cc: Language:
Patch status: Platform: all

Description

The fetch and vivify opcodes in src/ops/experimental.ops can share almost all of their bodies between each other (if not between both types of ops themselves). We should extract the similar code into a static function or two in the PREAMBLE section of the experimental.ops file.

I'll do this in a day or two if no one beats me to it. In theory, we can do this in the PREAMBLE. I haven't seen it done yet, however.

Attachments

tt1215.patch Download (55.1 KB) - added by soh_cah_toa 4 years ago.
This should work now. 'make bootstrap-ops' ran with no errors.

Change History

in reply to: ↑ description   Changed 4 years ago by jkeenan

Replying to chromatic:.

I'll do this in a day or two if no one beats me to it.

Can we get an update on the status of this ticket?

Thank you very much.

kid51

  Changed 4 years ago by cotto

  • keywords newbie added

vivify and fetch are still poorly factored. It should be a simple matter to break out the common code for anyone reasonably familiar with C who has a few spare tuits. I've added the newbie tag to indicate as much.

  Changed 4 years ago by soh_cah_toa

  • status changed from new to assigned
  • owner changed from chromatic to soh_cah_toa

I'll take care of this. It'll be done before by tomorrow night.

  Changed 4 years ago by soh_cah_toa

I submitted the patch in a pull request. I'll close this once the changes are merged.

  Changed 4 years ago by jkeenan

Please submit your patch as an attachment to this ticket.

For a number of reasons:

1. Not all developers are fluent in pull requests from other repositories. This poses an obstacle to getting your patch seen by the largest possible number of developers. (And you haven't identified your repository.)

2. Even if we were all that fluent in git, github.com is comparatively slow.

3. Attaching the patch here makes it part of the permanent record of this ticket.

Please post it here so that it gets the broadest possible evaluation.

Thank you very much.

kid51

follow-up: ↓ 7   Changed 4 years ago by cotto

It sounds like pull requests vs. patches is a point that could use some discussion. I much prefer github pull requests and would like to see us make better use of them. I'll make sure to bring this up at the next #ps.

I've given some feedback as part of the pull request. For the record, it's at  https://github.com/parrot/parrot/pull/129 .

in reply to: ↑ 6   Changed 4 years ago by jkeenan

Replying to cotto:

It sounds like pull requests vs. patches is a point that could use some discussion.

cotto,

I strongly disagree with the direction you're heading in.

If a Parrot contributor is working on a problem covered by a Trac ticket, then it is reasonable to expect that the solution developed for that problem will appear in the ticket.

If people also want to issue git pull requests, that's fine. And if it's a problem not being tracked by a Trac ticket -- say, a problem small enough to be pasted -- then that's fine as well.

But if we've opened a ticket, and if a developer has taken the ticket, then the proposed solution should be attached to the ticket.

Thank you very much.

kid51

  Changed 4 years ago by cotto

A couple things comments: First, please include the generated core_ops.c file in your diff. If there's no diff, you forgot to rebootstrap the ops. Run make bootstrap-ops or just ./ops2c --core and the core_ops file will be regenerated. The attached patch misspells "BEGIN_OPS_PREAMBLE" and doesn't build. Second, the patch incorrectly changes some calls to VTABLE_*_pmc_keyed_*. That part of the code doesn't need any changes and causes the build to fail with a bootstrapped core_ops file.

Thank you for working on this.

Changed 4 years ago by soh_cah_toa

This should work now. 'make bootstrap-ops' ran with no errors.

  Changed 4 years ago by cotto

  • status changed from assigned to closed
  • resolution set to fixed

Thanks. I applied the attached patch as 91bd7c3.

Note: See TracTickets for help on using tickets.