NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:123
state = state->BindExpr(B, LCtx, Result);
}
----------------
Charusso wrote:
> I have seen we are producing tons of Unknowns and I am still not sure where
> they go, but even if I remove that condition these Unknowns will not shown up
> in the ExplodedGraph. I believe that should be the correct behavior.
Unknowns don't show up in the Environment, but they do show up in the Store.
================
Comment at: clang/test/Analysis/symbol-escape.cpp:5-6
+// expected-no-diagnostics
+
#include <stdint.h>
----------------
Charusso wrote:
> NoQ wrote:
> > I think something went wrong while uploading the patch. This diff should
> > add this whole test file, not update it.
> This is a test-driven-development-driven patch, first make the test fail,
> then make it pass.
Sounds nice in your local git but for phabricator it's definitely better to
upload the exact thing that you're going to commit. It is assumed that the
tests would fail without the patch and will pass with the patch, and the
information on how exactly did the tests fail previously would look nice in the
description. Sometimes some (but usually not all) of the tests are added just
because they were discovered to be nice tests related to the patch but not
because they failed before the patch, which is also fine and should ideally be
mentioned in the description.
================
Comment at: clang/test/Analysis/symbol-escape.cpp:7
+
#include <stdint.h>
----------------
Charusso wrote:
> NoQ wrote:
> > Relying on everybody's system headers is super flaky, don't do this in
> > tests.
> >
> > Please define stuff that you use directly like other tests do:
> > ```lang=c++
> > typedef unsigned __INTPTR_TYPE__ uintptr_t;
> > ```
> It is a predefined header in Modules so Windows will not break.
>
> Please note that only one of the test files (malloc.c) use that definition,
> most of them using that I have used in D62926, so I believe this header is
> the correct one as it is in four tests and Windows-approved by D62926.
Mmm, ok, if it's an internal file then i guess it might be non-flaky. But even
then, if it's something more complicated than the definition of `uintptr_t`,
then your test will be affected by the contents of this file, which may change.
I'd slightly prefer to reduce the amount of such stuff in our tests simply for
keeping them minimal.
================
Comment at: clang/test/Analysis/symbol-escape.cpp:15
~static_cast<uintptr_t>(0x1)) |
(reinterpret_cast<uintptr_t>(Bar) & 0x1));
(void)Bar;
----------------
Charusso wrote:
> NoQ wrote:
> > `Bar` is reduced to one bit here. It's a legit leak. I think you meant to
> > swap `Foo` and `Bar`.
> I assumed that we are doing our low-bits magic, where we have two identical
> objects. The SymRegion sets up the HeapSymRegion's property, where no leak
> happen. We just could change the first low-bit and set up some crazy flag,
> like `isChecked()`, so we do not lost the tracking. Also we have four bits to
> change, so there is no edge case going on.
I guess just flip a single bit like i did in my test?
================
Comment at: clang/test/Analysis/symbol-escape.cpp:12
C *payload(C *Foo) {
C *Bar = new C();
----------------
Let's make fancier names, like, dunno, `test_escape_into_bitwise_ops` and
`test_indirect_escape_into_bitwise_ops`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63720/new/
https://reviews.llvm.org/D63720
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
