FAQ

On Sat Apr 26 13:03:54 2014, jhi wrote:
Attached.
--- a/toke.c
+++ b/toke.c
@@ -10398,7 +10398,7 @@ S_scan_inputsymbol(pTHX_ char *start)
       Copy("ARGV",d,5,char);

   /* Check whether readline() is overriden */
- gv_readline = gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
+ gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
   if ((gv_readline = gv_override("readline",8)))
       readline_overriden = TRUE;


I think that line can just be removed. The first thing gv_override() does is:

     GV *gv = gv_fetchpvn(name, len, GV_NOTQUAL, SVt_PVCV);

--- a/x2p/a2py.c
+++ b/x2p/a2py.c
@@ -70,7 +70,7 @@ main(int argc, const char **argv)

      myname = argv[0];
      linestr = str_new(80);
- str = str_new(0); /* first used for -I flags */
+ str_new(0); /* first used for -I flags */
      for (argc--,argv++; argc; argc--,argv++) {
   if (argv[0][0] != '-' || !argv[0][1])
       break;

Should simply be removed, str_new() is an allocator that pulls from a free list if it can.

--- a/x2p/walk.c
+++ b/x2p/walk.c
@@ -254,7 +254,7 @@ sub Pick {\n\
   break;
      case OHUNK:
   if (len == 1) {
- str = str_new(0);
+ str_new(0);
       str = walk(0,level,oper1(OPRINT,0),&numarg,P_MIN);
       str_cat(str," if ");
       str_scat(str,fstr=walk(0,level,ops[node+1].ival,&numarg,P_MIN));

Again.

@@ -264,7 +264,7 @@ sub Pick {\n\
   else {
       tmpstr = walk(0,level,ops[node+1].ival,&numarg,P_MIN);
       if (*tmpstr->str_ptr) {
- str = str_new(0);
+ str_new(0);
    str_set(str,"if (");
    str_scat(str,tmpstr);
    str_cat(str,") {\n");

This one looks wrong - str is used on the next line.

Tony

---
via perlbug: queue: perl5 status: new
https://rt.perl.org/Ticket/Display.html?id=121747

Search Discussions

  • bulk88 via RT at Apr 28, 2014 at 7:11 am

    On Sat Apr 26 13:03:54 2014, jhi wrote:
    Attached.
    I disagree with patch. When step debugging -O0/-Od code, seeing the c auto assignment which will appear in a "watch" area is important. On optimized code, the assignments and/or c auto vars are optimized away so the assignments dont hurt.

    --
    bulk88 ~ bulk88 at hotmail.com

    ---
    via perlbug: queue: perl5 status: open
    https://rt.perl.org/Ticket/Display.html?id=121747
  • Jarkko Hietaniemi at Apr 28, 2014 at 11:14 am

    On Monday-201404-28, 3:11, bulk88 via RT wrote:
    I disagree with patch.
    When step debugging -O0/-Od code, seeing the c auto assignment which
    will appear in a "watch" area is important.

    I don't understand. How is seeing an assignment that doesn't
    effectively do anything important? The RHS is still being called
    (unless it was already dead code, of course). If you want to break on
    that line, break on that line.

      >
    On optimized code, the assignments and/or c auto vars are optimized away
    so the assignments dont hurt.

    I don't think this is a good trade-off. Dead code is confusing to the
    reader of the code.
  • bulk88 via RT at Apr 30, 2014 at 6:04 am

    On Mon Apr 28 04:15:01 2014, jhi wrote:
    On Monday-201404-28, 3:11, bulk88 via RT wrote:
    I disagree with patch.
    When step debugging -O0/-Od code, seeing the c auto assignment which
    will appear in a "watch" area is important.

    I don't understand. How is seeing an assignment that doesn't
    effectively do anything important? The RHS is still being called
    (unless it was already dead code, of course). If you want to break on
    that line, break on that line.
    How are you doing to see the returned value of the function in a debugger even if you break on the line if its not assigned to a var?
    On optimized code, the assignments and/or c auto vars are optimized away
    so the assignments dont hurt.

    I don't think this is a good trade-off. Dead code is confusing to the
    reader of the code.
    What is the value of slice tcode? and you can not recompile the code or make the C debugger call sv_dump on demand or look at asm code and registers to figure it out

    av_store(av, 0 newSVsv(*hv_fetch(hv, "tcode", sizeof("tcode")-1, 1)));

    or

    startPerlTranscact(pTHX_ hv) {
         hv_fetch(hv, "started", sizeof("started")-1, 1);
         startTransact(SvIVx(*hv_fetch(hv, "ptr", sizeof("ptr")-1, 0))); /* SEGV intentional */
    }

    Was it in progress already? startTransact() doesn't care, but maybe you do, unless you save the result of hv_fetch to SV ** "svp" and get a "ptr values immediately discarded" warning, you won't know if it was started already unless you stepped through hv_common, and stepping through hv_common costs alot of human time.

    --
    bulk88 ~ bulk88 at hotmail.com

    ---
    via perlbug: queue: perl5 status: open
    https://rt.perl.org/Ticket/Display.html?id=121747
  • Dave Mitchell at Apr 30, 2014 at 3:54 pm

    On Tue, Apr 29, 2014 at 11:04:00PM -0700, bulk88 via RT wrote:
    On Mon Apr 28 04:15:01 2014, jhi wrote:
    On Monday-201404-28, 3:11, bulk88 via RT wrote:
    I disagree with patch.
    When step debugging -O0/-Od code, seeing the c auto assignment which
    will appear in a "watch" area is important.

    I don't understand. How is seeing an assignment that doesn't
    effectively do anything important? The RHS is still being called
    (unless it was already dead code, of course). If you want to break on
    that line, break on that line.
    How are you doing to see the returned value of the function in a debugger even if you break on the line if its not assigned to a var?
    This is silly. How would you debug code like

         if (foo() || bar()) ....
    or
         (void)foo(....);
    or
         foo(bar());

    Would you ban all such uses from the perl core? There are thousands of
    places in the perl core where function calls don't assign their return
    value to an explicit local var.

    If I were to debug such a statement using gdb say, I might single-step
    into the function, then do 'finish' to run to the end of the function. gdb
    will then automatically display the return value of the function.

    If I needed to do a lot of debugging on that statement, I might
    temporarily edit the code to add an intermediate variable.

    --
    "There's something wrong with our bloody ships today, Chatfield."
         -- Admiral Beatty at the Battle of Jutland, 31st May 1916.
  • bulk88 via RT at Apr 30, 2014 at 8:32 pm

    On Wed Apr 30 08:55:06 2014, davem wrote:

    This is silly. How would you debug code like

    if (foo() || bar()) ....
    or
    (void)foo(....);
    or
    foo(bar());

    Would you ban all such uses from the perl core? There are thousands
    of
    places in the perl core where function calls don't assign their return
    value to an explicit local var.
    If saving it to an auto is already there, we shouldn't be removing it just to satisfy a tool's policies. I am not calling for adding it everywhere to the core, just not removing it if its already there for some reason. Also if for some reason, the return value needs to be used in the future, the name of the var to hold the returned value is already known and a new name doesn't have to be thought up of.
    On Wed Apr 30 10:03:33 2014, shay wrote:

    In Dev Studio (2010, at least, I've not looked at earlier versions for
    a long time), if you break on a function call and then step over it
    then the return value of the function is shown in the Autos window,
    with the Name column (normally containing a variable's name) being
    "<function> returned" and the Value column being the <function>'s
    return value.
    I didn't want to discuss VS IDE specifically in this ticket, but I know of the Autos window, but I never keep it on the screen, since it will show the value for exactly 1 line, not 2 or 3 lines. 1 click too many and its time to restart the app. I find the Auto tab to be almost useless. Locals tab is what I keep on screen.

    --
    bulk88 ~ bulk88 at hotmail.com

    --
    bulk88 ~ bulk88 at hotmail.com

    ---
    via perlbug: queue: perl5 status: open
    https://rt.perl.org/Ticket/Display.html?id=121747
  • Steve Hay at Apr 30, 2014 at 5:03 pm

    On 30 April 2014 07:04, bulk88 via RT wrote:
    On Mon Apr 28 04:15:01 2014, jhi wrote:
    On Monday-201404-28, 3:11, bulk88 via RT wrote:
    I disagree with patch.
    When step debugging -O0/-Od code, seeing the c auto assignment which
    will appear in a "watch" area is important.

    I don't understand. How is seeing an assignment that doesn't
    effectively do anything important? The RHS is still being called
    (unless it was already dead code, of course). If you want to break on
    that line, break on that line.
    How are you doing to see the returned value of the function in a debugger even if you break on the line if its not assigned to a var?
    In Dev Studio (2010, at least, I've not looked at earlier versions for
    a long time), if you break on a function call and then step over it
    then the return value of the function is shown in the Autos window,
    with the Name column (normally containing a variable's name) being
    "<function> returned" and the Value column being the <function>'s
    return value.
  • Jarkko Hietaniemi at Apr 28, 2014 at 11:29 am

    - gv_readline = gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
    + gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
    if ((gv_readline = gv_override("readline",8)))
    readline_overriden = TRUE;


    I think that line can just be removed. The first thing gv_override() does is:

    GV *gv = gv_fetchpvn(name, len, GV_NOTQUAL, SVt_PVCV);
    Discarded.
    - str = str_new(0); /* first used for -I flags */
    + str_new(0); /* first used for -I flags */
    Should simply be removed, str_new() is an allocator that pulls from a free list if it can.
    Removed. Left the comment though.
    - str = str_new(0);
    + str_new(0);
    Again. Ditto.
    @@ -264,7 +264,7 @@ sub Pick {\n\
    else {
    tmpstr = walk(0,level,ops[node+1].ival,&numarg,P_MIN);
    if (*tmpstr->str_ptr) {
    - str = str_new(0);
    + str_new(0);
    str_set(str,"if (");
    str_scat(str,tmpstr);
    str_cat(str,") {\n");

    This one looks wrong - str is used on the next line.
    My bad.

    Thanks for the review of my patches, by the way! I'm a bit rusty in
    this business.

    Refreshed patch attached.
  • Tony Cook via RT at May 5, 2014 at 5:42 am

    On Mon Apr 28 04:30:00 2014, jhi wrote:
    - str = str_new(0); /* first used for -I flags */
    + str_new(0); /* first used for -I flags */
    Should simply be removed, str_new() is an allocator that pulls from a
    free list if it can.
    Removed. Left the comment though.
    I'm not sure the comment ever applied - I don't see any handling of a -I flag, even back in perl 1.0.

    I don't see any other issues.

    Tony


    ---
    via perlbug: queue: perl5 status: open
    https://rt.perl.org/Ticket/Display.html?id=121747
  • Jarkko Hietaniemi at May 6, 2014 at 8:04 pm
    Refreshed patch attached.
    Found one more similar spot, in toke.c:10260-ish

       s = lex_grow_linestr(...);

    the s was very shortly overwritten.

    Refreshed patch attached.

    >
  • Tony Cook via RT at May 7, 2014 at 12:18 am

    On Tue May 06 13:04:57 2014, jhi wrote:

    Refreshed patch attached.
    Found one more similar spot, in toke.c:10260-ish

    s = lex_grow_linestr(...);

    the s was very shortly overwritten.

    Refreshed patch attached.
    @@ -10257,7 +10257,7 @@ S_scan_heredoc(pTHX_ char *s)
       }
       CopLINE_set(PL_curcop, origline);
       if (!SvCUR(PL_linestr) || PL_bufend[-1] != '\n') {
    - s = lex_grow_linestr(SvLEN(PL_linestr) + 3);
    + lex_grow_linestr(SvLEN(PL_linestr) + 3);
                  /* ^That should be enough to avoid this needing to grow: */
           sv_catpvs(PL_linestr, "\n\0");

    In debugging builds s is used before it's overwritten:

                  assert(s == SvPVX(PL_linestr));

    Nicholas added both the assignment and the assert() in d8fe30ad.

    Tony

    ---
    via perlbug: queue: perl5 status: open
    https://rt.perl.org/Ticket/Display.html?id=121747
  • Jarkko Hietaniemi at May 7, 2014 at 12:28 am

    In debugging builds s is used before it's overwritten:

    assert(s == SvPVX(PL_linestr));

    Nicholas added both the assignment and the assert() in d8fe30ad.
    Urgnkh. Please discard the latest revision, and prefer the one before
    that, then.
    Tony
  • Tony Cook via RT at May 29, 2014 at 6:14 am

    On Mon Apr 28 04:30:00 2014, jhi wrote:
    Refreshed patch attached.
    Thanks, applied as de092133b63ee6f61ed274e5c366adf60622300e with a subject edit.

    Tony

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupperl5-porters @
categoriesperl
postedApr 28, '14 at 4:56a
activeMay 29, '14 at 6:14a
posts13
users4
websiteperl.org

People

Translate

site design / logo © 2021 Grokbase