Charusso added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:123
state = state->BindExpr(B, LCtx, Result);
}
----------------
NoQ wrote:
> 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.
Oh, I really wanted to see them in the Environment. Thanks for the
clarification!
================
Comment at: clang/test/Analysis/symbol-escape.cpp:5-6
+// expected-no-diagnostics
+
#include <stdint.h>
----------------
NoQ wrote:
> 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.
Lesson learned, thanks for the idea!
================
Comment at: clang/test/Analysis/symbol-escape.cpp:7
+
#include <stdint.h>
----------------
NoQ wrote:
> 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.
Hm, I hope that `malloc.c` one works.
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
