David Blaikie via llvm-dev
2017-Jan-09 16:24 UTC
[llvm-dev] [cfe-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy
On Mon, Jan 9, 2017 at 6:17 AM Piotr Padlewski via cfe-dev < 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.> > 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 > > > _______________________________________________ > cfe-dev mailing list > cfe-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170109/ac0e683d/attachment.html>
Piotr Padlewski via llvm-dev
2017-Jan-09 18:10 UTC
[llvm-dev] [cfe-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy
2017-01-09 17:24 GMT+01:00 David Blaikie <dblaikie at gmail.com>:> > > On Mon, Jan 9, 2017 at 6:17 AM Piotr Padlewski via cfe-dev < > 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?> >> 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 >> >> >> _______________________________________________ >> cfe-dev mailing list >> cfe-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170109/b030bc20/attachment.html>
David Blaikie via llvm-dev
2017-Jan-09 18:16 UTC
[llvm-dev] [cfe-dev] Modernizing LLVM Coding Style Guide and enforcing Clang-tidy
On Mon, Jan 9, 2017 at 10:10 AM Piotr Padlewski <piotr.padlewski at gmail.com> wrote: 2017-01-09 17:24 GMT+01:00 David Blaikie <dblaikie at gmail.com>: On Mon, Jan 9, 2017 at 6:17 AM Piotr Padlewski via cfe-dev < 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>: 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 _______________________________________________ cfe-dev mailing list cfe-dev at lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170109/828fa08e/attachment.html>