<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2016-12-29 14:54 GMT+01:00 Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div>From yesterday discussion, Daniel Berlin proposed using emplace_back everywhere to make code easier to maintain. I think it is valid argument, but I believe it would reduce readability.</div></div></blockquote><div><br></div></span><div>Just to be clear; I proposed not trying to switch uses back and forth without real data, and to come to some agreement about what should be written in the first place, preferably based on real data, and then, switching in some organized fashion, not just randomly :)</div><div><br></div><div>IE Either have a clear coding standard and enforce it, or leave uses alone one way or the other without some demonstration that it actually matters.</div><div><br></div><div>I would *personally* prefer to just use emplace_back everywhere, and not try to switch between the two, without demonstration of real benefit.</div><div><br></div><div>(i'd also accept the other way, use push_back everywhere, and not try to switch between the two, without demonstration of real benefit).</div><div><br></div><div>This preference is based on a simple truth: People suck at deciding when to use one or the other. Yes, i know how to use it and when to use it.  Lots of our types are cheaply movable, etc, so you probably won't see any performance difference even when using them "right".  People have enough style/etc issues without having to try to think hard about this.</div><div><br></div><div>So that isn't what I would necessarily propose for LLVM.</div><div><br></div><div>For LLVM, my answer would be "either make tool do what we want, force use of tool, or be consistent and obvious with what we want and then fix it if it leads to performance issue"</div><span class=""><div><br></div></span></div></div></div></blockquote><div>Sure, that's why we have clang-tidy. I propose using push_back everywhere except when temporary is passed, where using emplace_back would simplify by not having to write type name.</div><div>modernize-use-emplace can find places like push_back(Type(a, b, c)) and transform it into emplace_back(a, b, c); </div><div>I hand someone a patches for LLVM and clang that </div><div>1) modifies every push_back to emplace_back</div><div>2) modifies every emplace_back to push_back (if it is valid)</div><div>3) modifies every push_back(Type(a, b)) to emplace_back(a, b);</div><div><br></div><div>I actually wanted to do this last time at google because you have some framework to test performance of clang, but I didn't have enough time.</div><div>I only have compared time of running tests vs running tests after 3), and the results were almost the same.</div><div>That's why I think that performance is not concern here.</div><div>So if someone with such framework would like to help me, then we could check if there are any performance wins.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>There is also other reason - emplace_back can't take arguments of some types like bitfields, NULL, static member. Using emplace can also lead to memory leak in case of smart ptrs</div><div>"</div><div><pre style="overflow-x:auto;overflow-y:hidden;border-color:rgb(12,55,98);border-style:dotted;border-width:thin;margin-top:0px;margin-bottom:12px;padding:0.8em;background-color:rgb(240,240,240);color:rgb(51,51,51)"><span class="m_-6903527064178528341m_4359502425263540212gmail-k" style="color:rgb(0,112,32);font-weight:bold">auto</span> <span class="m_-6903527064178528341m_4359502425263540212gmail-o" style="color:rgb(102,102,102)">*</span><span class="m_-6903527064178528341m_4359502425263540212gmail-n">ptr</span> <span class="m_-6903527064178528341m_4359502425263540212gmail-o" style="color:rgb(102,102,102)">=</span> <span class="m_-6903527064178528341m_4359502425263540212gmail-k" style="color:rgb(0,112,32);font-weight:bold">new</span> <span class="m_-6903527064178528341m_4359502425263540212gmail-kt" style="color:rgb(144,32,0)">int</span><span class="m_-6903527064178528341m_4359502425263540212gmail-p">(</span><span class="m_-6903527064178528341m_4359502425263540212gmail-mi" style="color:rgb(64,160,112)">1</span><span class="m_-6903527064178528341m_4359502425263540212gmail-p">);</span>
<span class="m_-6903527064178528341m_4359502425263540212gmail-n">v</span><span class="m_-6903527064178528341m_4359502425263540212gmail-p">.</span><span class="m_-6903527064178528341m_4359502425263540212gmail-n">push_back</span><span class="m_-6903527064178528341m_4359502425263540212gmail-p">(</span><span class="m_-6903527064178528341m_4359502425263540212gmail-n">std</span><span class="m_-6903527064178528341m_4359502425263540212gmail-o" style="color:rgb(102,102,102)">::</span><span class="m_-6903527064178528341m_4359502425263540212gmail-n">unique_ptr</span><span class="m_-6903527064178528341m_4359502425263540212gmail-o" style="color:rgb(102,102,102)"><</span><span class="m_-6903527064178528341m_4359502425263540212gmail-kt" style="color:rgb(144,32,0)">in<wbr>t</span><span class="m_-6903527064178528341m_4359502425263540212gmail-o" style="color:rgb(102,102,102)">></span><span class="m_-6903527064178528341m_4359502425263540212gmail-p">(</span><span class="m_-6903527064178528341m_4359502425263540212gmail-n">ptr</span><span class="m_-6903527064178528341m_4359502425263540212gmail-p">));</span></pre></div><div><p style="text-align:justify;color:rgb(51,51,51);font-family:"dejavu sans",arial,helvetica,sans-serif;font-size:14.4px">This is because replacing it with <code class="m_-6903527064178528341m_4359502425263540212gmail-docutils m_-6903527064178528341m_4359502425263540212gmail-literal" style="background-color:rgb(226,226,226);font-size:1em"><span class="m_-6903527064178528341m_4359502425263540212gmail-pre">emplace_back</span></code> could cause a leak of this pointer if <code class="m_-6903527064178528341m_4359502425263540212gmail-docutils m_-6903527064178528341m_4359502425263540212gmail-literal" style="background-color:rgb(226,226,226);font-size:1em"><span class="m_-6903527064178528341m_4359502425263540212gmail-pre">emplace_back</span></code> would throw exception before emplacement (e.g. not enough memory to add new element).".</p></div></div></blockquote><div><br></div></span><div>This seems, IMHO, not a useful argument for LLVM since it does not try to use exception based error handling to recover.</div><div><br></div></div></div></div></blockquote><div>But there is still problem with initializer lists, bit-fields, static members and nulls. When I was testing this check it actually found many of these cases in LLVM (specially bit fields and initializer lists).</div><div><br></div><div> </div></div></div></div>