Ticket #455 (closed todo: fixed)

Opened 5 years ago

Last modified 5 years ago

[TODO] Change name of 'Parrot_add_library_path' to 'Parrot_pf_add_library_path_from_cstring'

Reported by: allison Owned by: jkeenan
Priority: normal Milestone: 1.3
Component: none Version:
Severity: medium Keywords:
Cc: jkeenan Language:
Patch status: applied Platform:

Description

The function 'Parrot_add_library_path' in src/library.c will be renamed to 'Parrot_lib_add_path_from_cstring'. A new function 'Parrot_lib_add_path' will be added that accepts a Parrot STRING.

The C string version will simply convert the C string to a Parrot STRING and call 'Parrot_lib_add_path'. (Currently, the first thing the C string version does is convert the C string to a Parrot STRING. It's a waste to have opcodes converting STRINGs to C strings, only to be immediately converted back to STRINGS.)

Attachments

add_library_path.diff Download (3.1 KB) - added by jkeenan 5 years ago.
Current state of work being done in the add_library_path_remove branch

Change History

  Changed 5 years ago by coke

  • owner set to coke

follow-up: ↓ 3   Changed 5 years ago by coke

  • owner changed from coke to allison

Note: the function names in the subject and the body of this message differ; we should probably also consider 'Parrot_add_library_path_from_cstring' in this.

Passing back to allison for re-evaluation. We may need to open another ticket for other deprecation items.

in reply to: ↑ 2 ; follow-up: ↓ 4   Changed 5 years ago by jkeenan

  • cc jkeenan added

There is an entry for this item in DEPRECATED.pod:

       Parrot_add_library_path [eligible in 1.1]
           Will be renamed to "Parrot_lib_add_path_from_cstring".

           <https://trac.parrot.org/parrot/ticket/455>

This suggests that we could work on this ticket right away. Am I correct in thinking that this will largely be a matter of changing all instances of Parrot_add_library_path to Parrot_lib_add_path_from_cstring -- much of which has already been done -- and then making all tests pass?

If so, then I will take the ticket and prepare a patch.

These are the files that would have to be touched:

$ fns . | xargs grep -l Parrot_add_library_path
./src/packfile.c
./src/library.c
./include/parrot/library.h
./compilers/imcc/main.c
./DEPRECATED.pod

Thank you very much.

kid51

in reply to: ↑ 3 ; follow-up: ↓ 5   Changed 5 years ago by jkeenan

Replying to jkeenan:

This suggests that we could work on this ticket right away. Am I correct in thinking that this will largely be a matter of changing all instances of Parrot_add_library_path to Parrot_lib_add_path_from_cstring -- much of which has already been done -- and then making all tests pass? If so, then I will take the ticket and prepare a patch.

I created the add_library_path_remove branch in SVN to work on this. I got as far as the attached patch, add_library_path.diff. At that point I began to get build warnings such as:

src/packfile.c:4776: warning: passing argument 2 of 'Parrot_add_library_path_from_cstring' from incompatible pointer type
src/packfile.c:4778: warning: passing argument 2 of 'Parrot_add_library_path_from_cstring' from incompatible pointer type

Subsequent discussion on IRC #parrot with Infinoid and NotFound suggested that more needed to be done for this TT than I originally understood:

09:21 NotFound "The C string version will simply convert the C string to a Parrot STRING and call 'Parrot_lib_add_path'."
09:21 NotFound So, the ticket ask for the two versions.
09:22 Infinoid Oh, I see.  So the callers of the old function (who are right to call the old function) should now call Parrot_lib_add_path(), instead
09:22 NotFound The naming used seem confusing, anyway.
09:22 kid51 So this is more than a simple "rip out this feature" deprecation.
09:22 Infinoid It's a "rename this function to that, and add this other function to make some of the callers cleaner"
09:23 NotFound Is renamimg a function and adding another.
09:23 Infinoid You've got the "this other function" part done

So, anyone who has a better understanding of these issues is welcome to take a crack at them in the add_library_path_remove branch.

Thank you very much.
kid51

Changed 5 years ago by jkeenan

Current state of work being done in the add_library_path_remove branch

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

Replying to jkeenan:

So, anyone who has a better understanding of these issues is welcome to take a crack at them in the add_library_path_remove branch.

Infinoid picked up the ball and the results were committed to trunk in r39431. No test failures due to these changes. I'll keep the ticket open through Tuesday for final comments.

Thank you very much.
kid51

  Changed 5 years ago by jkeenan

  • status changed from new to assigned
  • owner changed from allison to jkeenan

  Changed 5 years ago by jkeenan

  • status changed from assigned to closed
  • milestone changed from 1.4 to 1.3
  • resolution set to fixed
  • patch set to applied

There have been no complaints in the allocated time, so I'm closing this ticket.

Thank you very much.
kid51

Note: See TracTickets for help on using tickets.