lebedev.ri added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:28
+// 2. it is immutable, the way it was constructed it will stay.
+template <typename T, unsigned SmallSize> class ImmutableSmartSet {
+ ArrayRef<T> Vector;
----------------
aaron.ballman wrote:
> "Smart" is not a descriptive term. This seems like it's an
> `ImmutableSmallSet`?
I was really trying to convey the fact that it can either be in small-sized
mode (linear search over vector),
or in set mode (constructed with a large size, search in set).
But then, SmallVector also can be in small-sized mode (stack allocation), and
large-sized mode (heap).
So here i guess `ImmutableSmallSet` name should work..
================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:70
+
+template <typename T, unsigned SmallSize> class SmartSmallSetVector {
+public:
----------------
aaron.ballman wrote:
> Same complaint here about `Smart` -- should probably just be a
> `SmallSetVector`.
There already is a `SmallSetVector`, and it does have different semantics,
i've added a code comment explaining their differences, PTAL.
For now, i'd prefer to keep this as-is, but afterwards i suspect
i might be able to "upstream" this into `SmallSetVector` itself,
thus getting rid of `SmartSmallSetVector`.
Let me know if this makes sense?
================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:203
+ Decl *D = N->getDecl();
+ diag(D->getLocation(), "function '%0' is part of call graph loop")
+ << cast<NamedDecl>(D)->getName();
----------------
aaron.ballman wrote:
> I think "call graph loop" is a bit much for a diagnostic -- how about `%0 is
> within a recursive call chain` or something along those lines?
Sure, that should be fine too.
================
Comment at: clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp:204
+ diag(D->getLocation(), "function '%0' is part of call graph loop")
+ << cast<NamedDecl>(D)->getName();
+ }
----------------
aaron.ballman wrote:
> Drop the call to `getName()` and remove the single quotes from the diagnostic
> around `%0`, and just pass in the `NamedDecl` -- the diagnostics engine will
> do the right thing (and it gives better names for strange things like
> operators).
Nice!
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-recursion.rst:8
+that are loops), diagnoses each function in the cycle,
+and displays one example of possible call graph loop (recursion).
----------------
aaron.ballman wrote:
> Eugene.Zelenko wrote:
> > It'll be reasonable to add links to relevant coding guidelines.
> Agreed.
Is this better?
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp:153
+// CHECK-NOTES: :[[@LINE-7]]:3: note: Frame #1: function 'boo' calls function
'bar' here:
+// CHECK-NOTES: :[[@LINE-14]]:18: note: Frame #2: function 'bar' calls
function 'boo' here:
+// CHECK-NOTES: :[[@LINE-15]]:18: note: ... which was the starting point of
this call graph
----------------
NoQ wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > NoQ wrote:
> > > > Aha, yeah, the warning is present, i guess that part is correct, but
> > > > look how your note `function 'bar' calls function 'boo' here:` doesn't
> > > > really point into the body of 'bar'. In this case it still makes sense
> > > > because it's easy to see that 'foo' is called from 'bar', but the chain
> > > > of default arguments may be arbitrarily long (what if 'boo' has yet
> > > > another default argument?). You might want to add a separate facility
> > > > just to handle this edge case.
> > > To make this more readable, the diag is:
> > > ```
> > > /builddirs/llvm-project/build-Clang9-unknown/tools/clang/tools/extra/test/clang-tidy/checkers/Output/misc-no-recursion.cpp.tmp.cpp:138:5:
> > > warning: function 'boo' is part of call graph loop [misc-no-recursion]
> > > int boo();
> > > ^
> > > /builddirs/llvm-project/build-Clang9-unknown/tools/clang/tools/extra/test/clang-tidy/checkers/Output/misc-no-recursion.cpp.tmp.cpp:140:6:
> > > warning: function 'bar' is part of call graph loop [misc-no-recursion]
> > > void bar() {
> > > ^
> > > /builddirs/llvm-project/build-Clang9-unknown/tools/clang/tools/extra/test/clang-tidy/checkers/Output/misc-no-recursion.cpp.tmp.cpp:138:5:
> > > note: Example call graph loop, starting from function 'boo'
> > > int boo();
> > > ^
> > > /builddirs/llvm-project/build-Clang9-unknown/tools/clang/tools/extra/test/clang-tidy/checkers/Output/misc-no-recursion.cpp.tmp.cpp:145:3:
> > > note: Frame #1: function 'boo' calls function 'bar' here:
> > > bar();
> > > ^
> > > /builddirs/llvm-project/build-Clang9-unknown/tools/clang/tools/extra/test/clang-tidy/checkers/Output/misc-no-recursion.cpp.tmp.cpp:139:18:
> > > note: Frame #2: function 'bar' calls function 'boo' here:
> > > void foo(int x = boo()) {}
> > > ^
> > > ```
> > > Am i looking at this wrong, or all the info is present?
> > Edit: right, `bar->foo` edge is not shown there.
> Yup. And, well, semantically it's kind of correct, because it's not `foo()`
> that calls `boo()`, but it's `bar()`'s responsiblity to call `boo()` directly
> before calling `foo()`. So it's more like a three-way relationship:
> ```
> misc-no-recursion.cpp.tmp.cpp:139:18: note: Frame #1: function 'bar' calls
> function 'boo' here:
> void foo(int x = boo()) {}
> ^
> misc-no-recursion.cpp.tmp.cpp:141:7: note: ...in order to compute default
> argument for function 'foo' here:
> foo();
> ^
> ```
> So i'd imagine `CallRecord`s potentially containing arbitrarily long chains
> of backreferences from a default argument initializer to the respective
> `CXXDefaultArgExpr` that we're currently visiting (which may in turn be part
> of another default argument initializer, given that those are arbitrary
> expressions).
>
> (it's of course up to you whether you want to deal with this immediately or
> later, i'm just ranting, don't mind me)
I agree that this isn't perfect, but i'm not very sure how that information
could be added to the call stack.
I think i'll leave this as-is for now..
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-no-recursion.cpp:127
+}
+
+// CHECK-NOTES: :[[@LINE-14]]:13: warning: function
'indirect_recursion_with_depth2' is part of call graph loop [misc-no-recursion]
----------------
JonasToth wrote:
> Does the check find recursion through function pointers? (Probably not?
> Should be noted as limitation).
>
> Please add cases from lambdas. And cases where recursion happens in templates
> / only with one of multiple instantiations.
This indeed gets blinded by function pointers, test added.
As for lambdas, i'm not sure what specifically you mean?
The lambda itself can't be recursive i think?
https://godbolt.org/z/GYLRnB
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72362/new/
https://reviews.llvm.org/D72362
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
