-
Notifications
You must be signed in to change notification settings - Fork 560
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
Spurious "Assuming NOT a POSIX class" warning #16001
Comments
From @maukeCreated by @maukeThis is actually a regression. Bug #127581 was reported against 5.23.8 and $ perl -we 'qr/[][[:alpha:]\@\\^_?]/' This should not throw any warnings. NB: When this was patched, a regression test was added to t/re/reg_mesg.t. But - It was added to @death, not @warning, so it actually tests that the pattern Perl Info
|
From @maukeOn Tue, 06 Jun 2017 13:52:52 -0700, mauke- wrote:
I bisected it to commit 7eec73e move warning text to RExC_state (via RExC_warn_text) This way we reuse the same AV each time, and avoid various refcount bookkeeping issues, all at a relatively modest cost (IMO) |
From @khwilliamsonOn 6/6/2017 4:20 PM, l.mai@web.de via RT wrote:
Yves, What was the motivation of this commit. I don't understand from your |
The RT System itself - Status changed from 'new' to 'open' |
From @cpansproutOn Tue, 13 Jun 2017 11:16:40 -0700, public@khwilliamson.com wrote:
I seem to remember that the purpose was to avoid having to free the via the savestack AV in various code paths, which was done before to prevent fatal warnings from causing a memory leak. The commit was supposed to accomplish the same thing in a different way that was easier to maintain, with a negligible memory trade-off. -- Father Chrysostomos |
From @khwilliamsonOn 06/13/2017 04:47 PM, Father Chrysostomos via RT wrote:
I don't know anything about the savestack. The code this commit changed |
From @cpansproutOn Wed, 14 Jun 2017 10:44:37 -0700, public@khwilliamson.com wrote:
Sorry for taking so long to respond. I meant the temps stack, not the savestack (though the same thing can be done with the savestack via SAVEFREESV).
It seems I misremember and confused it with another commit (probably accb436). Again, apologies for the confusion. The details for the commits leading to *this* bug (ee072c8 and 7eec73e) can be found in ticket #128313. -- Father Chrysostomos |
From @cpansproutOn Sat, 17 Jun 2017 07:28:38 -0700, sprout wrote:
I don’t see anything wrong with the code in 7eec73e (which moved a C auto, warn_text, into the RExC_state_t struct, to avoid the possibility of too many mortal AVs), but I do not fully understand the scope of the new location. I suspect some internal temporary warning state is being allowed to last too long. I.e., RExC_warn_text is not being cleared at the right point. But I am only guessing. -- Father Chrysostomos |
From @demerphqI missed this mail. Next time please add me to cc explicitly. I will On 6 Jun 2017 18:20, "l.mai@web.de via RT" <perlbug-followup@perl.org> On Tue, 06 Jun 2017 13:52:52 -0700, mauke- wrote:
I bisected it to commit 7eec73e move warning text to RExC_state (via RExC_warn_text) This way we reuse the same AV each time, and avoid various refcount via perlbug: queue: perl5 status: new |
From @khwilliamsonOn 06/17/2017 08:28 AM, Father Chrysostomos via RT wrote:
Hmm. I was involved in that discussion, but forgot all about it. Now The commit breaks the algorithm. The algorithm is to create tentative I have several possible simple fixes; I was just trying to really grok |
From @cpansproutOn Sun, 18 Jun 2017 09:18:50 -0700, public@khwilliamson.com wrote:
The commit message for ee072c8 says leak, and it did fix a leak my making the AV mortal earlier in a function that could exit early. The commit message for 7eec73e (the problematic) commit does not say leak. It is the one can came up with a different approach (which is faulty, as you explained), in order to avoid using the temps stack. As Dan Collins pointed out, ee072c8 did not actually increase the number of mortals, but changed when things were made mortal. So simply reverting 7eec73e may be the solution.
-- Father Chrysostomos |
From @demerphqOn 18 June 2017 at 19:32, Father Chrysostomos via RT
I guess it depends on your definition of faulty.
IIRC still problematically.
I don't think that is the right thing to do. 7eec73e makes sure we
I have a patch to fix this already, I just haven't had time to push Yves -- |
From @demerphqOn 18 June 2017 at 20:43, demerphq <demerphq@gmail.com> wrote:
Fixed with: commit d730a80 add test for [perl #131522] and fix test for (related) [perl #127581] commit bab0f8e Resolve Perl #131522: Spurious "Assuming NOT a POSIX class" warning yves -- |
From @demerphqOn 18 June 2017 at 18:18, Karl Williamson <public@khwilliamson.com> wrote:
This thread has gotten a bit fragmented. To recap from FC:
The problematic point of of 7eec73e was that I misunderstood the The patch bab0f8e fixes this and IIRC prior to 7eec7 we would create a new AV each parse pass per POSIX cheers, -- |
From @steve-m-hayNow fixed in 5.26.1. |
@steve-m-hay - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#131522 (status was 'resolved')
Searchable as RT131522$
The text was updated successfully, but these errors were encountered: