NoQ added a comment.
Tests are still not working - they pass now, but they don't actually test
anything. Please make sure that tests actually work. Which means that
1. Tests pass when you run `make -j4 check-clang-analysis`;
2. Tests start failing when you change your code or expected-warning directives.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:110
+ auto ThiSVal =
+ State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame()));
+ const MemRegion *Reg = ThiSVal.getAsRegion();
----------------
wangxindsb wrote:
> NoQ wrote:
> > I don't think this is working. CXXThisRegion is never a region of a C++
> > object; it's the segment of the stack where the pointer is passed, you need
> > to dereference its value to get the actual object region.
> >
> > Probably tests for the visitor might make things more clear.
> ```
> class Y {
> public:
> virtual void foobar();
> void fooY() {
> F f1;
> foobar();
> }
> Y() {
> fooY();
> }
> };
>
> ```
> I thought this test is for this situation. If the visitor is correct, it will
> issued "Called from this constructor 'Y'", else it will issued "Called from
> this constructor 'F'".
>
Right, i didn't notice `getSVal()`; seems correct.
This test doesn't verify anything though, because it has no expected-warnings.
Even if it had, it wouldn't verify where the visitor diagnostic appears,
because without a more specific `-analyzer-output` option (eg. `=text`), the
analyzer wouldn't emit visitor notes, so the `-verify` option would not be able
to verify them.
So by visitor tests i mean adding `-analyzer-output=text` to the run-line and
then adding `expected-note` directives wherever notes are expected.
================
Comment at: lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:124-127
+ if (CD)
+ InfoText = "Called from this constrctor '" + CD->getNameAsString() + "'";
+ else
+ InfoText = "Called from this destrctor '" + DD->getNameAsString() + "'";
----------------
Typo: "constr//**u**//ctor", "destr//**u**//ctor".
Also i guess we need to think about this warning message more carefully anyway.
Like, we already have an "entering function..." event diagnostic piece. Because
the warning always happens inside the function in question, this event would
never be pruned, so it'd always be there. So what do we expect the visitor's
diagnostic piece to say?
I suggest "This [constructor|destructor] of an object of type 'Foo' has not
returned when the virtual method was called". With a probable future
improvement of specifying the name of the object (when eg. it's a variable).
It might even be that later we'd decide that the visitor is not necessary for
this checker. I'm not sure, i guess i'd have to see how it works in practice.
Also, right now the visitor's piece is above the "entering function..." event.
Not sure if having it below, or even inside the constructor, would be better.
================
Comment at: test/Analysis/virtualcall.cpp:1-6
+// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall
+// -analyzer-store region -verify -std=c++11 %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall
+// -analyzer-store region -analyzer-config
+// optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -verify -std=c++11 %s
----------------
Tests are still not working because your auto-format tool has inserted weird
line breaks.
https://reviews.llvm.org/D34275
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
