Front page | perl.perl5.porters |
Postings from July 2016
Re: Indented here docs?
Thread Previous
|
Thread Next
From:
wolfsage
Date:
July 2, 2016 12:22
Subject:
Re: Indented here docs?
Message ID:
CAJ0K8bjmqaVBN434W+iZkeFeWtAjxemOkq5BngJSqVj-hvHAUQ@mail.gmail.com
On Sat, Jul 2, 2016 at 1:38 AM, Father Chrysostomos <sprout@cpan.org> wrote:
> Matthew Horsfall wrote:
>> My C is rusty so some extra eyes would be wonderful.
>
> I have done a cursory reading. It mostly looks good but I do have a few observations. First:
Thank you for the review.
> What about <<~`EOF`? I think your patch covers it, but it is untested.
Ah yeah, more testing required.
>
>> + /* Shift left 1 char, nuke first \n */
>
> I don’t understand why you shift left one char. (I have not run it through a debugger.) If you did s++ before scanning the heredoc delimiter, then why does PL_tokenbuf not already hold the right value? (I am probably missing something obvious.)
Earlier on:
PERL_ARGS_ASSERT_SCAN_HEREDOC;
s += 2;
d = PL_tokenbuf + 1;
e = PL_tokenbuf + sizeof PL_tokenbuf - 1;
*PL_tokenbuf = '\n';
peek = s;
So PL_tokenbuf becomes \nDELIMITER\n. Although looking at the code
again, I can probably just do PL_tokenbuf+1 later where needed. I'll
look into that.
>
>> I haven't tested with PERL_STRICT_CR,
>
> PERL_STRICT_CR has probably not been tested for more than a decade. :-) In any case, the non-PERL_STRICT_CR builds (the normal builds) are the more complex, so I don’t think you need to worry.
Hrm... should we remove it?
>> /* inside a string eval or quote-like operator */
>> if (!infile || PL_lex_inwhat) {
>
> That code parses the heredoc when the rest of the ‘file’ is inside a single buffer. See the comments above S_scan_heredoc that explain the two approaches.
Ah fun. I blame myself taking "Don't read the comments!" too far.
> The beginning of that block is scary code to deal with things like "${s//<<END/e}". (Welcome to perl 5!) The actual here-doc parsing happens after the ‘if (PL_lex_inwhat)’ block.
>
> You need to modify that code, too. I think your existing tests will fail if you put them inside a string eval.
>
> In fact, you would probably end up repeating a large amount of code, so you may want to put some of it in static functions.
Okay, will do.
> We currently have a policy that new features go through an experimental period for a while. I think that would have to apply to this feature, too. And then you can use <<- without having to worry (for now) about breaking existing syntax.
Oh fun. Okay.
>
> I agree with Zefram and others that the exact sequence of tabs and spaces found before the closing delimiter should be required on each preceding line, and that anything else should be an error.
I'm glad most people are learning towards this solution, as it keeps
my job easy :)
I'll upgrade the warning to a fatal.
While I have your attention, I do have one concern. In other places I see:
if (*s == term && PL_bufend-s >= len
&& memEQ(s,PL_tokenbuf + 1,len)) {
Which appears to be guarding memEQ from reading past the end of s.
Should I similarly guard these:
char * found = ninstr(s, s + (PL_bufend-s), PL_tokenbuf, PL_tokenbuf + len);
and
/* Found our indentation? Strip it */
} else if (memEQ(ss, indent, indent_len)) {
I would think not since the end of s and ss should contain a \0 and
memEQ/ninstr should stop there, but I guess they'd keep going if
PL_tokenbuf or indent somehow had a null byte in the middle...
Also, am I leaking anything here?
sv_setsv(tmpstr,newstr);
Thanks again,
-- Matthew Horsfall (alh)
Thread Previous
|
Thread Next