<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Feb 19, 2014 at 12:44 PM, Marshall Clow <span dir="ltr"><<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class=""><div>On Feb 19, 2014, at 11:21 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Feb 19, 2014 at 9:17 AM, Marshall Clow <span dir="ltr"><<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>On Feb 14, 2014, at 4:31 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
</div><div><div><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote">On Fri, Feb 14, 2014 at 4:21 PM, Marshall Clow <span dir="ltr"><<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Users can write<br> for(sregex_iterator i(s.begin(), s.end(), regex("meow")), end; i != end; ++i)<br>
<br>binding a temporary regex to const regex& and storing a pointer to it.<br>This will compile silently, triggering undefined behavior at runtime.<br><br>Fixing this involves defining constructors for the various regex iterator types from rvalue regexes, and then marking them as “deleted”.<br>
And tests.<br></blockquote></div></div></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><div dir="ltr">Would this break (what I assume is) correct code doing things like:<br><br>std::find(sregex_iterator(s.begin(), s.end(), regex("meow")), end, "foo”); ?</div>
</blockquote><div><br></div></div><div>(Recapping a discussion on IRC)</div><div><br></div>Yes, it would.</div><div>And though that code is "technically correct” it (a) is dangerous, and (b) doesn’t do anything useful.</div>
<div><br></div><div>The return result from std::find is an interator into a destroyed object, so you can’t really do anything with that.</div><div><br></div><div>The “fix” for this code is simple:</div><div><br></div><div>
<span style="white-space:pre-wrap"> </span>regex re{“meow”}</div><div><div dir="ltr"><span style="white-space:pre-wrap"> </span>std::find(sregex_iterator(s.begin(), s.end(), re), end, "foo”); </div><div dir="ltr">
<br></div><div dir="ltr">and now the return value of std::find is valid (until the end of the lifetime of “re”).</div></div></div></blockquote><div><br></div><div>Fair point about find, though it's not too hard to find examples that take iterators and don't return temporaries from them.<br>
<br>There are lots of APIs that take references to things and return references with the same lifetime as the parameter and would break in similar situations, but perhaps these particular APIs are substantially more prone to this kind of problem and worthy of a specific safety guard.<br>
</div></div></div></div></blockquote><div><br></div></div>If you’ve got a list, I’ll take a look at them.</div><div>Maybe the committee will want to “protect” them too.</div></div></blockquote><div><br></div><div>Well, I mean something as simple as:<br>
<br> const char *c = std::string(...).c_str();<br><br>things we deal with all the time. But perhaps that one's obvious enough as to not benefit from protection compared to the cost in usability.<br><br>The other decorating iterators have non-const references (streams (stream_iterator) or containers (back/front/insert iterators)) so they don't suffer from this problem.<br>
<br>Looking at it most of the examples I can find involve member functions returning something that's a reference into the object itself - which is probably obvious/common enough/not as spooky as the case you're addressing here.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div><div class=""><blockquote type="cite"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div dir="ltr">See <br>
</div><div dir="ltr"><span style="white-space:pre-wrap"> </span><a href="http://cplusplus.github.io/LWG/lwg-active.html#2329" target="_blank">http://cplusplus.github.io/LWG/lwg-active.html#2329</a></div>
<div dir="ltr">and<span style="white-space:pre-wrap"> </span><a href="http://cplusplus.github.io/LWG/lwg-active.html#2332" target="_blank">http://cplusplus.github.io/LWG/lwg-active.html#2332</a></div><div><div dir="ltr">
<br>
</div></div><div dir="ltr">for the changes to the (C++14) standard.</div></div></blockquote><div><br>In any case, this was entirely a misunderstanding on my part - not realizing (though you'd mentioned in the subject) that this was a new, deliberately non-strictly-backwards-compatible change mandated by the next standard. I'd mistakenly thought this was an attempt to add some standard-allowable-but-not-mandated safety so I was confused why it would break existing correct callers. I'm still a little surprised at this being worth a non-backward-compatible change in the standard, but that's up to you and the rest of the committee and I didn't mean to get in the way of whatever decision came out of that.<br>
<br>A few minor, optional, comments on the patch itself:<br><br>* An extra explicit scope in each of the test cases seems strange, but perhaps there's a reason for it given how consistently you've done that.<br></div>
</div></div></div></blockquote><div><br></div></div>That makes it easy to copy/paste blocks of code, w/o worrying about variable name conflicts.</div></div></blockquote><div><br></div><div>Ah. OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div class=""><br><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>* The "= delete" ctors you've added aren't preprocessor protected (just "// C++14") - why's that? Won't this break C++11? Or was this change accepted as a DR to ’11?<br>
</div></div></div></div></blockquote><div><br></div></div>They aren’t? Checks…</div><div><br></div><div>In the synopsis (the big comment at the front of the file), this is true. </div><div><br></div><div> regex_token_iterator(BidirectionalIterator a, BidirectionalIterator b,</div>
<div> const regex_type&& re, int submatch = 0,</div><div> regex_constants::match_flag_type m = regex_constants::match_default) = delete; // C++14</div><div><br></div>
<div>But that’s just a comment.</div></div></blockquote><div><br></div><div>Right - my mistake, just stuff in comments. Sorry.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>Down below, in the code, we have:</div><div><br></div><div><div>#if _LIBCPP_STD_VER > 11</div><div> regex_token_iterator(_BidirectionalIterator __a, _BidirectionalIterator __b,</div>
<div> const regex_type&& __re, int __submatch = 0,</div><div> regex_constants::match_flag_type __m =</div><div> regex_constants::match_default) = delete;</div>
<div>#endif</div><div><br></div><div><br></div></div><div class=""><div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>* The version tests of "> 11" and "<= 11" seem a bit harder to read (to me) than ">= 14" and "< 14" which speaks to the actual version the feature was introduced in rather than having to mentally compute that from the prior version.</div>
</div></div></div>
</blockquote></div><br></div><div>Sure. But we don’t have a C++14 standard yet. It’s just “the one after 11”. Right now, _LIBCPP_STD_VER is set to “some value > 11” if you compile with "-std=c++1y” (actually 13). When the standard is approved (probably in July), then I’ll change _LIBCPP_STD_VER to 14 when compiling with “-std=c++14”. (and make a new value for “the standard after 14”).</div>
<div><br></div><div>This is keyed off the value of __cplusplus, which is defined in the standard. But until we have a C++14 standard, we won’t have a value for __cplusplus (though I would guess it will be 201407L).</div>
</div></blockquote><div><br>Right... hrm. Ah well.<br><br>- David </div></div></div></div>