[llvm-dev] [cfe-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy

[Chandler Carruth via llvm-dev]

On Fri, Dec 30, 2016 at 2:08 AM Piotr Padlewski via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> Thanks for very accurate responses.
> - I totally agree what Dave and Chandler said about explicit and implicit
> operations, this is what I meant my first email.
>   I believe there are places like
>     v.emplace_back(A, B);
>   istead of
>     v.push_back(make_pair(A, B));b
>   That can make code simpler.
>

Do you have examples? The only ones i can come up with are the ones where
the push_back variant literally can't compile because the type isn't
movable.

Perhaps it would be useful to break down categories of can happen here...

Case 1: there is one object already -- this is a *conversion* of a type.
- If the author of the conversion made it *implicit*, then 'v.push_back(x)'
just works.
- If the author of the conversion made it *explicit* I would like to see
the name of the type explicitly: 'v.push_back(T(x))'.

Case 2a: There is a collection of objects that are being composed into an
aggregate. We don't have any interesting logic in the constructor, it takes
an initializer list.
- This should work with 'v.push_back({a, b, c})'
- If it doesn't today, we can fix the type's constructors so that it does.
- Using 'emplace_back' doesn't help much -- you still need {}s to form the
std::initializer_list in many cases. Pair and tuple are somewhat unusual in
not requiring them.

Case 2b: A specific constructor needs to be called with an argument list.
These arguments are not merely being aggregated but are inputs to a
constructor that contains logic.
- This is analogous to a case called out w.r.t. '{...}' syntax in the
coding standards[1]
- Similar to that rule, I would like to see a *call to the constructor*
rather than hiding it behind 'emplace_back' as this is a function with
interesting logic.
- That means i would write T(a, b, c) anyways, and 'v.push_back(T(a, b,
c))' works.

[1]:
http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor

Case 3: Passing objects of type 'T' through 'push_back' fails to compile
because they cannot be copied or moved.
- You *must* use 'emplace_back' here. No argument (obviously).

My experience with LLVM code and other codebases is that case 3 should be
extremely rare. The intersection of "types that cannot be moved or copied"
and "types that you put into containers" is typically small.


Anyways, I don't disagree with this point with a tiny fix:

> I think in cases like this we can leave it for judgement of contributor.
>
*or reviewer*. ;]

I continue to think exceptions can be made in rare cases when folks have
good reasons. But I expect this to be quite rare. =]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161230/84370edf/attachment.html>