document/publicize THINKFIRST

[p5pRT]

Migrated from rt.perl.org#116925 (status was 'resolved')

Searchable as RT116925$

[p5pRT]

From @rjbs

A number of things broken by the new COW work were answered with, "By the way,
you need to use THINKFIRST there." This, in turn, was followed by, "What?
That's undocumented and how was I supposed to know that?" The third line in
this exchange, if any, was "Welcome to the perl guts!"

It's true enough, but if it's going to be needed, let's get this documented so
we can point people at the documentation when the code breaks.

--
rjbs

[p5pRT]

From @doy

On Mon, Feb 25, 2013 at 08​:06​:11AM -0800, Ricardo SIGNES wrote​:

A number of things broken by the new COW work were answered with, "By the way,
you need to use THINKFIRST there." This, in turn, was followed by, "What?
That's undocumented and how was I supposed to know that?" The third line in
this exchange, if any, was "Welcome to the perl guts!"

It's true enough, but if it's going to be needed, let's get this documented so
we can point people at the documentation when the code breaks.

Also, the core typemap for char* (not const char*) needs to be updated
to use THINKFIRST, since apparently people expect it to produce a
writable string buffer.

-doy

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @bulk88

On Mon Feb 25 08​:33​:29 2013, doy@​tozt.net wrote​:

Also, the core typemap for char* (not const char*) needs to be updated
to use THINKFIRST, since apparently people expect it to produce a
writable string buffer.

-doy

perlxs and perlxstypemap needs to be fixed. It uses "bool_t
rpcb_gettime(const char *host, time_t *timep);" as an example, but in
the XS code it uses "char *" everywhere. It needs to be written in
perlguts or perlxs to advise the reader to always use const char * and
SvPV_const() if possible for performance reasons. Also a new T-* type
needs to be created for non-de-cowed PVs. I randomly throw out T_CPV as
a suggestion. I am not sure what exactly needs to be added to perlguts.

As a separate issue, "Memory Allocation" in perlguts should mention
sv_usepvn_flags exists, and the pod for sv_usepvn_flags needs to say it
only takes "Newx" memory never any kind of other memory like "malloc"
memory.

This ticket also needs to be linked to #116569.

--
bulk88 ~ bulk88 at hotmail.com

[p5pRT]

From @Leont

On Mon, Feb 25, 2013 at 5​:06 PM, Ricardo SIGNES
<perlbug-followup@​perl.org> wrote​:

A number of things broken by the new COW work were answered with, "By the way,
you need to use THINKFIRST there." This, in turn, was followed by, "What?
That's undocumented and how was I supposed to know that?" The third line in
this exchange, if any, was "Welcome to the perl guts!"

It's true enough, but if it's going to be needed, let's get this documented so
we can point people at the documentation when the code breaks.

Father C already documented SV_THINKFIRST in a11eaec, though I think
he was too optimistic (it's not just needed on SvPVX, but on any SvPV*
write) and not marked as API. SV_CHECK_THINKFIRST and
SV_CHECK_THINKFIRST_COW_DROP still need to be documented though.

Leon

[p5pRT]

From @iabyn

On Thu, Mar 21, 2013 at 01​:15​:00PM +0100, Leon Timmermans wrote​:

On Mon, Feb 25, 2013 at 5​:06 PM, Ricardo SIGNES
<perlbug-followup@​perl.org> wrote​:

A number of things broken by the new COW work were answered with, "By the way,
you need to use THINKFIRST there." This, in turn, was followed by, "What?
That's undocumented and how was I supposed to know that?" The third line in
this exchange, if any, was "Welcome to the perl guts!"

It's true enough, but if it's going to be needed, let's get this documented so
we can point people at the documentation when the code breaks.

Father C already documented SV_THINKFIRST in a11eaec, though I think
he was too optimistic (it's not just needed on SvPVX, but on any SvPV*
write) and not marked as API. SV_CHECK_THINKFIRST and
SV_CHECK_THINKFIRST_COW_DROP still need to be documented though.

Also, Its not just adding API documentation. The xs*.pod and/or perlguts
docs need updating so people know they need to use these.

--
You never really learn to swear until you learn to drive.

[p5pRT]

From @rjbs

I have moved this from 5.18 to 5.20 blockers.

It sounds like this needs more than mere documentation, and we're not going to gain much by
documenting it now, encouraging its uptake, then changing how one must use it or its future
analogue.

--
rjbs

[p5pRT]

From @cpansprout

On Tue Mar 26 13​:55​:32 2013, rjbs wrote​:

I have moved this from 5.18 to 5.20 blockers.

It sounds like this needs more than mere documentation, and we're not
going to gain much by
documenting it now, encouraging its uptake, then changing how one must
use it or its future
analogue.

Also, if I’m not mistaken, sv_force_normal *is* already in the API, so
we don’t actually *need* to ‘expose’ SV_CHECK_THINKFIRST.

--

Father Chrysostomos

[p5pRT]

From @Leont

On Tue, Mar 26, 2013 at 9​:55 PM, Ricardo SIGNES via RT
<perlbug-followup@​perl.org> wrote​:

I have moved this from 5.18 to 5.20 blockers.

It sounds like this needs more than mere documentation, and we're not going to gain much by
documenting it now, encouraging its uptake, then changing how one must use it or its future
analogue.

But what to do about the typemap?

[p5pRT]

From @bulk88

On Tue Mar 26 17​:13​:22 2013, sprout wrote​:

Also, if I’m not mistaken, sv_force_normal *is* already in the API, so
we don’t actually *need* to ‘expose’ SV_CHECK_THINKFIRST.

And why unconditionally call a function that isn't needed?

______________________________________________________________________
#if PERL_BCDVERSION >= 0x5007001
# define PREP_SV_SET(sv) if(SvTHINKFIRST((sv)))
sv_force_normal_flags((sv), SV_COW_DROP_PV)
#else
# define PREP_SV_SET(sv) if(SvTHINKFIRST((sv))) sv_force_normal((sv))
#endif
______________________________________________________________________
later on
______________________________________________________________________
  PREP_SV_SET(TARG);
  SvOK_off(TARG);
______________________________________________________________________

--
bulk88 ~ bulk88 at hotmail.com

[p5pRT]

From @bulk88

On Tue Mar 26 20​:53​:12 2013, bulk88 wrote​:

On Tue Mar 26 17​:13​:22 2013, sprout wrote​:

Also, if I’m not mistaken, sv_force_normal *is* already in the API, so
we don’t actually *need* to ‘expose’ SV_CHECK_THINKFIRST.

And why unconditionally call a function that isn't needed?

______________________________________________________________________
#if PERL_BCDVERSION >= 0x5007001
# define PREP_SV_SET(sv) if(SvTHINKFIRST((sv)))
sv_force_normal_flags((sv), SV_COW_DROP_PV)
#else
# define PREP_SV_SET(sv) if(SvTHINKFIRST((sv))) sv_force_normal((sv))
#endif
______________________________________________________________________
later on
______________________________________________________________________
PREP_SV_SET(TARG);
SvOK_off(TARG);
______________________________________________________________________

  ^
Accidental post /|\
  |
And why unconditionally call a function that isn't needed?

This is what I figured out is how to make an existing SV undef-enough to
write directly into. SvUPGRADE will also be required but isn't listed here.

______________________________________________________________________
#if PERL_BCDVERSION >= 0x5007001
# define PREP_SV_SET(sv) if(SvTHINKFIRST((sv)))
sv_force_normal_flags((sv), SV_COW_DROP_PV)
#else
# define PREP_SV_SET(sv) if(SvTHINKFIRST((sv))) sv_force_normal((sv))
#endif
______________________________________________________________________
later on
______________________________________________________________________
PREP_SV_SET(sv);
SvOK_off(sv);
______________________________________________________________________

Some dont care about the contents of a COW SV and want to release it.
Others want to use the contents but also overwrite it. There are 2
different features. Also, SvOK_off may call sv_backoff(), (force_normal
wont) which is also confusion, since OOK is another form of trickery.

In a number of places SV_CHECK_THINKFIRST_COW_DROP is followed exactly
by, or often (not all branches), SvOK_off. SvOK_off,
SV_CHECK_THINKFIRST_COW_DROP, and SvUPGRADE are all flag test and maybe
func call macros. Maybe the 3 should be merged into 1 or 2 flag tests (2
meaning flags and sv type num), and 1 func with a flags bitfield (extend
force_normal to also do upgrades and mathom sv_upgrade(). All 3 macros
would call (made up name) sv_force_anywayuwant_flags under the hood with
a flags argument (1 char devoted to new requested type, 0x00 (SVt_NULL,
can't upgrade to that right?) or 0xff mean no upgrade happens, other 3
bytes control want to write to PV, want to replace, de-cow, cow drop,
de-offset, get/set magic calls during emptying of SV,
de-rv/glob/version, die on non-COW RO, etc (my list is too large to be
sane and some things are the same)).

--
bulk88 ~ bulk88 at hotmail.com

[p5pRT]

From @iabyn

On Tue, Mar 26, 2013 at 05​:13​:22PM -0700, Father Chrysostomos via RT wrote​:

On Tue Mar 26 13​:55​:32 2013, rjbs wrote​:

I have moved this from 5.18 to 5.20 blockers.

It sounds like this needs more than mere documentation, and we're not
going to gain much by
documenting it now, encouraging its uptake, then changing how one must
use it or its future
analogue.

Also, if I’m not mistaken, sv_force_normal *is* already in the API, so
we don’t actually *need* to ‘expose’ SV_CHECK_THINKFIRST.

Also, since the thing that triggered this discussion originally was
"how to make it safe under COW to directly modify the SvPVX buffer", note
that SV_CHECK_THINKFIRST etc don't actually guarantee you a writeable
SvPVX; for that you need SvPV_force (which does the equivalent of
SV_CHECK_THINKFIRST, but then in addition converts a IV/NV/RV etc into an
equivalent string value).

Since SvPV_force is already API, and is already documented (since 2000 or
earlier) as "You want force if you are going to update the SvPVX
directly", I think that's enough to keep us going till 5.18.

See also my recent comments in ticket RT #116407 about improving the docs
for SvPV to make it clear you can't just trample on PVX, and need
SvPV_force instead.

--
Art is anything that has a label (especially if the label is "untitled 1")

[p5pRT]

From @tonycoz

On Tue Mar 26 13​:55​:32 2013, rjbs wrote​:

I have moved this from 5.18 to 5.20 blockers.

It sounds like this needs more than mere documentation, and we're not
going to gain much by
documenting it now, encouraging its uptake, then changing how one must
use it or its future
analogue.

Beating this dead horse which is a blocker for 5.20​:

What might need to be done​:

- neither perlxs nor perlxstut discuss modifying (or even
  reading) the PV part of a scalar directly, but since they're
  about *XS* I'm not sure they should

- L<perlguts/Working with SVs> should probably discuss how to
  work with the SV's buffer, including SvPV_force(),
  SvPVbyte_force(), SvPVutf8_force(), SvGROW(). I'm not sure if
  this belongs after the SvGROW discussion, or after the
  sv_cat*() discussion.

- do we need a typemap entry? I can think of two cases where
  you're writing back to the SV​:

  a) the caller provides a fixed length buffer, in which case
  you need your own typemap entry (or inline code) to
  verify/force the buffer size

  b) the caller provides a variable length buffer

  For case b) in most cases I think you'd also need the
  length (remember, it's writable, no-one does gets() type
  interfaces anymore, right?), which requires inline code.

  So the only case that doesn't need the length (and hence
  inline code or a custom typemap) is a gets() style interface,
  for which I'm inclined to let people shoot their own feet off.

  The only real way I could see doing it would be to provide our
  own custom typedefs​:

  typedef unsigned char WRITABLE_BYTE;
  typedef unsigned char WRITABLE_UTF8;
  typedef unsigned char WRITABLE_DONTCARE; /* for the wild ones */

  and create typemaps for WRITABLE_BYTE etc​:

  WRITEABLE_BYTE * T_WRITEABLE_BYTE
  WRITEABLE_UTF8 * T_WRITEABLE_UTF8
  WRITEABLE_DONTCARE * T_WRITEABLE_DONTCARE
  ...
  T_WRITEABLE_BYTE
  $var = SvPVbyte_force_nolen($arg); // no such macro

  T_WRITEABLE_UTF8
  $var = SvPVutf8_force_nolen($arg); // no such macro

  T_WRITEABLE_BYTE
  $var = SvPV_force_nolen($arg);

What doesn't need to be done​:

- SvPV() documents that you should use SvPV_force() if you want
  to write to the PV string. Possibly this could be made
  clearer (making the mention of SvPV_force() a new paragraph
  might do it)

- SvPV_force() documents that the PV string is writable after
  calling it.

- under Copy on Write, perlguts documents that you need
  sv_force_normal() or SvPV_force_nolen() to make the SV
  non-COW.

Hopefully that makes some sense.

Tony

[p5pRT]

From @bulk88

On Tue Feb 04 20​:24​:36 2014, tonyc wrote​:

On Tue Mar 26 13​:55​:32 2013, rjbs wrote​:

I have moved this from 5.18 to 5.20 blockers.

It sounds like this needs more than mere documentation, and we're not
going to gain much by
documenting it now, encouraging its uptake, then changing how one must
use it or its future
analogue.

Beating this dead horse which is a blocker for 5.20​:

What might need to be done​:

- neither perlxs nor perlxstut discuss modifying (or even
reading) the PV part of a scalar directly, but since they're
about *XS* I'm not sure they should

- L<perlguts/Working with SVs> should probably discuss how to
work with the SV's buffer, including SvPV_force(),
SvPVbyte_force(), SvPVutf8_force(), SvGROW(). I'm not sure if
this belongs after the SvGROW discussion, or after the
sv_cat*() discussion.

- do we need a typemap entry? I can think of two cases where
you're writing back to the SV​:

a) the caller provides a fixed length buffer, in which case
you need your own typemap entry (or inline code) to
verify/force the buffer size

b) the caller provides a variable length buffer

For case b) in most cases I think you'd also need the
length (remember, it's writable, no-one does gets() type
interfaces anymore, right?), which requires inline code.

So the only case that doesn't need the length (and hence
inline code or a custom typemap) is a gets() style interface,
for which I'm inclined to let people shoot their own feet off.

The only real way I could see doing it would be to provide our
own custom typedefs​:

 typedef unsigned char WRITABLE\_BYTE;
 typedef unsigned char WRITABLE\_UTF8;
 typedef unsigned char WRITABLE\_DONTCARE; /\* for the wild ones \*/

and create typemaps for WRITABLE_BYTE etc​:

 WRITEABLE\_BYTE \*           T\_WRITEABLE\_BYTE
 WRITEABLE\_UTF8 \*           T\_WRITEABLE\_UTF8
 WRITEABLE\_DONTCARE \*       T\_WRITEABLE\_DONTCARE
 \.\.\.
 T\_WRITEABLE\_BYTE
    $var = SvPVbyte\_force\_nolen\($arg\); // no such macro

 T\_WRITEABLE\_UTF8
    $var = SvPVutf8\_force\_nolen\($arg\); // no such macro

 T\_WRITEABLE\_BYTE
    $var = SvPV\_force\_nolen\($arg\);

What doesn't need to be done​:

- SvPV() documents that you should use SvPV_force() if you want
to write to the PV string. Possibly this could be made
clearer (making the mention of SvPV_force() a new paragraph
might do it)

- SvPV_force() documents that the PV string is writable after
calling it.

- under Copy on Write, perlguts documents that you need
sv_force_normal() or SvPV_force_nolen() to make the SV
non-COW.

Hopefully that makes some sense.

Tony

The problem is SvPV_force_nolen and SvPVbyte_force_nolen do something totally useless if you just want to write into them. If I am going to just write, why do I want to A. copy the COW contents to a new non-COW block, B. convert encodings, then write over it? I want a uninitialized PV. sv_force_normal() copies the COW data over. Some people want R/W buffers to manipulate in place the data too.

--
bulk88 ~ bulk88 at hotmail.com

[p5pRT]

From perl5-porters@perl.org

Tony Cook wrote​:

Beating this dead horse which is a blocker for 5.20​:

What might need to be done​:

The Copy on Write section added to perlguts was meant to address this.
I think it is sufficient that this ticket no longer needs to be a
blocker, but I do not mean to imply that your suggestions should not
be followed,