<div dir="rtl"><div dir="ltr">The LLVM ADTs has several common "themes" or preferences</div><div dir="ltr"><br></div><div dir="ltr">1) Efficient by default, efforts to avoid or minimize heap allocation - even at the cost of making debugging harder: pointers are not pointers but packed with bits or shared with other pointers, thus not the correct type, StringRef are not null terminated, etc. (We have visualizers to help.)</div>
<div dir="ltr">2) Easy to use by default, compatible with the STL containers, simply work.<br></div><div dir="ltr">3) Open to "advanced" customization of 1) and 2) via template arguments if required.<br></div>
<div dir="ltr"><br></div><div dir="ltr"><span style="font-size:12.800000190734863px;color:rgb(80,0,80);font-family:arial,sans-serif">Wether these features should be introduced in additional </span><span style="font-size:12.800000190734863px;color:rgb(80,0,80);font-family:arial,sans-serif">raw_string_ostream functionality or a new </span><span style="font-size:12.800000190734863px;font-family:arial,sans-serif">string_ostream</span><span style="font-size:12.800000190734863px;font-family:arial,sans-serif"> may be the result of what's technically possible such that it</span><br>
</div><div dir="ltr"><div dir="ltr"><br></div></div><div dir="ltr"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:12.800000190734863px">1) Uses a non-heap container (SmallString) by default.</span></div>
<div dir="ltr"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:12.800000190734863px">2) Is able to use other containers (std::string?) for clients who provide it.</span></div><div dir="ltr"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:12.800000190734863px">3) Easy to use = standard string/ostream semantics where possible.</span></div>
<div dir="ltr"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:12.800000190734863px">4) Hard to misuse = hard/impossible to get into invalid/unsynchronized state, does not require repeating code patterns.</span></div>
<div dir="ltr"><span style="color:rgb(80,0,80);font-family:arial,sans-serif;font-size:12.800000190734863px"><br></span></div><div dir="ltr">Yaron<br></div><div dir="ltr"><br></div><div class="gmail_extra" dir="ltr"><br><br>
<div class="gmail_quote"><div>2014-06-27 4:20 GMT+03:00 Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span>:</div>
<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"><div><div><br>
On 27/06/2014 02:50, David Blaikie 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">
On Thu, Jun 26, 2014 at 4:05 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>> 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">
On 27/06/2014 01:42, David Blaikie 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">
On Thu, Jun 26, 2014 at 3:37 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>> 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">
On 27/06/2014 00:48, David Blaikie 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">
On Wed, Jun 25, 2014 at 9:53 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>><br>
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">
On Wed, Jun 25, 2014 at 5:52 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>> 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">
On 26/06/2014 03:16, Chandler Carruth wrote:<br>
<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">
On Thu, Jun 26, 2014 at 2:09 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
I've made that change, plus a couple more updated uses, and<br>
landed<br>
this in r211749.<br>
</blockquote></blockquote>
FWIW, perhaps I wasn't clear - but my "I'd like to loop back around<br>
with Chandler to nail down the design choice" was "I'm not signing off<br>
on this patch without Chandler", so the commit may've been a bit<br>
premature.<br>
</blockquote>
Given this confusion, I think appropriate to back this patch out until<br>
it's actually been signed off on.<br>
<br>
Sorry for my vague language/lack of clarity over incremental review<br>
compared to sign-off.<br>
</blockquote>
<br>
I don't agree there was any problem with review here. The patch got its<br>
OKs<br>
and looks greats,<br>
</blockquote>
You asked me if it was OK to land. I gave some feedback and said I'd<br>
like to wait. You interpreted that as sign-off. I did not intend it as<br>
such. We seem to have failed to communicate and I'm trying to clarify<br>
what I intended to communicate.<br>
<br>
I'm sorry for the miscommunication, but I really didn't mean to give<br>
you the impression that this was ready to commit. Please revert it.<br>
</blockquote>
<br>
There's obviously no way to revert this without losing the bug fixes.<br>
</blockquote>
Right - they can all be included together in whatever form this is<br>
recommitted. No worries about that. (happens all the time here - given<br>
we prefer fine grained commits (ideally so that sometimes they can be<br>
reverted separately, or at least examined separately when looking<br>
through commit history, etc) so sometimes a set of related patches<br>
have to be reverted together. Not a problem)<br>
<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">
I've backed them out together temporarily in r211814.<br>
</blockquote>
Thanks!<br>
<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">
Could you state any concerns you have clearly?<br>
</blockquote>
Sure - sorry, just didn't want to confuse code review protocol with<br>
the specific merits or caveats of this patch. Not even a question of<br>
whether I had concerns or not, just that I hadn't actually signed off<br>
on the patch. A simple misunderstanding. I'll try to be more clear in<br>
the future.<br>
<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">
A critical feature of the patch is guaranteed safety. Where you see a<br>
"string_ostream" type you get an assurance that the string building<br>
operations are valid.<br>
</blockquote>
The difference here would just be "Where you see string_ostream<br>
default constructed, you get an assurance that the string building<br>
operations are valid", right? (it's just "if you use this constructor<br>
that takes an externally-provided buffer, buyer beware" - tihs is true<br>
of many types (eg: std::unique_ptr's raw ctor, compared to something<br>
like make_unique)).<br>
</blockquote>
<br></div></div>
What you're suggesting would be equivalent to having std::unique_ptr default-construct the pointee type which I'm pretty sure it doesn't do?<br>
<br>
By having a separate type, we get to document the semantics clearly, and search by type. We also get to hide the flush() function directly via the type system.<br>
<br>
In your scheme that would end up tucked away in the documentation:<br>
<br>
// If you constructed with X, then behaviour is Y, otherwise behaviour is Z.<br>
<br>
And even after documenting, I'm sure people would still feel compelled to flush() before str() occasionally. With the proposed patch the compiler issues a helpful diagnostic instead.<br>
<br>
(Note that std::stringstream *does* take a dual approach, and suffers from consistency as a result -- 66 open question on ' stringstream' on StackOverflow at time of writing is high for a single utility. This isn't really part of the standard C++ library we want to emulate in LLVM -- we're much more conscientious about our strings in general, and shouldn't drop the ball here.)<div>
<br>
<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"><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">
Those assurances apply even when the builder is passed by reference. You can<br>
see the clarity this adds for example in LLVMCreateMessage() and a few other<br>
places on ToT.<br>
</blockquote>
The cleanup in LLVMCreateMessage and callers seems almost entirely<br>
orthogonal to your new type. The same API change could be made even<br>
with the old code:<br>
<br>
string s;<br>
raw_string_ostream stream(s);<br>
...<br>
return LLVMCreateMessage(stream);<br>
</blockquote>
<br></div>
Actually I don't think the form you suggest above particularly safe. I'm glad you raised this as I chose the example intentionally.<br>
<br>
It's unconventional for a function to modify local variables in parent stack frames that weren't explicitly named in the argument list whichever way you bake it.<br>
<br>
No other code really does this in the LLVM source tree, and although you and I immediately grok what's going on as C++ frontend developers, it's actually subtle and easy to miss. The programming errors on ToT show just how easy it is to lose the plot with that pattern.<br>
<br>
So my change here wasn't orthogonal -- in fact it's completely aligned with and dependent upon the new safety guarantees introduced in the patch. We absolutely need representation in the type system so that we can enforce correctness.<div>
<br>
<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">
LLVMCreateMessage is still safe, clear, and correct using either API<br>
because str(), even on raw_string_ostream as it is today, is still<br>
well defined to flush and then give access to the string.<br>
<br>
Or, with a single type, it'd just be:<br>
<br>
raw_string_ostream stream;<br>
<br>
instead of<br>
<br>
string_ostream stream;<br>
<br>
Or is there some other clarity benefit you were referring to?<br>
<br>
(with the same LLVMCreateMessage change you made (though I personally<br>
would have LLVMCreateMessage take a const std::string& - it seems more<br>
appropriate for a variety of callers, even if the current callers all<br>
happen to have raw_string_ostreams at the moment - indeed, perhaps<br>
some of them should just be using string concatenation anyway))<br>
<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">
Importantly, there is no locality of construction so *the type becomes the<br>
documentation*.<br>
</blockquote>
Generally it's only important to know this distinction in the scope<br>
where the type is declared (because it's that scope where the external<br>
storage is provided) - once the stream is passed to some other API,<br>
the only access to the string is likely going to be through<br>
raw_string_ostream's "str()" member, which does the right thing.<br>
</blockquote>
<br></div>
See above.<div><br>
<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">
<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">
If we were to lump storage into raw_string_ostream, this strong safety<br>
indicated by the type system would disappear along with all the heap<br>
savings.<br>
</blockquote>
Heap savings can be reacquired where /really/ necessary by using<br>
external storage, if necessary. By building in storage, that would<br>
become the exception rather than the rule, and something we could more<br>
closely scrutinize during code review.<br>
<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">
(which simply aren't negligible compared to some of the effort going into<br>
bit packing elsewhere in the codebase).<br>
</blockquote>
Apples and oranges, I suspect (& wouldn't mind some evidence to the<br>
contrary before we proliferate more choices in data structure type on<br>
the codebase - we have a lot, many for good reason, but there is a<br>
cognitive/code review/etc load we pay by adding more). The bit packing<br>
sort of stuff happens in data structures that there are often<br>
thousands upon thousands of in a single run, or at a single point in<br>
time so they have a significant impact on peak memory usage. I doubt<br>
we have more than a handful of raw_string_ostreams in existence<br>
simultaneously.<br>
</blockquote>
<br></div>
I'd like numbers too, but from visual inspection we can already see that this introduces some worthwhile savings.<br>
<br>
string_ostream basically just encourages and simplifies our best practice of using SmallString<> on the stack, and that's never been in doubt.<br>
<br>
In the patch you can see we've fixed truncation issues in code that was previously storing in an external SmallString<>, not just std::string. Chandler's proposed raw_string_ostream wouldn't be a suitable replacement for those uses, missing about 50% of the potential fixes where we want to use this, particularly in clang and LLVM's IR facility. In my patch, we cover the lot with a single type.<br>
<br>
I saw you discussing how to avoid heap-allocation of a *singleton* recently on this list so I know you care at least a little about this matter :-)<div><br>
<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"><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">
If we mix semantics in the base, we'd basically just end up a raw string<br>
ostream that sometimes isn't raw, even making the name of the type a<br>
little dubious.<br>
</blockquote>
(On naming: as Chandler mentioned, these are all raw streams - even<br>
the new ones you're proposing, the "raw"ness is intended to<br>
distinguish them from the C++ Standard stream libraries that have<br>
streambufs and all that other stuff, not about whether they have<br>
internal storage or not)<br>
</blockquote>
<br></div>
Bike shed. I'm happy with what's in the patch, but still open to better names.<br>
<br>
Does this go some way to covering your remaining concerns?<br>
<br>
Alp.<br>
<br>
<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">
<br>
- David<br>
</blockquote><div>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br></div><div><div>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>