Warning "/* within comment" in Win32 config.h-related files

[p5pRT]

Migrated from rt.perl.org#71852 (status was 'open')

Searchable as RT71852$

[p5pRT]

From @kmx

Created by @kmx

Hi,

I would like to propose a cosmetic fix to some "config.h-related" files
in perl core sources mostly regarding Win32 platform.

### What is the problem?

The issue occurs when some module with XS part tries to '#include
"perl.h"' (all XS do that) and sets compiler option '-Wall' in its
CCFLAGS then you will experience the following warnings​:

C​:/strawberry/perl/lib/CORE/config.h​:39​:20​: warning​: "/*" within comment
C​:/strawberry/perl/lib/CORE/config.h​:45​:21​: warning​: "/*" within comment
+ many many similar

### Why does it happen?

The reason is that in config.h there are blocks like this​:

/* HAS_FORK​:
* This symbol, if defined, indicates that the fork routine is
* available.
*/
/*#define HAS_FORK /**/

And as you can see the line with "#define HAS_FORK" obviously has some
extra '/*' causing the warnings.

### How to fix it?

The proper way of handling this should be using​:
/*#define HAS_FORK / **/
instead of
/*#define HAS_FORK /**/

### Where does it occurs?

./win32/config_H.bc
./win32/config_H.ce
./win32/config_H.gc
./win32/config_H.gc64
./win32/config_H.gc64nox
./win32/config_H.vc
./win32/config_H.vc64
./win32/win32.h
./NetWare/config_H.wc
./plan9/config.plan9
./plan9/config_h.sample
./plan9/plan9ish.h

Could you please consider cleaning up the header files mentioned above?
It does not harm any existing functionality.

Considering the number of incorrect lines (approx. 3000 in total in all
files) it would probably need some automated processing.

Thanks.

--
kmx

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.11.3:

Configured by miko at Tue Jan  5 09:08:32 2010.

Summary of my perl5 (revision 5 version 11 subversion 3) configuration:
   
  Platform:
    osname=MSWin32, osvers=4.0, archname=MSWin32-x86-multi-thread
    uname=''
    config_args='undef'
    hint=recommended, useposix=true, d_sigaction=undef
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags =' -s -O2 -DWIN32 -DHAVE_DES_FCRYPT 
-DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS -fno-strict-aliasing
-mms-bitfields -DPERL_MSVCRT_READFIX',
    optimize='-s -O2',
    cppflags='-DWIN32'
    ccversion='', gccversion='4.4.3', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='long
long', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='g++', ldflags ='-s -L"c:\perl\lib\CORE" -L"C:\MinGW64\lib"'
    libpth=C:\MinGW64\lib c:\mingw64\i686-w64-mingw32\lib
    libs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32
-ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr
-lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    perllibs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool
-lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid
-lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    libc=, so=dll, useshrplib=true, libperl=libperl511.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-mdll -s -L"c:\perl\lib\CORE"
-L"C:\MinGW64\lib"'

Locally applied patches:
    


@INC for perl 5.11.3:
    c:/perl/site/lib
    c:/perl/lib
    .


Environment for perl 5.11.3:
    HOME=D:\profile
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
   
PATH=c:\perl\bin;c:\mingw64\bin;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\System32\Wbem
    PERL_BADLANG (unset)
    SHELL (unset)

[p5pRT]

@kmx - Status changed from 'new' to 'open'

[p5pRT]

From zefram@fysh.org

kmx wrote​:

The proper way of handling this should be using​:
/*#define HAS_FORK / **/

That would miss the point of having the "/**/" at the end of the line.
The point of that is precisely so that the directive's state can be
switched by the minimal edit of adding or removing "/*" at the start.
If the "/* ... /**/" scheme is not to be used, then the inactive state
of the line should be something like

  /* #define HAS_FORK */

and the active version might be either of

  #define HAS_FORK
  #define HAS_FORK /**/

Considering the number of incorrect lines

The lines are not incorrect. The definition of the C language is
perfectly clear that "/*" may appear within a comment. However, avoiding
such a commonly-provided warning is still a legitimate objective.

-zefram

[p5pRT]

From @Tux

On Tue, 05 Jan 2010 04​:58​:52 -0800, kmx (via RT)
<perlbug-followup@​perl.org> wrote​:

Hi,

I would like to propose a cosmetic fix to some "config.h-related" files
in perl core sources mostly regarding Win32 platform.

The code that is having these `problems' is generated code. Cleaning it
up by hand would be a plain waste of time, as the next round of
generation will replace it with the current state.

The generation code is not as simple as it it might look, so I don't
think I want to spend time into fixing something that isn't broken, but
merely issues warnings. I agree that these warnings are annoying.

Maybe your C-compiler has an option to suppress these specific warnings.

### What is the problem?

The issue occurs when some module with XS part tries to '#include
"perl.h"' (all XS do that) and sets compiler option '-Wall' in its
CCFLAGS then you will experience the following warnings​:

C​:/strawberry/perl/lib/CORE/config.h​:39​:20​: warning​: "/*" within comment
C​:/strawberry/perl/lib/CORE/config.h​:45​:21​: warning​: "/*" within comment
+ many many similar

### Why does it happen?

The reason is that in config.h there are blocks like this​:

/* HAS_FORK​:
* This symbol, if defined, indicates that the fork routine is
* available.
*/
/*#define HAS_FORK /**/

And as you can see the line with "#define HAS_FORK" obviously has some
extra '/*' causing the warnings.

### How to fix it?

The proper way of handling this should be using​:
/*#define HAS_FORK / **/
instead of
/*#define HAS_FORK /**/

### Where does it occurs?

./win32/config_H.bc
./win32/config_H.ce
./win32/config_H.gc
./win32/config_H.gc64
./win32/config_H.gc64nox
./win32/config_H.vc
./win32/config_H.vc64
./win32/win32.h
./NetWare/config_H.wc
./plan9/config.plan9
./plan9/config_h.sample
./plan9/plan9ish.h

Could you please consider cleaning up the header files mentioned above?
It does not harm any existing functionality.

Considering the number of incorrect lines (approx. 3000 in total in all
files) it would probably need some automated processing.

Thanks.

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, OpenSuSE 10.3, 11.0, and 11.1, AIX 5.2 and 5.3.
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

[p5pRT]

From @kmx

Hello,

Hi,

I would like to propose a cosmetic fix to some "config.h-related" files
in perl core sources mostly regarding Win32 platform.

The code that is having these `problems' is generated code. Cleaning it
up by hand would be a plain waste of time, as the next round of
generation will replace it with the current state.

Well, I am not sure that this is true for ./win32/config_H.* files which
are the main problem. At least quite recently (between 5.11.2/5.11.3)
added files related to newly introduced MINGW64 support were created
AFAIK by hand. But correct me if I am wrong.

The generation code is not as simple as it it might look, so I don't
think I want to spend time into fixing something that isn't broken, but
merely issues warnings. I agree that these warnings are annoying.

Maybe your C-compiler has an option to suppress these specific warnings.

Our (= strawbery perl) compiler is gcc that can surely suppress them but
keep in mind that it is not my decision what compiler flags the perl
module author has put into his/her Makefile.

Just to illustrate what 'annoying' means in this case - if you install
for example "cpan -fi autobox" on Win32 strawberry perl you got 1107 (in
words​: more than one thousand) warnings - IMHO it is more than just
annoying.

I understand that it has no priority as it is only cosmetics and affects
only those running perl on Win32 who are kind of
"non-core-perl-user-target-group". But I would really appreciate if this
can be fixed.

--
kmx

[p5pRT]

From @nwc10

On Wed, Jan 06, 2010 at 10​:09​:49AM +0100, kmx wrote​:

Hello,

Hi,

I would like to propose a cosmetic fix to some "config.h-related" files
in perl core sources mostly regarding Win32 platform.

The code that is having these `problems' is generated code. Cleaning it
up by hand would be a plain waste of time, as the next round of
generation will replace it with the current state.

Well, I am not sure that this is true for ./win32/config_H.* files which
are the main problem. At least quite recently (between 5.11.2/5.11.3)
added files related to newly introduced MINGW64 support were created
AFAIK by hand. But correct me if I am wrong.

oops. Merijn seems to have missed that you're referring to the Win32 files,
not the metaconfig generated files.

I understand that it has no priority as it is only cosmetics and affects
only those running perl on Win32 who are kind of
"non-core-perl-user-target-group". But I would really appreciate if this
can be fixed.

That Win32 is not core is a myth when it comes to the core perl. It may be
the attitude of some CPAN authors, but it's not the case in the core. Win32
is easy compared with VMS, and we're still working hard to keep VMS first
class too.

Please stop perpetuating untruths.

I *think* that the files in question are generated by win32/config_h.PL
I don't have access to a win32 machine to test things.

As you clearly care about this issue, and are in a position to verify that
a proposed fix would work, please could you consider sending a patch to that
file to cause the correct generation of the headers.

The repository is as http​://perl5.git.perl.org/perl.git

Nicholas Clark

[p5pRT]

From @kmx

Hi Nicholas,

I *think* that the files in question are generated by win32/config_h.PL
I don't have access to a win32 machine to test things.

As you clearly care about this issue, and are in a position to verify that
a proposed fix would work, please could you consider sending a patch to that
file to cause the correct generation of the headers.

The repository is as http​://perl5.git.perl.org/perl.git

Please find enclosed patch for config_h.PL that does​:
1) small change in code handling $0 (to make it work in Win32 strawberry
perl)
2) fix for preventing "/*" inside comments.

I have tried to recreate some win32/config_H.*
*) the comments are fine now
*) HOWEVER, they differ from those already placed in win32 directory

To sum up, I do not know who/when/how initiates regeneration of the files​:
./win32/config_H.bc
./win32/config_H.ce
./win32/config_H.gc
./win32/config_H.gc64
./win32/config_H.gc64nox
./win32/config_H.vc
./win32/config_H.vc64

It seems that it is not only by simply running config_h.PL - there seems
to be some additional manual patching (it is just my feeling, I might be
wrong).

--
kmx

[p5pRT]

From @kmx

win32_config_h.PL.patch

diff --git a/win32/config_h.PL b/win32/config_h.PL
index 531ddce..89b8553 100644
--- a/win32/config_h.PL
+++ b/win32/config_h.PL
@@ -8,7 +8,8 @@ BEGIN
  }
 use File::Compare qw(compare);
 use File::Copy qw(copy);
-my $name = $0;
+use File::Basename qw(fileparse);
+my ($name, $dir) = fileparse($0);
 $name =~ s#^(.*)\.PL$#../$1.SH#;
 my %opt;
 while (@ARGV && $ARGV[0] =~ /^([\w_]+)=(.*)$/)
@@ -62,6 +63,7 @@ while (<SH>)
   munge();
   s/\\\$/\$/g;
   s#/[ *\*]*\*/#/**/#;
+  s#(.)/\*\*/#$1/ **/# if(/^\/\*/); #avoid "/*" inside comments
   if (/^\s*#define\s+(PRIVLIB|SITELIB|VENDORLIB)_EXP/)
    {
      $_ = "#define ". $1 . "_EXP (win32_get_". lc($1) . "(PERL_VERSION_STRING, NULL))\t/**/\n";
@@ -69,7 +71,7 @@ while (<SH>)
   # incpush() handles archlibs, so disable them
   elsif (/^\s*#define\s+(ARCHLIB|SITEARCH|VENDORARCH)_EXP/)
    {
-     $_ = "/*#define ". $1 . "_EXP \"\"\t/**/\n";
+     $_ = "/*#define ". $1 . "_EXP \"\"\t/ **/\n";
    }
   print H;
  }

[p5pRT]

From @steve-m-hay

kmx wrote on 2010-01-06​:

Hi Nicholas,

I *think* that the files in question are generated by win32/config_h.PL
I don't have access to a win32 machine to test things.

As you clearly care about this issue, and are in a position to verify
that a proposed fix would work, please could you consider sending a
patch to that file to cause the correct generation of the headers.

The repository is as http​://perl5.git.perl.org/perl.git

Please find enclosed patch for config_h.PL that does​: 1) small change
in code handling $0 (to make it work in Win32 strawberry perl) 2) fix
for preventing "/*" inside comments.

But this patch "fixes" the problem by changing /**/ to / **/, which it has already been commented by others is undesirable as it breaks the convenience of the whole /* ... /**/ idea.

I have tried to recreate some win32/config_H.*
*) the comments are fine now
*) HOWEVER, they differ from those already placed in win32 directory

To sum up, I do not know who/when/how initiates regeneration of the
files​: ./win32/config_H.bc ./win32/config_H.ce ./win32/config_H.gc
./win32/config_H.gc64 ./win32/config_H.gc64nox ./win32/config_H.vc
./win32/config_H.vc64

It seems that it is not only by simply running config_h.PL - there seems
to be some additional manual patching (it is just my feeling, I might be
wrong).

I've regenerated these files several times in the past. There are some comments in the Win32 makefiles above the regen_config_h target about how to do it, but you're right that it does require some manual editing too and is quite fiddly to do.

[p5pRT]

From @sisyphus

----- Original Message -----
From​: "kmx" <kmx@​volny.cz>
To​: <perlbug-followup@​perl.org>
Sent​: Wednesday, January 06, 2010 8​:09 PM
Subject​: Re​: [perl #71852] Warning "/* within comment" in Win32
config.h-related files

The code that is having these `problems' is generated code. Cleaning it
up by hand would be a plain waste of time, as the next round of
generation will replace it with the current state.

Well, I am not sure that this is true for ./win32/config_H.* files which
are the main problem. At least quite recently (between 5.11.2/5.11.3)
added files related to newly introduced MINGW64 support were created
AFAIK by hand. But correct me if I am wrong.

I think that's correct (except I think it was between 5.11.1/5.11.2).
I know for a fact that config_H.gc64nox is just config_H.gc64 with a couple
of minor changes made by hand (because I did that myself). I believe
config_H.gc64, itself, was originally created by modifying config_H.gc by
han) - but I don't have first hand experience of the actual process used,
as it was done by someone else (Tor Lillqvist).

I think we could probably just amend those 2 files - I don't expect that
they will be overwritten by running config_sh.PL any time soon. Any problem
if I create patches for config_H.gc64 and config_H.6c64nox ? Any problem if
I do the same for config_H.gc, too ?

Cheers,
Rob

[p5pRT]

From @doughera88

On Thu, 7 Jan 2010, Steve Hay wrote​:

kmx wrote on 2010-01-06​:

Hi Nicholas,

I *think* that the files in question are generated by win32/config_h.PL
I don't have access to a win32 machine to test things.

As you clearly care about this issue, and are in a position to verify
that a proposed fix would work, please could you consider sending a
patch to that file to cause the correct generation of the headers.

The repository is as http​://perl5.git.perl.org/perl.git

Please find enclosed patch for config_h.PL that does​: 1) small change
in code handling $0 (to make it work in Win32 strawberry perl) 2) fix
for preventing "/*" inside comments.

But this patch "fixes" the problem by changing /**/ to / **/, which it
has already been commented by others is undesirable as it breaks the
convenience of the whole /* ... /**/ idea.

It's a question of balancing the editing convenience of that little trick
versus the annoying warnings put out by the compiler. From the Unix
Configure end, we changed to the "/ **/" style many years ago in order to
avoid the warnings. Very few people hand-edit the Configure-generated
config.h file, so it hasn't been a big burden. Hand-editing the windows
config.h versions is, perhaps, slightly more common. Still, on balance,
the change is probably worthwhile.

--
  Andy Dougherty doughera@​lafayette.edu

[p5pRT]

From @craigberry

On Thu, Jan 7, 2010 at 7​:50 AM, Andy Dougherty <doughera@​lafayette.edu> wrote​:

On Thu, 7 Jan 2010, Steve Hay wrote​:

kmx wrote on 2010-01-06​:

Hi Nicholas,

I *think* that the files in question are generated by win32/config_h.PL
I don't have access to a win32 machine to test things.

As you clearly care about this issue, and are in a position to verify
that a proposed fix would work, please could you consider sending a
patch to that file to cause the correct generation of the headers.

The repository is as http​://perl5.git.perl.org/perl.git

 Please find enclosed patch for config_h.PL that does​: 1) small change
in code handling $0 (to make it work in Win32 strawberry perl) 2) fix
for preventing "/*" inside comments.

But this patch "fixes" the problem by changing /**/ to / **/, which it
has already been commented by others is undesirable as it breaks the
convenience of the whole /* ... /**/ idea.

It's a question of balancing the editing convenience of that little trick
versus the annoying warnings put out by the compiler.  From the Unix
Configure end, we changed to the "/ **/" style many years ago in order to
avoid the warnings.  Very few people hand-edit the Configure-generated
config.h file, so it hasn't been a big burden.  Hand-editing the windows
config.h versions is, perhaps, slightly more common.  Still, on balance,
the change is probably worthwhile.

Wouldn't it also be an option to put

#ifdef __GNUC__
#pragma GCC diagnostic ignore -Wcomment
#endif

at some appropriate place? Probably better just to avoid the construct though.

[p5pRT]

From @doughera88

On Thu, 7 Jan 2010, Craig A. Berry wrote​:

Wouldn't it also be an option to put

#ifdef __GNUC__
#pragma GCC diagnostic ignored "-Wcomment"
#endif

[I've edited the above to what I think is the correct syntax.]

Sadly, with gcc-4.1, this now gives me *two* warnings​:

  try.c​:2​: warning​: ignoring #pragma GCC diagnostic
  try.c​:5​:16​: warning​: "/*" within comment

I don't know what version of gcc started supporting that construct, but it
would have to be wrapped in some additional #ifdef's, and maybe even have
a fallback strategy anyway. At which point, I come to the same conclusion
you did​:

Probably better just to avoid the construct though.

--
  Andy Dougherty doughera@​lafayette.edu
 

[p5pRT]

From @kmx

Dne 7.1.2010 14​:50, Andy Dougherty napsal(a)​:

On Thu, 7 Jan 2010, Steve Hay wrote​:

kmx wrote on 2010-01-06​:

Hi Nicholas,

I *think* that the files in question are generated by win32/config_h.PL
I don't have access to a win32 machine to test things.

As you clearly care about this issue, and are in a position to verify
that a proposed fix would work, please could you consider sending a
patch to that file to cause the correct generation of the headers.

The repository is as http​://perl5.git.perl.org/perl.git

Please find enclosed patch for config_h.PL that does​: 1) small change
in code handling $0 (to make it work in Win32 strawberry perl) 2) fix
for preventing "/*" inside comments.

But this patch "fixes" the problem by changing /**/ to / **/, which it
has already been commented by others is undesirable as it breaks the
convenience of the whole /* ... /**/ idea.

It's a question of balancing the editing convenience of that little trick
versus the annoying warnings put out by the compiler. From the Unix
Configure end, we changed to the "/ **/" style many years ago in order to
avoid the warnings. Very few people hand-edit the Configure-generated
config.h file, so it hasn't been a big burden. Hand-editing the windows
config.h versions is, perhaps, slightly more common. Still, on balance,
the change is probably worthwhile.

I agree with Andy. We have to compare​:

1) on one hand​: (un)convenience of config.h hand editing - deleting 2
chars in /**/ case vs. deleting 3 chars in / **/ case (which happens
quite rarely)

2) on the other hand​: more than one thousand warnings per each
installation of each module using "-Wall" per each Win32/gcc perl user
(which happens IMHO daily)

Considering the fact that "/ **/" style is already in place in other
parts of perl core sources I would definitely vote for using the same
style also for Win32.

--
kmx

[p5pRT]

From @steve-m-hay

kmx wrote on 2010-01-07​:

Dne 7.1.2010 14​:50, Andy Dougherty napsal(a)​:

On Thu, 7 Jan 2010, Steve Hay wrote​:

kmx wrote on 2010-01-06​:

Hi Nicholas,

I *think* that the files in question are generated by
win32/config_h.PL I don't have access to a win32 machine to test
things.

As you clearly care about this issue, and are in a position to
verify that a proposed fix would work, please could you consider
sending a patch to that file to cause the correct generation of
the
headers.

The repository is as http​://perl5.git.perl.org/perl.git

Please find enclosed patch for config_h.PL that does​: 1) small
change
in code handling $0 (to make it work in Win32 strawberry perl) 2)
fix
for preventing "/*" inside comments.

But this patch "fixes" the problem by changing /**/ to / **/, which
it
has already been commented by others is undesirable as it breaks the
convenience of the whole /* ... /**/ idea.

It's a question of balancing the editing convenience of that little
trick versus the annoying warnings put out by the compiler. From the
Unix Configure end, we changed to the "/ **/" style many years ago in
order to avoid the warnings. Very few people hand-edit the
Configure-
generated config.h file, so it hasn't been a big burden.
Hand-editing
the windows config.h versions is, perhaps, slightly more common.
Still, on balance, the change is probably worthwhile.

I agree with Andy. We have to compare​:

1) on one hand​: (un)convenience of config.h hand editing - deleting 2
chars in /**/ case vs. deleting 3 chars in / **/ case (which happens
quite rarely)

2) on the other hand​: more than one thousand warnings per each
installation of each module using "-Wall" per each Win32/gcc perl user
(which happens IMHO daily)

Considering the fact that "/ **/" style is already in place in other
parts of perl core sources I would definitely vote for using the same
style also for Win32.

Yes, okay, that sounds fair enough. I hadn't realized before Andy
pointed it out that the "/ **/" style was already being used elsewhere.

I'll try to apply your patch and update the canned config files in the
next couple of days if I get a chance and nobody else gets there first.