Declaration needs to be at top of block #19384
Conversation
inline.h
Outdated
| else | ||
| return NULL; | ||
| { | ||
| const char *file = CopFILE(cop); |
There was a problem hiding this comment.
IMO it would make more sense to just move up const char *file = CopFILE(cop); to come before PERL_ARGS_ASSERT_COP_FILE_AVN;
There was a problem hiding this comment.
Done. When I now build with gcc-12, I can install Sereal::Decoder via cpan without build failures. Please review.
|
Fundamentally this is a policy decision. Are we ok with C99 leaking out in our headers or not? Having done a little grepping I think we should not leak it for now. Most of the issue is with dists using Module::Install:XSUtil, it's a gift that keeps on giving… |
|
Perl now requires a C compiler that supports mixed declarations and code, upstream configures the compiler to not support that. I think it should be fixed upstream. WRT this patch, moving the declaration up means cop is dereferenced before it's asserted on, which is a minor regression (I expect there's similar problems in many other places though.) |
On second thought, all the other dists only enable it as a warning. If Serial is the only thing declaring it an error, then we don't have much of an urgent issue. |
|
On Mon, 31 Jan 2022, 07:18 Leon Timmermans, ***@***.***> wrote:
Perl now requires a C compiler that supports mixed declarations and code,
upstream configures the compiler to not support that.
I think it should be fixed upstream.
I agree we should, but I'm not sure 4 months until 3.36 is enough time for
that.
I don't mind changing this, but it feels ungenerous to penalize folks like
us who tried to keep our code clean and compliant with the old policy and
make sure our code compiles on as wide a range of compilers as possible.
Why can't the policy be that *headers* have to stay old policy compliant?
The rest of the internals can follow the new policy.
I'd say it's fair to say this is a regression. Sereal was fine until Perl
changed its policy.
Yves
… |
|
On 1/30/22 17:15, Tony Cook wrote:
Perl now requires a C compiler that supports mixed declarations and
code, upstream configures the compiler to not support that.
I think it should be fixed upstream.
WRT this patch, moving the declaration up means cop is dereferenced
before it's asserted on, which is a minor regression (I expect there's
similar problems in many other places though.)
Would my first commit -- which merely created a block in which the
declaration was the first statement -- be desirable by itself?
|
|
On Mon, 31 Jan 2022, 08:44 James E Keenan, ***@***.***> wrote:
On 1/30/22 17:15, Tony Cook wrote:
> Perl now requires a C compiler that supports mixed declarations and
> code, upstream configures the compiler to not support that.
>
> I think it should be fixed upstream.
>
> WRT this patch, moving the declaration up means cop is dereferenced
> before it's asserted on, which is a minor regression (I expect there's
> similar problems in many other places though.)
>
Would my first commit -- which merely created a block in which the
declaration was the first statement -- be desirable by itself?
I'd just split the statement. Decl goes up top, assignment after the macro.
Yves
|
I don't see the point in that. If perl was built using a C99 compiler, that means a C99 compiler can also be used for building modules. |
|
On Mon, 31 Jan 2022, 12:10 xenu, ***@***.***> wrote:
Why can't the policy be that *headers* have to stay old policy compliant?
The rest of the internals can follow the new policy.
I don't see the point in that. If perl was built using a C99 compiler that
means a C99 compiler can also be used for building modules.
That is besides the point. Making headers c89 compliant (or whatever) means
modules like Sereal who took cross compilation seriously for years by
enforcing the gcc flag mentioned in this thread do not die like Sereal has
here, and it wouldn't inconvenience anybody who wants to build without the
flag either. The only affected parties would be a small group of perl
internals devs who touch the headers. It wouldn't even affect .c files in
core just .h files.
Yves
|
|
I don't think it's reasonable to ban C99 (a 23 years old standard!) from headers forever because a very small number of modules uses compiler flags that ban them. If a module author really needs to keep supporting hypothetical users stuck with ancient compilers, those flags should be either changed to warnings or restricted to a CI environment. |
|
For all CPAN it's only Sereal which faults this as an error, and Sereal/Sereal#265 was submitted last year to fix this. The other distributions which made it an error are updated. The rest are all warnings. |
|
On Mon, 31 Jan 2022 at 07:42, Nicholas Clark ***@***.***> wrote:
For *all* CPAN it's *only* Sereal which faults this as an error, and
Sereal/Sereal#265 <Sereal/Sereal#265> was
submitted last year to fix this. The other distributions which made it an
error are updated. The rest are all warnings.
I already said I would make the change. Thanks for the PR, I had not
noticed that.
I'll roll a new Sereal release tomorrow. I have other priorities today.
Yves
|
|
On Mon, 31 Jan 2022 at 08:13, demerphq ***@***.***> wrote:
On Mon, 31 Jan 2022 at 07:42, Nicholas Clark ***@***.***>
wrote:
> For *all* CPAN it's *only* Sereal which faults this as an error, and
> Sereal/Sereal#265 <Sereal/Sereal#265> was
> submitted last year to fix this. The other distributions which made it an
> error are updated. The rest are all warnings.
>
I already said I would make the change. Thanks for the PR, I had not
noticed that.
I'll roll a new Sereal release tomorrow. I have other priorities today.
Well, it's a week late, but I just pushed to PAUSE now. 4.019, with zstd
updates to 1.5.1
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
@tonycoz, can I ask you to make an executive decision on this p.r.? What, if anything, should be committed to blead? Thank you very much. |
I'm not Tony, but: blead is fine as is, nothing here should be committed. The only module for which the current code caused a problem has had a new version released so that it no longer causes a problem. I'd recommend withdrawing this PR. |
For: #19382