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

File::Glob GLOB_NOCHECK documentation unclear #14954

Closed
p5pRT opened this issue Oct 1, 2015 · 28 comments
Closed

File::Glob GLOB_NOCHECK documentation unclear #14954

p5pRT opened this issue Oct 1, 2015 · 28 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 1, 2015

Migrated from rt.perl.org#126239 (status was 'resolved')

Searchable as RT126239$

@p5pRT
Copy link
Author

p5pRT commented Oct 1, 2015

From @epa

Created by @epa

The documentation for File​::Glob says

  "GLOB_NOCHECK"
  If the pattern does not match any pathname, then bsd_glob()
  returns a list consisting of only the pattern. If
  "GLOB_QUOTE" is set, its effect is present in the pattern
  returned.

I understood this to mean​: if you set the GLOB_NOCHECK flag, and the
pattern does not match any pathname, then bsd_glob() returns a list of
only the pattern. By implication, this means that if GLOB_NOCHECK is
not set, and the pattern does not match, then bsd_glob() returns the
empty list.

However, the behaviour looks like the opposite​:

% perl -MFile​::Glob -E '@​g = File​::Glob​::bsd_glob("nonexistent", GLOB_NOCHECK); say foreach @​g'
% perl -MFile​::Glob -E '@​g = File​::Glob​::bsd_glob("nonexistent"); say foreach @​g'
nonexistent

If GLOB_NOCHECK is set then you get an empty result from a
non-matching pattern. The default, if that flag is not set, is to
pass the pattern back if it fails to match.

This isn't consistent with the documentation - and it appears
inconsistent with POSIX glob(3), which returns the original pattern if
GLOB_NOCHECK is set and GLOB_NOMATCH otherwise.

I don't know whether the best way forward is to fix the documentation
to match what File​::Glob does (and note the inconsistency with POSIX)
or to fix the behaviour to match the docs (and POSIX).

Perl Info

Flags:
    category=library
    severity=low
    module=File::Glob

Site configuration information for perl 5.20.3:

Configured by Red Hat, Inc. at Mon Sep 14 07:37:15 UTC 2015.

Summary of my perl5 (revision 5 version 20 subversion 3) configuration:
   
  Platform:
    osname=linux, osvers=4.1.6-100.fc21.x86_64, archname=x86_64-linux-thread-multi
    uname='linux buildhw-02.phx2.fedoraproject.org 4.1.6-100.fc21.x86_64 #1 smp mon aug 17 22:20:37 utc 2015 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Doptimize=-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -Dccdlflags=-Wl,--enable-new-dtags -Dlddlflags=-shared -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -Wl,-z,relro  -Dshrpdir=/usr/lib64 -DDEBUGGING=-g -Dversion=5.20.3 -Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red Hat, Inc. -Dprefix=/usr -Dvendorprefix=/usr -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl5 -Dsitearch=/usr/local/lib64/perl5 -Dprivlib=/usr/share/perl5 -Dvendorlib=/usr/share/perl5/vendor_perl -Darchlib=/usr/lib64/perl5 -Dvendorarch=/usr/lib64/perl5/vendor_perl -Darchname=x86_64-linux-thread-multi -Dlibpth=/usr/local/lib64 /lib64 /usr/lib64 -Duseshrplib -Dusethreads -Duseithreads -Dusedtrace=/usr/bin/dtrace -Duselargefiles -Dd_semctl_semun -Di_db -Ui_ndbm -Di_gdbm -Di_shadow -Di_syslog -Dman3ext=3pm -Duseperlio -Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly -Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhostent_r_proto -Ud_sethostent_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto -Ud_endservent_r_proto -Ud_setservent_r_proto -Dscriptdir=/usr/bin -Dusesitecustomize'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='5.1.1 20150618 (Red Hat 5.1.1-4)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib64 /lib64 /usr/lib64 /usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib
    libs=-lpthread -lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lpthread -lresolv -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.21.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.21'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,--enable-new-dtags'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic -Wl,-z,relro  -L/usr/local/lib'

Locally applied patches:
    Fedora Patch1: Removes date check, Fedora/RHEL specific
    Fedora Patch3: support for libdir64
    Fedora Patch4: use libresolv instead of libbind
    Fedora Patch5: USE_MM_LD_RUN_PATH
    Fedora Patch6: Skip hostname tests, due to builders not being network capable
    Fedora Patch7: Dont run one io test due to random builder failures
    Fedora Patch15: Define SONAME for libperl.so
    Fedora Patch16: Install libperl.so to -Dshrpdir value
    Fedora Patch22: Document Math::BigInt::CalcEmu requires Math::BigInt (CPAN RT#85015)
    Fedora Patch25: Use stronger algorithm needed for FIPS in t/op/crypt.t (RT#121591)
    Fedora Patch26: Make *DBM_File desctructors thread-safe (RT#61912)
    Fedora Patch27: Report inaccesible file on failed require (RT#123270)
    Fedora Patch28: Use stronger algorithm needed for FIPS in t/op/taint.t (RT#123338)
    Fedora Patch200: Link XS modules to libperl.so with EU::CBuilder on Linux
    Fedora Patch201: Link XS modules to libperl.so with EU::MM on Linux


@INC for perl 5.20.3:
    /home/eda/share/perl5
    /home/eda/lib/perl5/
    /home/eda/lib64/perl5/
    /usr/local/lib64/perl5
    /usr/local/share/perl5
    /usr/lib64/perl5/vendor_perl
    /usr/share/perl5/vendor_perl
    /usr/lib64/perl5
    /usr/share/perl5
    .


Environment for perl 5.20.3:
    HOME=/home/eda
    LANG=en_GB.UTF-8
    LANGUAGE (unset)
    LC_COLLATE=C
    LC_CTYPE=en_GB.UTF-8
    LC_MESSAGES=en_GB.UTF-8
    LC_MONETARY=en_GB.UTF-8
    LC_NUMERIC=en_GB.UTF-8
    LC_TIME=en_GB.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/eda/bin:/home/eda/bin:/usr/local/bin:/usr/bin:/sbin:/usr/sbin:/sbin:/usr/sbin
    PERL5LIB=/home/eda/share/perl5:/home/eda/lib/perl5/:/home/eda/lib64/perl5/
    PERL_BADLANG (unset)
    SHELL=/bin/bash

-- 
Ed Avis <eda@waniasset.com>
Please ignore autogenerated disclaimer below this point.


This email is intended only for the person to whom it is addressed and may contain confidential information. Any retransmission, copying, disclosure or other use of, this information by persons other than the intended recipient is prohibited. If you received this email in error, please contact the sender and delete the material. This email is for information only and is not intended as an offer or solicitation for the purchase or sale of any financial instrument. Wadhwani Asset Management LLP is a Limited Liability Partnership registered in England (OC303168) with registered office at 40 Berkeley Square, 3rd Floor, London, W1J 5AL. It is authorised and regulated by the Financial Conduct Authority.

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2015

From @iabyn

On Thu, Oct 01, 2015 at 09​:22​:44AM -0700, Ed Avis wrote​:

# New Ticket Created by "Ed Avis"
# Please include the string​: [perl #126239]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=126239 >

This is a bug report for perl from eda@​waniasset.com,
generated with the help of perlbug 1.40 running under perl 5.20.3.

-----------------------------------------------------------------
[Please describe your issue here]

The documentation for File​::Glob says

   "GLOB\_NOCHECK"
       If the pattern does not match any pathname\, then bsd\_glob\(\)
       returns a list consisting of only the pattern\.  If
       "GLOB\_QUOTE" is set\, its effect is present in the pattern
       returned\.

I understood this to mean​: if you set the GLOB_NOCHECK flag, and the
pattern does not match any pathname, then bsd_glob() returns a list of
only the pattern. By implication, this means that if GLOB_NOCHECK is
not set, and the pattern does not match, then bsd_glob() returns the
empty list.

However, the behaviour looks like the opposite​:

% perl -MFile​::Glob -E '@​g = File​::Glob​::bsd_glob("nonexistent", GLOB_NOCHECK); say foreach @​g'
% perl -MFile​::Glob -E '@​g = File​::Glob​::bsd_glob("nonexistent"); say foreach @​g'
nonexistent

It's actually doing the right thing; it's a combination of two things that's
causing your example to go astray. First, GLOB_NOCHECK isn't exported by
default, so you're actually using the value '0'; second, in the absence of
a second arg, the flags of bsd_glob() doesn't default to zero, but rather
to​:

  If the second argument is omitted, C<GLOB_CSH> (or C<GLOB_CSH|GLOB_NOCASE>
  on VMS and DOSish systems) is used by default.

which happens to include GLOB_NOMAGIC, which is nearly equivalent to
GLOB_NOCHECK.

This code​:

use File​::Glob qw(bsd_glob GLOB_NOCHECK GLOB_NOMAGIC);
my @​g;
for my $f ("nonexistent", "nonexstent*") {
  @​g = bsd_glob($f); print "bsd_glob($f)​: [@​g]\n";
  @​g = bsd_glob($f, 0); print "bsd_glob($f,0)​: [@​g]\n";
  @​g = bsd_glob($f, GLOB_NOCHECK); print "bsd_glob($f,GLOB_NOCHECK)​: [@​g]\n";
  @​g = bsd_glob($f, GLOB_NOMAGIC); print "bsd_glob($f,GLOB_NOMAGIC)​: [@​g]\n";
}

produces this output​:

  bsd_glob(nonexistent)​: [nonexistent]
  bsd_glob(nonexistent,0)​: []
  bsd_glob(nonexistent,GLOB_NOCHECK)​: [nonexistent]
  bsd_glob(nonexistent,GLOB_NOMAGIC)​: [nonexistent]
  bsd_glob(nonexstent*)​: []
  bsd_glob(nonexstent*,0)​: []
  bsd_glob(nonexstent*,GLOB_NOCHECK)​: [nonexstent*]
  bsd_glob(nonexstent*,GLOB_NOMAGIC)​: []

which I think is the correct behaviour.

--
More than any other time in history, mankind faces a crossroads. One path
leads to despair and utter hopelessness. The other, to total extinction.
Let us pray we have the wisdom to choose correctly.
  -- Woody Allen

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2015

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Nov 19, 2015

From @epa

Thanks for spotting the problems with my example code.

First, GLOB_NOCHECK isn't exported by default, so you're actually using the
value '0';

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
  The POSIX defined flags for bsd_glob() are​:

  "GLOB_ERR"
  Force bsd_glob() to return an error...

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'
GLOB_N_CKECK

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?

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @iabyn

On Thu, Nov 19, 2015 at 09​:08​:37AM -0800, Ed Avis via RT wrote​:

Thanks for spotting the problems with my example code.

First, GLOB_NOCHECK isn't exported by default, so you're actually using the
value '0';

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
The POSIX defined flags for bsd_glob() are​:

   "GLOB\_ERR"
       Force bsd\_glob\(\) to return an error\.\.\.

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?

But it already does​:

  $ perl -MFile​::Glob -wE 'File​::Glob​::bsd_glob("foo", GLOB_NOCHECK);'
  Argument "GLOB_NOCHECK" isn't numeric in subroutine entry at -e line 1.
  $

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'
GLOB_N_CKECK

I don't suggest File​::Glob should attempt to handle that; it should just
warn that the unknown string will be treated as zero.

It already does.

So I don't think anything needs changing.

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?

Yes, that would be fine.

--
"Strange women lying in ponds distributing swords is no basis for a system
of government. Supreme executive power derives from a mandate from the
masses, not from some farcical aquatic ceremony."
  -- Dennis, "Monty Python and the Holy Grail"

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @epa

I 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 Patch
diff --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

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @ap

* Dave Mitchell <davem@​iabyn.com> [2015-11-23 12​:30]​:

On Thu, Nov 19, 2015 at 09​:08​:37AM -0800, Ed Avis via RT wrote​:

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?

But it already does​:

$ perl \-MFile&#8203;::Glob \-wE 'File&#8203;::Glob&#8203;::bsd\_glob\("foo"\, GLOB\_NOCHECK\);'
Argument "GLOB\_NOCHECK" isn't numeric in subroutine entry at \-e line 1\.
$

So I don't think anything needs changing.

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
warnings for itself in order for it to actually warn about this issue.

So most users will never see this warning.

I think File​::Glob does need changing.

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?

Yes, that would be fine.

If the warning actually got raised then that would tip users off to the
problem without the docs having to get bloated.

Regards,
--
Aristotle Pagaltzis // <http​://plasmasturm.org/>

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @ap

* Ed Avis via RT <perlbug-followup@​perl.org> [2015-11-23 12​:50]​:

I 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?

I think we should teach people to turn on strict instead of making all
the code examples uglier.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @epa

I think we should teach people to turn on strict instead of making all
the code examples uglier.

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.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @iabyn

On Mon, Nov 23, 2015 at 02​:52​:14PM +0100, Aristotle Pagaltzis wrote​:

* Dave Mitchell <davem@​iabyn.com> [2015-11-23 12​:30]​:

On Thu, Nov 19, 2015 at 09​:08​:37AM -0800, Ed Avis via RT wrote​:

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?

But it already does​:

$ perl \-MFile&#8203;::Glob \-wE 'File&#8203;::Glob&#8203;::bsd\_glob\("foo"\, GLOB\_NOCHECK\);'
Argument "GLOB\_NOCHECK" isn't numeric in subroutine entry at \-e line 1\.
$

So I don't think anything needs changing.

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
warnings for itself in order for it to actually warn about this issue.

So most users will never see this warning.

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)'
Argument "GLOB_NOCHECK" isn't numeric in subroutine entry at -e line 1.
$

--
My Dad used to say 'always fight fire with fire', which is probably why
he got thrown out of the fire brigade.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @ap

* Dave Mitchell <davem@​iabyn.com> [2015-11-23 15​:55]​:

The warning is checked in the caller's scope, since it's an XS sub​:

Oh!

So this will only catch out people who turn on *neither* strictures
*nor* warnings.

Let’s say I’m not inclined to consider File​::Glob the problem there.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @epa

The 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?

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @ap

* Ed Avis via RT <perlbug-followup@​perl.org> [2015-11-23 16​:20]​:

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?

I do. (Well aside from the fact that that is too long for a warning.)
I’m still not sure it’s really necessary but I’m not opposed. It should
be terser, though, with the full explanation (if one is needed) under
DIAGNOSTICS in the POD. (So you get your wish for a doc patch in the
end! :-))

If I write a patch to make the warning message more idiot-proof, will
you consider applying it?

I wouldn’t feel qualified to apply an XS patch, myself, so Dave or co
will have to answer that.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @epa

My 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.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @iabyn

On Mon, Nov 23, 2015 at 07​:46​:37AM -0800, Ed Avis via RT wrote​:

My 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.

I personally really don't see the point in making bsd_glob check whether
its args are strings or not. You already get a warning, and this is no
different than any of the other places which use symbolic constants, e.g.

  flock $fh, LOCK_SH;

Should we be fixing up all them too?

--
Art is anything that has a label (especially if the label is "untitled 1")

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @epa

There 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.

@p5pRT
Copy link
Author

p5pRT commented Nov 23, 2015

From @demerphq

On 23 November 2015 at 18​:54, Ed Avis via RT <perlbug-followup@​perl.org> wrote​:

There 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.

IIRC we have had other cases where this was also true and we fixed them.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2015

From @iabyn

On Mon, Nov 23, 2015 at 07​:20​:44PM +0100, demerphq wrote​:

On 23 November 2015 at 18​:54, Ed Avis via RT <perlbug-followup@​perl.org> wrote​:

There 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.

IIRC we have had other cases where this was also true and we fixed them.

The difference between flock() and bsd_glob() is that for the former, zero
isn't a valid value for the flags arg, while it is for the latter.

Another example of a function using symbolic constants with zero as a
valid flag is sysopen​:

  $ perl -we'sysopen my $fh, "/dev/null", O_RDONLY; print "ok\n"'
  Argument "O_RDONLY" isn't numeric in sysopen at -e line 1.
  ok
  $

--
A power surge on the Bridge is rapidly and correctly diagnosed as a faulty
capacitor by the highly-trained and competent engineering staff.
  -- Things That Never Happen in "Star Trek" #9

@p5pRT
Copy link
Author

p5pRT commented Nov 24, 2015

From @epa

FWIW, 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.

@p5pRT
Copy link
Author

p5pRT commented Apr 4, 2016

From @epa

I 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?

@p5pRT
Copy link
Author

p5pRT commented Apr 5, 2016

From @iabyn

On Mon, Apr 04, 2016 at 07​:49​:12AM -0700, Ed Avis via RT wrote​:

I 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?

I think we decided that replacing all the constants FOO in the docs
with FOO() wasn't necessary, as only in the absence of *both* use strict
and use warnings would the code compile and run cleanly when using an
unimported constant.

--
That he said that that that that is is is debatable, is debatable.

@p5pRT
Copy link
Author

p5pRT commented Apr 5, 2016

From @epa

Dave Mitchell <davem <at> iabyn.com> writes​:

I think we decided that replacing all the constants FOO in the docs
with FOO() wasn't necessary, as only in the absence of *both* use strict
and use warnings would the code compile and run cleanly when using an
unimported constant.

OK, could you apply the rest of the patch please?

(FWIW, the default behaviour for oneliners is indeed to run without both
strict and warnings. Really, either that behaviour should change, or code
examples should be written taking the oneliner default environment into
account. My preference would be for warnings, but not strict, to be enabled
by default in oneliners.)

--
Ed Avis <eda@​waniasset.com>

@p5pRT
Copy link
Author

p5pRT commented Apr 5, 2016

From @iabyn

On Tue, Apr 05, 2016 at 11​:42​:03AM +0000, Ed Avis wrote​:

Dave Mitchell <davem <at> iabyn.com> writes​:

I think we decided that replacing all the constants FOO in the docs
with FOO() wasn't necessary, as only in the absence of *both* use strict
and use warnings would the code compile and run cleanly when using an
unimported constant.

OK, could you apply the rest of the patch please?

Could you provide an updated patch?

(FWIW, the default behaviour for oneliners is indeed to run without both
strict and warnings. Really, either that behaviour should change, or code
examples should be written taking the oneliner default environment into
account. My preference would be for warnings, but not strict, to be enabled
by default in oneliners.)

My preference would be to leave -e as-is.

--
You never really learn to swear until you learn to drive.

@p5pRT
Copy link
Author

p5pRT commented Apr 5, 2016

From @epa

--- a/ext/File-Glob/Glob.pm
+++ b/ext/File-Glob/Glob.pm
@​@​ -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

@p5pRT
Copy link
Author

p5pRT commented Apr 5, 2016

From @ap

* Ed Avis via RT <perlbug-followup@​perl.org> [2016-04-05 15​:27]​:

+Remember that to use the named constants for flags you must import
+them, for example with C<​:bsd_glob> described above.

Do File​::Glob’s docs really have to explain that you can’t use something
you haven’t imported…?

                                                  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.

You mean if the subs stricture is not turned on? Whether or not you
have warnings turned on doesn’t make any difference to how bareword
strings are treated.

Regards,
--
Aristotle Pagaltzis // <http​://plasmasturm.org/>

@p5pRT
Copy link
Author

p5pRT commented Apr 5, 2016

From @epa

Thanks Aristotle P., here's a corrected patch​:

--- a/ext/File-Glob/Glob.pm
+++ b/ext/File-Glob/Glob.pm
<at> <at> -176,10 +176,14 <at> <at> 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 C<use strict> is not in effect, then the constants will be
+treated as bareword strings, which won't do what you what.

=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​:
+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

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

From @iabyn

On Tue, Apr 05, 2016 at 08​:24​:50PM +0000, Ed Avis wrote​:

Thanks Aristotle P., here's a corrected patch​:

Thanks, applied as v5.25.1-185-g8fe4bbc

--
Art is anything that has a label (especially if the label is "untitled 1")

@p5pRT p5pRT closed this as completed Jun 17, 2016
@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2016

@iabyn - Status changed from 'open' to 'resolved'

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

No branches or pull requests

1 participant