[cfe-dev] [llvm-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy
Philip Reames via cfe-dev
cfe-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/cfe-dev/attachments/20170113/25fecf00/attachment.html>
More information about the cfe-dev
mailing list