lebedev.ri added a comment. In https://reviews.llvm.org/D44602#1041153, @JonasToth wrote:
> Looks fine to me, but Aaron or someone else must accept :) Thanks for review! In https://reviews.llvm.org/D44602#1041153, @JonasToth wrote: > Just out of curiosity: The decomposition visit is for structured binding and > the binding visit for range based for? Are there other places where binding > occurs, for example in some template stuff? > Just asking if this might hit me in a other check :D The comment i added is actually wrong. The range-based for just works anyway. The `VisitBindingDecl()` is needed to get all the 'variables' declared in structured binding. But as you can see in https://godbolt.org/g/be6Juf, the `BindingDecl`'s are wrapped into `DecompositionDecl`. And `DecompositionDecl` is actually inherited from `VarDecl`, https://github.com/llvm-mirror/clang/blob/b031fdc9b7dbd9c7f942d8060a4f00d63f3c9af2/include/clang/AST/DeclCXX.h#L3824-L3825, so we count the `DecompositionDecl` in `VisitVarDecl()`, thus we need a `VisitDecompositionDecl()` that subtracts it. This may be the wrong/incomplete solution. ================ Comment at: docs/ReleaseNotes.rst:125 + + Flags functions exceeding this number of variables declared in the body. + ---------------- JonasToth wrote: > I would rephrase this a little to: > > ``` > Flags function bodies exceeding this number of declared variables. > ``` I agree that it looks gibberish. Reworded a little. Probably still not ideal. ================ Comment at: test/clang-tidy/readability-function-size.cpp:118 } + // CHECK-MESSAGES: :[[@LINE-22]]:6: note: 6 variables (threshold 1) } ---------------- JonasToth wrote: > Why is this check here and not on line 100 like the other conditions? It can not be there, the `note: nesting level <> starts here (threshold 2)` are outputted before `note: <> variables (threshold 1)`, and filecheck respects the order of `// CHECK-MESSAGES:` ================ Comment at: test/clang-tidy/readability-function-size.cpp:153 +// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0) +void variables_1(int i) { + int j; ---------------- JonasToth wrote: > Could you please add a test for function having more parameters then allowed > variables, but no variables? It's already checked in `foo7()`, but sure. ================ Comment at: test/clang-tidy/readability-function-size.cpp:180 +// CHECK-MESSAGES: :[[@LINE-6]]:6: note: 2 variables (threshold 1) +void variables_5() { + for (int i;;) ---------------- JonasToth wrote: > This testfile is not C++17 right now, but do you know how structured bindings > are handled? Maybe its worth adding another file for C++17 testing it. > > Could you add a little test for range based for loops? > This testfile is not C++17 right now, but do you know how structured bindings > are handled? Maybe its worth adding another file for C++17 testing it. Nice catch! > Could you add a little test for range based for loops? Added. I expected them to not work (after a quick look at AST), but apparently they just work.. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44602 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
