Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh closed this revision. alexfh added a comment. Committed revision 245429. http://reviews.llvm.org/D10933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh added a comment. Another couple of comments (fine for a follow-up). Comment at: test/clang-tidy/readability-identifier-naming.cpp:72 @@ +71,3 @@ + +namespace FOO_NS { +// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: invalid case style for namespace 'FOO_NS' [readability-iden

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm updated this revision to Diff 32304. berenm added a comment. Alright! Here is the latest revision then, with FIXMEs comments mentioning this missing feature. I will work on implementing it and come back to you when i've got something working. Thanks for your time and your advices! htt

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh added a comment. In http://reviews.llvm.org/D10933#225696, @berenm wrote: > Indeed, I probably don't have rights on Clang/LLVM repositories. > > I have updated the patch with stricter tests - and found a missing break > statement (`clang-tidy/readability/IdentifierNamingCheck.cpp:201`).

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm updated this revision to Diff 32294. berenm added a comment. Indeed, I probably don't have rights on Clang/LLVM repositories. I have updated the patch with stricter tests - and found a missing break statement (`clang-tidy/readability/IdentifierNamingCheck.cpp:201`). I also realized that

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh added a comment. In http://reviews.llvm.org/D10933#225678, @berenm wrote: > In http://reviews.llvm.org/D10933#225671, @alexfh wrote: > > > Looks good with a couple of comments. Tell me if you need me to submit the > > patch for you, once you address the comments. > > > If you tell me how

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm added a comment. In http://reviews.llvm.org/D10933#225671, @alexfh wrote: > Looks good with a couple of comments. Tell me if you need me to submit the > patch for you, once you address the comments. If you tell me how I should proceed, I can maybe do it myself (do I just have to send t

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. Looks good with a couple of comments. Tell me if you need me to submit the patch for you, once you address the comments. Thank you for the awesome new check! Comment at: cla

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm marked 6 inline comments as done. berenm added a comment. http://reviews.llvm.org/D10933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm updated this revision to Diff 32277. berenm added a comment. Here is an updated version with the latest comments fixed. http://reviews.llvm.org/D10933 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/IdentifierNamingCheck.cpp clang-tidy/readability/IdentifierNami

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm marked 2 inline comments as done. Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:166 @@ +165,3 @@ + static llvm::Regex Splitter( + "(([a-z0-9A-Z]*)(_+)|([A-Z]?[a-z0-9]+)([A-Z]|$)|([A-Z]+)([A-Z]|$))"); + alexfh wrote: > Why do you need th

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh added a comment. Sorry for the delay. A few more comments, otherwise looks really nice. Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:166 @@ +165,3 @@ + static llvm::Regex Splitter( + "(([a-z0-9A-Z]*)(_+)|([A-Z]?[a-z0-9]+)([A-Z]|$)|([A-Z]+)([A-Z]|$))"

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm updated this revision to Diff 31447. berenm added a comment. Here is an updated version with some style fixes, and function splits. The styles are still selected the same way as before (no split between finding the best type style and falling back to available style), but there is only th

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh added inline comments. Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:178 @@ +177,3 @@ +if (NamingStyles[Typedef].isSet()) { + KindName = "typedef"; + Style = NamingStyles[Typedef]; berenm wrote: > alexfh wrote: > > berenm wrote:

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm added inline comments. Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:178 @@ +177,3 @@ +if (NamingStyles[Typedef].isSet()) { + KindName = "typedef"; + Style = NamingStyles[Typedef]; alexfh wrote: > berenm wrote: > > alexfh wrote:

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh added inline comments. Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:87 @@ +86,3 @@ + auto const fromString = [](StringRef Str) { +if (Str.equals("any") || Str.equals("aNy_CasE")) + return AnyCase; berenm wrote: > alexfh wrote: > >

Re: [PATCH] D10933: Add new IdentifierNaming check

berenm added a comment. Thanks for the review, I will fix the various style issues and come back with an updated version. Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:87 @@ +86,3 @@ + auto const fromString = [](StringRef Str) { +if (Str.equals("any") || Str

Re: [PATCH] D10933: Add new IdentifierNaming check

alexfh added a comment. Some initial comments, mostly style-related. It takes some time to get used to the coding style used in LLVM/Clang. One notable thing: though we use `auto` sometimes, we don't use the almost-always-auto style. Please see the inline comments for specific cases.