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

[Piotr Padlewski via cfe-dev]

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);

2016-12-30 12:26 GMT+01:00 Piotr Padlewski <piotr.padlewski at gmail.com>:

>
>
> 2016-12-30 11:34 GMT+01:00 Chandler Carruth <chandlerc at gmail.com>:
>
>> 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.
>>
>>  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170109/04661858/attachment.html>