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
File::Glob GLOB_NOCHECK documentation unclear #14954
Comments
From @epaCreated by @epaThe documentation for File::Glob says "GLOB_NOCHECK" I understood this to mean: if you set the GLOB_NOCHECK flag, and the However, the behaviour looks like the opposite: % perl -MFile::Glob -E '@g = File::Glob::bsd_glob("nonexistent", GLOB_NOCHECK); say foreach @g' If GLOB_NOCHECK is set then you get an empty result from a This isn't consistent with the documentation - and it appears I don't know whether the best way forward is to fix the documentation Perl Info
|
From @iabynOn Thu, Oct 01, 2015 at 09:22:44AM -0700, Ed Avis wrote:
It's actually doing the right thing; it's a combination of two things that's If the second argument is omitted, C<GLOB_CSH> (or C<GLOB_CSH|GLOB_NOCASE> which happens to include GLOB_NOMAGIC, which is nearly equivalent to This code: use File::Glob qw(bsd_glob GLOB_NOCHECK GLOB_NOMAGIC); produces this output: bsd_glob(nonexistent): [nonexistent] which I think is the correct behaviour. -- |
The RT System itself - Status changed from 'new' to 'open' |
From @epaThanks for spotting the problems with my example code.
That's not quite true. As a bareword, it becomes the string value "GLOB_NOCHECK". But then this string gets treated as equivalent to zero. This is unfortunate given that the File::Glob documentation seems to imply that strings can be passed: on my Linux system, 'man File::Glob' displays POSIX FLAGS "GLOB_ERR" Those "" quotation marks seem to suggest that strings are passed to bsd_glob(). I know that the quotation marks have only appeared in there as an artefact of the perldoc-to-manpage conversion. And I know that the flags argument should be an integer not a string. But it's all a bit confusing as documented. Given this series of unfortunate events, could I suggest that if bsd_glob() is given a string instead of an integer for its flags argument, it should warn? For extra niceness it could handle strings such as "GLOB_NOCHECK" as being equivalent to the corresponding flag, but still with a warning that you should change your code. In my opinion, this warning from File::Glob should appear even if $^W is false. Once you start doing bitwise operations on barewords things get even stranger: % perl -E '$x = GLOB_NOCHECK | GLOB_BRACE; say $x' I don't suggest File::Glob should attempt to handle that; it should just warn that the unknown string will be treated as zero. Thanks for pointing out the default set of flags. I had overlooked that, assuming that they were all off by default. Should I send a documentation patch to make this clearer? |
From @iabynOn Thu, Nov 19, 2015 at 09:08:37AM -0800, Ed Avis via RT wrote:
But it already does: $ perl -MFile::Glob -wE 'File::Glob::bsd_glob("foo", GLOB_NOCHECK);'
It already does. So I don't think anything needs changing.
Yes, that would be fine. -- |
From @epaI have taken the liberty of putting explicit parens on the constants in example code, which would prevent any naively copied code (in oneliners, etc) from hitting the problem I encountered. What do you think? Inline Patchdiff --git a/ext/File-Glob/Glob.pm b/ext/File-Glob/Glob.pm
index c23b7df..da5596f 100644
--- a/ext/File-Glob/Glob.pm
+++ b/ext/File-Glob/Glob.pm
@@ -91,7 +91,7 @@ File::Glob - Perl extension for BSD glob routine
use File::Glob ':bsd_glob';
@list = bsd_glob('*.[ch]');
- $homedir = bsd_glob('~gnat', GLOB_TILDE | GLOB_ERR);
+ $homedir = bsd_glob('~gnat', GLOB_TILDE() | GLOB_ERR());
if (GLOB_ERROR) {
# an error occurred reading $homedir
@@ -176,10 +176,14 @@ means this will loop forever:
=head3 C<bsd_glob>
This function, which is included in the two export tags listed above,
-takes one or two arguments. The first is the glob pattern. The second is
-a set of flags ORed together. The available flags are listed below under
-L</POSIX FLAGS>. If the second argument is omitted, C<GLOB_CSH> (or
-C<GLOB_CSH|GLOB_NOCASE> on VMS and DOSish systems) is used by default.
+takes one or two arguments. The first is the glob pattern. The
+second, if given, is a set of flags ORed together. The available
+flags and the default set of flags are listed below under L</POSIX FLAGS>.
+
+Remember that to use the named constants for flags you must import
+them, for example with C<:bsd_glob> described above. If not imported,
+and Perl's warnings are not turned on, then the constants will be
+treated as bareword strings, which won't do what you what.
=head3 C<:nocase> and C<:case>
@@ -196,7 +200,9 @@ uses this internally.
=head2 POSIX FLAGS
-The POSIX defined flags for bsd_glob() are:
+If no flags argument is give then C<GLOB_CSH> is set, and on VMS and
+Windows systems, C<GLOB_NOCASE> too. Otherwise the flags to use are
+determined solely by the flags argument. The POSIX defined flags are:
=over 4
@@ -268,7 +274,7 @@ Expand patterns that start with '~' to user name home directories.
=item C<GLOB_CSH>
For convenience, C<GLOB_CSH> is a synonym for
-C<GLOB_BRACE | GLOB_NOMAGIC | GLOB_QUOTE | GLOB_TILDE | GLOB_ALPHASORT>.
+C<GLOB_BRACE() | GLOB_NOMAGIC() | GLOB_QUOTE() | GLOB_TILDE() | GLOB_ALPHASORT()>.
=back |
From @ap* Dave Mitchell <davem@iabyn.com> [2015-11-23 12:30]:
But that using the global -w flag, which is generally not used any more. Absent of it, as in most code nowadays, File::Glob would have to enable So most users will never see this warning. I think File::Glob does need changing.
If the warning actually got raised then that would tip users off to the Regards, |
From @ap* Ed Avis via RT <perlbug-followup@perl.org> [2015-11-23 12:50]:
I think we should teach people to turn on strict instead of making all |
From @epa
I 'use strict' in all my programs except for oneliners with perl -E. I agree that in general it is to be encouraged. But you are also right that if File::Glob printed a warning (and ideally one more beginner-friendly than "isn't numeric") then it wouldn't be necessary to add any of these defensive measures or gotcha warnings to the documentation. |
From @iabynOn Mon, Nov 23, 2015 at 02:52:14PM +0100, Aristotle Pagaltzis wrote:
The warning is checked in the caller's scope, since it's an XS sub: $ perl -MFile::Glob -e'use warnings; File::Glob::bsd_glob("nonexistent", GLOB_NOCHECK)' -- |
From @ap* Dave Mitchell <davem@iabyn.com> [2015-11-23 15:55]:
Oh! So this will only catch out people who turn on *neither* strictures Let’s say I’m not inclined to consider File::Glob the problem there. |
From @epaThe only time I have run File::Glob with neither strictures or warnings is when writing oneliners to reproduce this 'bug'... so it is all a bit self-referential. Still, surely you agree that a warning message like The flags argument to bsd_glob has been passed as the string 'GLOB_NOCASE' but it should be a numeric value. Perhaps you need to import the named constants, or consider running with 'use strict'? would be an improvement on the current "isn't numeric" warning? If I write a patch to make the warning message more idiot-proof, will you consider applying it? |
From @ap* Ed Avis via RT <perlbug-followup@perl.org> [2015-11-23 16:20]:
I do. (Well aside from the fact that that is too long for a warning.)
I wouldn’t feel qualified to apply an XS patch, myself, so Dave or co |
From @epaMy plan was to rename the current XS routine to _bsd_glob_impl or something like that, and have bsd_glob be a small Perl wrapper to it that sanity checks the arguments. Unless someone more familiar with XS and/or File::Glob thinks that is a bad idea. |
From @iabynOn Mon, Nov 23, 2015 at 07:46:37AM -0800, Ed Avis via RT wrote:
I personally really don't see the point in making bsd_glob check whether flock $fh, LOCK_SH; Should we be fixing up all them too? -- |
From @epaThere is a small difference in that flock() will fail with 'invalid argument' if passed a duff file locking mode; bsd_glob() treats the string as a valid set of flags (all zero) and continues. |
From @demerphqOn 23 November 2015 at 18:54, Ed Avis via RT <perlbug-followup@perl.org> wrote:
IIRC we have had other cases where this was also true and we fixed them. Yves -- |
From @iabynOn Mon, Nov 23, 2015 at 07:20:44PM +0100, demerphq wrote:
The difference between flock() and bsd_glob() is that for the former, zero Another example of a function using symbolic constants with zero as a $ perl -we'sysopen my $fh, "/dev/null", O_RDONLY; print "ok\n"' -- |
From @epaFWIW, I would favour making sysopen warn more explicitly about 'the flags argument was passed as a string; import the constants; and please use strict'. And print that warning whether or not the general numification warning is enabled with -w etc. With this kind of stupid programming error you really can't be too explicit or too helpful in the diagnostics. But I take the general point: if sysopen and friends don't have a separate check for flags passed as strings, it may be too much to demand that bsd_glob should have one. If I were maintaining the module on CPAN I would add the warning anyway; better to have it than not to have it IMHO. But as it is I must defer to the maintainer's opinion. |
From @epaI think the decision in this bug was not to change the code, but could the documentation patch in <https://rt-archive.perl.org/perl5/Ticket/Attachment/1376745/739203/> be applied please? |
From @iabynOn Mon, Apr 04, 2016 at 07:49:12AM -0700, Ed Avis via RT wrote:
I think we decided that replacing all the constants FOO in the docs -- |
From @epaDave Mitchell <davem <at> iabyn.com> writes:
OK, could you apply the rest of the patch please? (FWIW, the default behaviour for oneliners is indeed to run without both -- |
From @iabynOn Tue, Apr 05, 2016 at 11:42:03AM +0000, Ed Avis wrote:
Could you provide an updated patch?
My preference would be to leave -e as-is. -- |
From @epa--- a/ext/File-Glob/Glob.pm This function, which is included in the two export tags listed above, =head3 C<:nocase> and C<:case> @@ -196,7 +200,9 @@ uses this internally. =head2 POSIX FLAGS -The POSIX defined flags for bsd_glob() are: =over 4 |
From @ap* Ed Avis via RT <perlbug-followup@perl.org> [2016-04-05 15:27]:
Do File::Glob’s docs really have to explain that you can’t use something
You mean if the subs stricture is not turned on? Whether or not you Regards, |
From @epaThanks Aristotle P., here's a corrected patch: --- a/ext/File-Glob/Glob.pm This function, which is included in the two export tags listed above, =head3 C<:nocase> and C<:case> <at> <at> -196,7 +200,9 <at> <at> uses this internally. =head2 POSIX FLAGS -The POSIX defined flags for bsd_glob() are: =over 4 |
From @iabynOn Tue, Apr 05, 2016 at 08:24:50PM +0000, Ed Avis wrote:
Thanks, applied as v5.25.1-185-g8fe4bbc -- |
@iabyn - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#126239 (status was 'resolved')
Searchable as RT126239$
The text was updated successfully, but these errors were encountered: