[perl #71852] Warning "/* within comment" in Win32 config.h-related files

[kmx]

# New Ticket Created by kmx
# Please include the string: [perl #71852]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=71852 >

This is a bug report for perl from k...@volny.cz,
generated with the help of perlbug 1.39 running under perl 5.11.3.

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

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

[Please do not change anything below this line]
-----------------------------------------------------------------
---
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)

[Zefram]

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

[H.Merijn Brand]

On Tue, 05 Jan 2010 04:58:52 -0800, kmx (via RT)
<perlbug-...@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.

--
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/

[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

[Nicholas Clark]

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

[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

[Steve 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.

[Sisyphus]

----- Original Message -----
From: "kmx" <k...@volny.cz>
To: <perlbug-...@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

[Andy Dougherty]

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 doug...@lafayette.edu

[Craig A. Berry]

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.

[Andy Dougherty]

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 doug...@lafayette.edu

[Kmx]

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

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

[Steve Hay]

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.

[Steve Hay]

2010/1/8 Steve Hay <Stev...@planit.com>:

I've now applied your patch and regenerated the canned config files in
f3b9bb7d1f91ecfed13b90d5e1e22678c754e87b.