<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 29, 2016 at 11:46 AM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br></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" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Dec 29, 2016, at 11:20 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-6648727749751248484Apple-interchange-newline gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Thu, Dec 29, 2016 at 10:04 AM Mehdi Amini via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="gmail_msg" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" 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 class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Dec 29, 2016, at 5:54 AM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" class="gmail_msg" target="_blank">dberlin@dberlin.org</a>> wrote:</div><br class="gmail_msg m_-6648727749751248484m_1399359644854761192Apple-interchange-newline"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><blockquote class="gmail_quote gmail_msg" 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 dir="ltr" class="gmail_msg"><br class="gmail_msg"><div class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">(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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><div class="gmail_msg">I agree that “people suck at deciding”, me in the first place in the line, and that’s why I like clear and good guideline and stick to it unless "demonstration of real benefit”.</div><div class="gmail_msg">I also think we can have clear guideline that are different from “always use emplace_back” or “always use push_back”, like Piotr is suggesting.</div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">I don't think Piotr is suggesting "always use push_back" but "always use push_back when it's valid" & I'd second this.<br class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">Define “valid”? Just that it will compile?</div></div></div></blockquote><div><br></div><div>Yep</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" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">To show a simpler example:<br class="gmail_msg"><br class="gmail_msg">std::unique_ptr<T> u(v);<br class="gmail_msg"><br class="gmail_msg">std::unique_ptr<T> u = v;<br class="gmail_msg"><br class="gmail_msg">I'd generally advocate for using copy init (the second form - it doesn't copy in this case, it moves) over direct init (the first form) because it's more legible - copy init can only execute implicit ctor operations, whereas direct init can invoke implicit and explicit operations. So from a readability perspective seeing the latter is easier than seing the former - I know the operation is further constrained to simpler/less interesting operations (eg: 'v' can't be a raw pointer in the second form, it might be (& then I have to worry about whether that's an ownership transfer that's intended), etc)<br class="gmail_msg"><br class="gmail_msg">& push_back V emplace_back is the same choice - push_back means only implicit ops are used and so it's more readable</div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">I don’t see what’s more readable about “only implicit ctor”.</div></div></div></blockquote><div><br></div><div>It limits the set of operations that can be performed by the code. So when I read it there's less I have to think about/consider. (& specifically, the implicit operations tend to be simpler/safer/more obvious - copy/move or operations that are similar - not complex/interesting things like "taking ownership from a raw pointer" or "creating a vector of N elements")</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" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"> Emplace is very explicit that we intended to construct an object, I don’t understand what hurts readability here?</div></div></div></blockquote><div><br></div><div>Going back to the example above, given the following two lines:<br><br class="inbox-inbox-Apple-interchange-newline">std::unique_ptr<T> u(foo());<br>std::unique_ptr<T> u = foo();<br><br>(& the equivalent: emplace_back(foo()) V push_back(foo()) for a vector of unique_ptr)<br><br>the copy init/push_back are simpler to read because they aren't as powerful - I don't have to wonder if something is taking ownership there (or if I'm creating a vector of N ints, etc). I know it's a simple/obvious operation, generally (because others shouldn't be implicit).</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" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">, emplace_back requires more careful attention.<br class="gmail_msg"><br class="gmail_msg">I think this is a reasonably good stylistic rule - but I'm happy leaving an open to judgment for sure - there might be readability reasons that an emplace_back may work better in some cases.<br class="gmail_msg"><br class="gmail_msg">I'd think of this like "else after return" - we don't have any enforcement, sometimes we make judgment about it being better (for consistency - sometimes there's a pattern such that else makes the code more readable), we haven't gone back to do any cleanup of the codebase, but it's pretty consistently applied/desired in code review.<br class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">I have a different impression: we are actively cleaning the codebase with tidy-checks. </div></div></div></blockquote><div><br>I think that's where Danny is pushing back - I'm on the fence about that. So I'd push back on the automated cleanup (& have done so on several occasions as these sort of patches have been sent) & encourage efforts to provide the tools (like clang-tidy integration for patches and editors) to avoid mistakes going forward in an advisory (rather than mandatory) way.<br> </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" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">For instance look for `git log --author=Zelenko`.</div><div class="gmail_msg">Another example is that a few months ago the entire LLDB codebase has been formatted with clang-format, after marking the specific places where it was not desirable (tables for instance) with comment to disable clang-format.</div></div></div></blockquote><div><br>*nod* Only because it was so far out of LLVM's style (by design originally). LLVM's generally "close enough" that the cleanup isn't desired/intended.<br><br>I'd say the same for this particular instance - implicit V explicit ctor calls. Mostly we write them using copy init, not direct init. I suspect push_back/emplace_back is similar (if only due to history - and perhaps due to my/other code review feedback encouraging push_back when possible in reviews that might otherwise be going to more emplace_back) so I'm not sure I care too much about the cleanup.<br><br>I think the issue Danny was getting at - which I agree - is unless we have good tools in place to not make these mistakes again going forward (I think clang-format has sort of reached that point - it's got easy integration in editors and source control systems) then there's limited merit in doing the cleanup. +1 to that.<br><br>If we have good integration for clang-tidy, then I'd be more OK with doing cleanup.<br><br>Either way (good or not integration) - if we have some people who have ways of using clang-tidy in their development process, I'd say it's worth having a project-wide clang-tidy config. I'd vote for putting "use implicit-only ops (like copy init and push_back) over explicit ops (like direct init and emplace_back) where both compile" in that list.<br><br>- Dave<br> </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" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">— </div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">Mehdi</div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg">All that said - sure, I'd like to have tools to help me get this right (else after return and "prefer copy init style over direct init") & make that really easy to do.<br class="gmail_msg"><br class="gmail_msg">- Dave</div><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" 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 class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">That said, I’m not convinced the relative “ugliness" of emplace_back  (I’m not sure what’s ugly there, I only see it as a habit to take)  vs push_back is enough to justify a set of rules to decide between the two, I’d be fine with “always using emplace_back” in the name of “simple rule is better when possible".</div><div class="gmail_msg"><br class="gmail_msg"></div><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"></div></div></div></div></blockquote></div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">So that isn't what I would necessarily propose for LLVM.</div><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div></div></blockquote></div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">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></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">+1</div><div class="gmail_msg">Ultimately whatever we do is not practical if it can’t be done by a tool.</div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Not sure I follow this - we have lots of things we don't currently do with a tool that's still part of our style & stuff we try to catch with code review, etc. (else after return is my best example)</div><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" 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 class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" 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 dir="ltr" class="gmail_msg"><div class="gmail_msg">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 class="gmail_msg">"</div><div class="gmail_msg"><pre class="gmail_msg" style="overflow-x:auto;overflow-y:hidden;border:thin dotted rgb(12,55,98);margin-top:0px;margin-bottom:12px;padding:0.8em;background-color:rgb(240,240,240);color:rgb(51,51,51)"><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-k gmail_msg" style="color:rgb(0,112,32);font-weight:bold">auto</span> <span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-o gmail_msg" style="color:rgb(102,102,102)">*</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-n gmail_msg">ptr</span> <span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-o gmail_msg" style="color:rgb(102,102,102)">=</span> <span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-k gmail_msg" style="color:rgb(0,112,32);font-weight:bold">new</span> <span class="gmail_msg m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-kt" style="color:rgb(144,32,0)">int</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-p gmail_msg">(</span><span class="gmail_msg m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-mi" style="color:rgb(64,160,112)">1</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-p gmail_msg">);</span>
<span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-n gmail_msg">v</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-p gmail_msg">.</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-n gmail_msg">push_back</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-p gmail_msg">(</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-n gmail_msg">std</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-o gmail_msg" style="color:rgb(102,102,102)">::</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-n gmail_msg">unique_ptr</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-o gmail_msg" style="color:rgb(102,102,102)"><</span><span class="gmail_msg m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-kt" style="color:rgb(144,32,0)">int</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-o gmail_msg" style="color:rgb(102,102,102)">></span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-p gmail_msg">(</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-n gmail_msg">ptr</span><span class="m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-p gmail_msg">));</span></pre></div><div class="gmail_msg"><p class="gmail_msg" 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="gmail_msg m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-docutils m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-literal" style="background-color:rgb(226,226,226);font-size:1em"><span class="gmail_msg m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-pre">emplace_back</span></code> could cause a leak of this pointer if <code class="gmail_msg m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-docutils m_-6648727749751248484m_1399359644854761192m_4359502425263540212gmail-literal" style="background-color:rgb(226,226,226);font-size:1em"><span class="gmail_msg m_-6648727749751248484m_1399359644854761192m_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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">This seems, IMHO, not a useful argument for LLVM since it does not try to use exception based error handling to recover.</div></div></div></div></div></blockquote></div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"></div><br class="gmail_msg"><div class="gmail_msg">This, and also we should not write code like that (naked pointers, calling new), using make_unique<> for instance would prevent any such situation. Passing a raw pointer to a container of unique_ptr can be flagged by a tool.</div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Sure - but it generalizes further than that, it's just a nice simple example.<br class="gmail_msg"><br class="gmail_msg">std::vector<int> v(i);<br class="gmail_msg"><br class="gmail_msg">'i' could be an int & this would make a vector of that many ints, or it could be another vector & this is making a copy, etc.<br class="gmail_msg"><br class="gmail_msg">std::vector<int> v = i;<br class="gmail_msg"><br class="gmail_msg">I know taht's not creating a vector of 'i' many ints.<br class="gmail_msg"><br class="gmail_msg">Yeah, not a great example either - but the general idea applies, the latter form is less powerful & so is easier to read because you don't have to worry/think about it as much.<br class="gmail_msg"><br class="gmail_msg">I think we mostly do this sort of thing out of habit anyway - but codifying it (& if possible, tools to help) could be nice.<br class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" 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 class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">— </div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg">Mehdi</div></div>_______________________________________________<br class="gmail_msg">cfe-dev mailing list<br class="gmail_msg"><a href="mailto:cfe-dev@lists.llvm.org" class="gmail_msg" target="_blank">cfe-dev@lists.llvm.org</a><br class="gmail_msg"><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a></blockquote></div></div></div></blockquote></div></div></blockquote></div></div>