-
Notifications
You must be signed in to change notification settings - Fork 556
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
[PATCH] Cleanup two argument open usage. #15721
Comments
From @lightseyCreated by @lightseyThis is a bug report for perl from john@nixnuts.net, ----------------------------------------------------------------- The 'cpan' and 't' directories were skipped. Notable changes: Perl Info
|
From @jkeenanOn Thu, 17 Nov 2016 17:52:28 GMT, john@nixnuts.net wrote:
1. The patch is large but contains the same changes over and over again -- which makes me nod off while reviewing it. :-) Could I ask that we have multiple people look at this, say, in 1000-line chunks? (I've looked over lines 1-1000.) 2. I'm in favor of this, but I recognize that there are people who may object on the basis of "Why fix something that isn't broken?". If you are one of that group, please raise your objection now. Thank you very much.
-- |
The RT System itself - Status changed from 'new' to 'open' |
From @jkeenanOn Thu, 17 Nov 2016 17:52:28 GMT, john@nixnuts.net wrote:
John, there are places in this patch where I believe we can do a bit more cleanup, i.e., places where, after re-writing an 'open' call in 3-arg form, we have double-quotes around a scalar holding a file name. ##### I believe that in these cases we don't need the double-quotes around the 3rd arg -- correct? Thank you very much. -- |
From @csjewellOn Fri, 23 Dec 2016 05:46:03 -0800, jkeenan wrote:
Incorrect. In all those cases, they are either appending an extension or filename on, or a second scalar. Therefore, they all need the double quotes, as an alternative to something like $AppName . '.app' |
From @jkeenanOn Fri, 23 Dec 2016 14:16:30 GMT, csjewell wrote:
Ah! The value of additional eyes on a problem. curtis++. I've now reviewed the first 2000 lines of the patch. Only 3500 to go! Anyone want to help out? Thank you very much. -- |
From @jkeenan
Pushed to blead in commit 1ae6ead. John, can you double-check that commit against your original patch? Since the patch was large and submitted some time ago, blead had moved. So the original patch did not apply cleanly. I essentially took it apart and applied it in chunks via 'patch'. Then I had to increment certain $VERSION numbers and run certain programs to pass the porting tests. So your proofreading would be helpful to make sure I didn't miss anything crucial. Also, you might want to submit -- in a new perlbug ticket -- a patch to clean up 2-arg open() calls like the following which still remain. ##### 193 open(SAVEERR, ">&STDERR"); Thank you very much. -- |
From @lightseyOn Fri, 2016-12-23 at 11:06 -0800, James E Keenan via RT wrote:
The minor code differences look fine to me. Thanks for following through on
I left the filehandle-copying opens in the two-argument format for compatibility If you don't think compatibility with 5.6.2 is worth keeping, particularly in |
From @jkeenanOn Fri, 23 Dec 2016 23:14:31 GMT, john@nixnuts.net wrote:
No, you can leave them as is. I overlooked your rationale for leaving them alone. Resolving ticket. -- |
@jkeenan - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#130122 (status was 'resolved')
Searchable as RT130122$
The text was updated successfully, but these errors were encountered: