Mailing List Archive: Perl: porters

[perl #119213] [PATCH] e915662 s/ajust/adjust/g

Index | Next | Previous | View Threaded

perlbug-followup at perl

Aug 10, 2013, 6:02 AM

Post #1 of 9 (22 views)
Permalink

[perl #119213] [PATCH] e915662 s/ajust/adjust/g

On Sat Aug 10 05:56:37 2013, jkeenan wrote:
> Same error spotted nearly simultaneously by two contributors. Am
> merging the second ticket into the first.

Patch applied in 0eccc83. I'm assuming that this has no impact outside
the file(s) touched in the patch. I'll keep the ticket open for 7 days
in case that assumption is incorrect.

Thanks to both contributors.

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213

perlbug-followup at perl

Aug 10, 2013, 9:28 AM

Post #2 of 9 (18 views)
Permalink

[perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

On Sat Aug 10 06:02:06 2013, jkeenan wrote:
> On Sat Aug 10 05:56:37 2013, jkeenan wrote:
> > Same error spotted nearly simultaneously by two contributors. Am
> > merging the second ticket into the first.
>
> Patch applied in 0eccc83. I'm assuming that this has no impact outside
> the file(s) touched in the patch. I'll keep the ticket open for 7 days
> in case that assumption is incorrect.

That assumption is correct, according to git-grep and grep.cpan.me.

I was going to tell you, ‘It’s easy: just see if it has “static” in its
declaration’, but this function is lacking that, which I find
surprising. So I need to ask the real C experts: Does that cause any
linking problems?

--

Father Chrysostomos

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213

zefram at fysh

Aug 10, 2013, 10:15 AM

Post #3 of 9 (17 views)
Permalink

Re: [perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

Father Chrysostomos via RT wrote:
>I was going to tell you, "It's easy: just see if it has "static" in its
>declaration", but this function is lacking that, which I find
>surprising. So I need to ask the real C experts: Does that cause any
>linking problems?

As the function is not referenced from any other translation unit,
and neither of its names is of any special form, it's pretty inert as
far as linking is concerned. However, it *should* be marked static.
The "S_" prefix is used in perl to mark functions with internal linkage
(which is what this use of "static" causes). Functions like this can
be declared in embed.fnc, causing generation of a macro so that the
function can be called without the "S_" prefix.

-zefram

perlbug-followup at perl

Aug 10, 2013, 2:15 PM

Post #4 of 9 (14 views)
Permalink

[perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

On Sat Aug 10 10:16:12 2013, zefram [at] fysh wrote:
> Father Chrysostomos via RT wrote:
> >I was going to tell you, "It's easy: just see if it has "static" in its
> >declaration", but this function is lacking that, which I find
> >surprising. So I need to ask the real C experts: Does that cause any
> >linking problems?
>
> As the function is not referenced from any other translation unit,
> and neither of its names is of any special form, it's pretty inert as
> far as linking is concerned. However, it *should* be marked static.
> The "S_" prefix is used in perl to mark functions with internal linkage
> (which is what this use of "static" causes). Functions like this can
> be declared in embed.fnc, causing generation of a macro so that the
> function can be called without the "S_" prefix.
>
> -zefram
>

Would it be possible to submit a patch for that?

Thank you very much.
Jim Keenan

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213

perlbug-followup at perl

Aug 10, 2013, 3:50 PM

Post #5 of 9 (13 views)
Permalink

[perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

On Sat Aug 10 14:15:06 2013, jkeenan wrote:
> On Sat Aug 10 10:16:12 2013, zefram [at] fysh wrote:
> >
> > As the function is not referenced from any other translation unit,
> > and neither of its names is of any special form, it's pretty inert as
> > far as linking is concerned. However, it *should* be marked static.
> > The "S_" prefix is used in perl to mark functions with internal linkage
> > (which is what this use of "static" causes). Functions like this can
> > be declared in embed.fnc, causing generation of a macro so that the
> > function can be called without the "S_" prefix.
> >
> > -zefram
> >
>
>
> Would it be possible to submit a patch for that?
>
> Thank you very much.
> Jim Keenan

Patch attachment attached. Is this what you meant?

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213

Attachments:

0001-add-adjust_size_and_find_bucket-to-embed.fnc.patch (3.27 KB)

perlbug-followup at perl

Aug 10, 2013, 4:41 PM

Post #6 of 9 (14 views)
Permalink

[perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

On Sat Aug 10 15:50:37 2013, mauke- wrote:
> On Sat Aug 10 14:15:06 2013, jkeenan wrote:
> >
> > Would it be possible to submit a patch for that?
> >
>
> Patch attachment attached. Is this what you meant?

Yes. This passes all tests for me on Dromedary (linux/x86_64) with a
plain configuration. But perhaps those with more C-fu than I should
stare at it a bit.

Thank you very much.
Jim Keenan

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213

public at khwilliamson

Aug 10, 2013, 6:00 PM

Post #7 of 9 (13 views)
Permalink

Re: [perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

On 08/10/2013 05:41 PM, James E Keenan via RT wrote:
> On Sat Aug 10 15:50:37 2013, mauke- wrote:
>> On Sat Aug 10 14:15:06 2013, jkeenan wrote:
>>>
>>> Would it be possible to submit a patch for that?
>>>
>>
>> Patch attachment attached. Is this what you meant?
>
> Yes. This passes all tests for me on Dromedary (linux/x86_64) with a
> plain configuration. But perhaps those with more C-fu than I should
> stare at it a bit.
>
> Thank you very much.
> Jim Keenan
>
> ---
> via perlbug: queue: perl5 status: open
> https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213
>

It looks fine to me, though I don't think I would have bothered to move
the initialization of nbytes until after the ASSERT.

I presume that the reason he did this was to take advantage of the
machine-generated ASSERT which asserts that nbytes_p is not NULL. Thus
if it is NULL, it will fail that assertion, rather than get a segfault
at the initialization of nbytes. I don't see much gain in in clarity
here, and, supposedly, declarations that also initialize are generally
faster than this way.

But, one could argue either way, and the patch looks perfectly valid to
me, so I think you should apply it.

perlbug-followup at perl

Aug 10, 2013, 9:16 PM

Post #8 of 9 (13 views)
Permalink

[perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

On Sat Aug 10 18:00:56 2013, public [at] khwilliamson wrote:
> On 08/10/2013 05:41 PM, James E Keenan via RT wrote:
> > On Sat Aug 10 15:50:37 2013, mauke- wrote:
> >> On Sat Aug 10 14:15:06 2013, jkeenan wrote:
> >>>
> >>> Would it be possible to submit a patch for that?
> >>>
> >>
> >> Patch attachment attached. Is this what you meant?
> >
> > Yes. This passes all tests for me on Dromedary (linux/x86_64) with a
> > plain configuration. But perhaps those with more C-fu than I should
> > stare at it a bit.
> >
> > Thank you very much.
> > Jim Keenan
> >
> > ---
> > via perlbug: queue: perl5 status: open
> > https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213
> >
>
> It looks fine to me, though I don't think I would have bothered to move
> the initialization of nbytes until after the ASSERT.
>
> I presume that the reason he did this was to take advantage of the
> machine-generated ASSERT which asserts that nbytes_p is not NULL. Thus
> if it is NULL, it will fail that assertion, rather than get a segfault
> at the initialization of nbytes. I don't see much gain in in clarity
> here, and, supposedly, declarations that also initialize are generally
> faster than this way.
>
> But, one could argue either way, and the patch looks perfectly valid to
> me, so I think you should apply it.
>

Thanks for the review. I have applied the patch in commit
bbb3b2d37817afff967fe98df398ed218dde5410 .

Since we have more than addressed the original subject of this RT, I am
marking it resolved. Please open a new RT for any new problems.

Thank you very much.
Jim Keenan

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=119213

ikegami at adaelis

Aug 11, 2013, 1:09 PM

Post #9 of 9 (13 views)
Permalink

Re: [perl #119213] [PATCH] e915662 s/ajust/adjust/g [In reply to]

On Sun, Aug 11, 2013 at 12:16 AM, James E Keenan via RT <
perlbug-followup [at] perl> wrote:

> Thanks for the review. I have applied the patch in commit
> bbb3b2d37817afff967fe98df398ed218dde5410 .
>

Or rather, 03f05cb48515d5cc33c90b97a28e7b8111da5d32

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads