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

Philip Reames via llvm-dev llvm-dev at lists.llvm.org
Fri Jan 13 21:40:13 PST 2017


Can we get at least a summary of any decisions made before action is 
taken?  I'm definitely not reading this thread in detail and I doubt 
others are either.

Philip

On 01/09/2017 10:16 AM, David Blaikie via llvm-dev wrote:
>
>
> On Mon, Jan 9, 2017 at 10:10 AM Piotr Padlewski 
> <piotr.padlewski at gmail.com <mailto:piotr.padlewski at gmail.com>> wrote:
>
>     2017-01-09 17:24 GMT+01:00 David Blaikie <dblaikie at gmail.com
>     <mailto:dblaikie at gmail.com>>:
>
>
>
>         On Mon, Jan 9, 2017 at 6:17 AM Piotr Padlewski via cfe-dev
>         <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>
>             Are there any other comments about changing style guide?
>             I would like to add points like
>
>             - prefer "using' instead of "typedef"
>             - use default member initialization
>             struct A {
>               void *ptr = nullptr;
>             };
>
>
>             (instead of doing it in constructor)
>
>             - use default, override, delete
>             - skip "virtual" with override
>
>             The last point is to get to consensus with
>
>             push_back({first, second})
>             or
>             emplace_back(first ,second);
>
>
>
>         It might be a bit noisy, but I'd be inclined to start a
>         separate thread for each of these on llvm-dev with a clear
>         subject line relevant to each one. So they don't get lost and
>         some of them don't get drowned out by the discussion of
>         others, etc.
>
>     Sure, I can do it, but at least now I don't see much of attention
>     of people to any of the subject, so I would not like to start 5
>     empty threads.
>     I guess as long as the thread is not getting noisy I will keep
>     only one. Does it sound ok?
>
>
> Perhaps - if no one else pipes up *shrug*
>
> If that's the case, I'd at least suggest submitting the changes to the 
> style guide separately with clear subject/titles so people reading 
> commits might see/notice/discuss there.
>
> FWIW: I'm also in favor of push_back where valid. Though I'm not sure 
> it's so much a matter of votes, but justification, etc. As for the 
> others - sure, they all sound good to me.
>
> Also - once these are in the style guide there's still a separate 
> discussion to be had about whether automated cleanup is worthwhile & 
> how best to go about that sort of thing.
>
>
>
>             2016-12-30 12:26 GMT+01:00 Piotr Padlewski
>             <piotr.padlewski at gmail.com
>             <mailto:piotr.padlewski at gmail.com>>:
>
>
>
>                 2016-12-30 11:34 GMT+01:00 Chandler Carruth
>                 <chandlerc at gmail.com <mailto:chandlerc at gmail.com>>:
>
>                     On Fri, Dec 30, 2016 at 2:08 AM Piotr Padlewski
>                     via cfe-dev <cfe-dev at lists.llvm.org
>                     <mailto: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.
>
>                  This sounds extremely reasonable.
>
>                     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.
>
>                 Calling emplace_back with 0 or multiple arguments is a
>                 clear way of saying "this constructor takes multiple
>                 arguments".
>                 We can do it with initializer list with easy way like:
>                 v.emplace_back()        == v.push_back({})
>                 v.emplace_back(a, b ,c) == v.push_back({a, b, c})
>
>                 I personally never liked the initializer syntax
>                 because of tricky casees like:
>
>                 vector<string> v{{"abc", "def"}};
>                 Which is equivalent of
>                 vector<string> v = {std::string("abc", "def")};
>                 That will call std::string ctor with 2 iterators
>                 likely crashing, and putting same string might gives
>                 us empty string.
>
>                 In this case programmer probably meant
>                 std::vector<std:string> v({"abc", "def"});
>                 or
>                 std::vector<std::string> v = {"abc", "def"};
>
>                 But this case is not possible to mess up with
>                 push_back (in the case of vector<vector<string>> or
>                 something). At least I hope it is not.
>                 So avoiding braces is my personal preference. It is
>                 fine for me if we would choose to prefer
>                 'v.push_back({a, b, c})' instead of 'v.emplace_back(a,
>                 b, c)', ofc as long as most of the community would
>                 prefer first form to the second :)
>
>
>                     [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. =]
>
>
>                 Piotr
>
>
>             _______________________________________________
>             cfe-dev mailing list
>             cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>             http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170113/25fecf00/attachment.html>


More information about the llvm-dev mailing list