dcoughlin added a comment.
Thanks for iterating on the patch! Some comments in-line.
Comment at: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:569
+ // allocating functions initialized to nullptr, which will never equal a
+ //non-null IdentifierInfo*, and nev
ariccio added a comment.
Nevermind, the order is correct!
https://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
On Mon, Aug 8, 2016 at 8:13 PM, Alexander Riccio wrote:
> ariccio added a comment.
>
> In https://reviews.llvm.org/D18073#504216, @dcoughlin wrote:
>
>> No. The identifier info will be null for C++ operators.
>
>
> I assume you mean `operator new/new[]/delete/delete[]
Correct.
~Aaron
>
>> > Thu
ariccio added a comment.
In https://reviews.llvm.org/D18073#504216, @dcoughlin wrote:
> No. The identifier info will be null for C++ operators.
I assume you mean `operator new/new[]/delete/delete[]
> > Thus, when`! isWindowsMSVCEnvironment`, I leave the Windows-only memory
> > allocating func
dcoughlin added a comment.
In https://reviews.llvm.org/D18073#437171, @ariccio wrote:
> I should elaborate. The principle of operation of this latest patch is that
> the `FunctionDecl` in `IsCMemFunction` should never return a `nullptr`
> `IdentifierInfo*` from `getIdentifier` (is that a valid
ariccio added a comment.
Bump?
https://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a comment.
Bump?
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ariccio added a comment.
I should elaborate. The principle of operation of this latest patch is that the
`FunctionDecl` in `IsCMemFunction` should never return a `nullptr`
`IdentifierInfo*` from `getIdentifier` (is that a valid assumption?)... Thus,
when`! isWindowsMSVCEnvironment`, I leave the
ariccio updated this revision to Diff 55774.
ariccio added a comment.
Only check for MSVC-specific functions when actually building FOR MSVC (i.e.
`isWindowsMSVCEnvironment`).
Sidenote: is there any reason why none of the `ASTContext&`s are `const`ified
in this file?
http://reviews.llvm.org/D
zaks.anna added a comment.
You conditionally defined when you build ON Windows machine, not when you build
FOR Windows. You should be able to query the compiler to check which targets
it's building for.
http://reviews.llvm.org/D18073
___
cfe-commi
ariccio marked an inline comment as done.
ariccio added a comment.
Wrongly `#if defined` out test is now fixed.
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe
ariccio updated this revision to Diff 55366.
ariccio added a comment.
Conditionally check Windows methods on Windows.
Is this what you're thinking of? It's kinda ugly - I was hoping to avoid the
`#if`s - but it does only check the Windows functions on Windows builds only.
http://reviews.llvm.o
zaks.anna added a comment.
"Since we are adding support for so many new APIs that are only available on
Windows, could you please condition checking them only when we build for
Windows. You probably can look and Language Options to figure that out."
By this, I was suggesting that we should be c
aaron.ballman added a comment.
This looks reasonable to me, but you should wait for @zaks.anna to sign off.
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
ariccio updated this revision to Diff 53995.
ariccio added a comment.
So, duh, it turns out I //can// use `_WIN32` to conditionally test.
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/alternative-malloc-ap
aaron.ballman added a comment.
In http://reviews.llvm.org/D18073#399270, @ariccio wrote:
> In http://reviews.llvm.org/D18073#398882, @zaks.anna wrote:
>
> > "Since we are adding support for so many new APIs that are only available
> > on Windows, could you please condition checking them only whe
ariccio added a comment.
In http://reviews.llvm.org/D18073#398882, @zaks.anna wrote:
> "Since we are adding support for so many new APIs that are only available on
> Windows, could you please condition checking them only when we build for
> Windows. You probably can look and Language Options to
ariccio updated this revision to Diff 53495.
ariccio added a comment.
Changed the new file name.
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/alternative-malloc-api.c
llvm/tools/clang/test/Analysis/mall
zaks.anna added a comment.
"Since we are adding support for so many new APIs that are only available on
Windows, could you please condition checking them only when we build for
Windows. You probably can look and Language Options to figure that out."
malloc-uses.c -> "alternative-malloc-api.c" ?
ariccio updated this revision to Diff 53193.
ariccio added a comment.
I'm not actually sure why I didn't want to split these off into a separate
file, but now I finally have. Is this what you were looking for?
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers
ariccio added a comment.
In http://reviews.llvm.org/D18073#393613, @zaks.anna wrote:
> You will have to add one test function to smoke test that the newly added API
> is modeled correctly.
Isn't that what I've already done?
> We also have a lot of existing tests that verify that each of the o
ariccio updated this revision to Diff 52876.
ariccio added a comment.
Fewer added test functions.
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/malloc.c
Index: llvm/tools/clang/test/Analysis/malloc.c
zaks.anna added a comment.
You will have to add one test function to smoke test that the newly added API
is modeled correctly. We also have a lot of existing tests that verify that
each of the original APIs (malloc. free, new) function correctly in all
possible settings. What is the contradicti
ariccio added a comment.
In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote:
> > So for _wcsdup_dbg, I should leave only testWinWcsdupDbg?
>
>
> Yes.
Ok, that I can do. Will upload patch later this afternoon/tonight.
In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote:
> Also,
zaks.anna added a comment.
> So for _wcsdup_dbg, I should leave only testWinWcsdupDbg?
Yes.
Also, since there will be many of these "alternate" functions, could you create
a separate test file for them?
http://reviews.llvm.org/D18073
___
cfe-com
ariccio added a comment.
Maybe I'm being thick headed and I can't see it - sorry if I am - but I'm still
a bit confused. Can you tell me what I should do in the `_wcsdup_dbg` example?
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-
ariccio added a comment.
So for `_wcsdup_dbg`, I should leave only `testWinWcsdupDbg`?
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
zaks.anna added a comment.
Can you add one test per new function, which tests that the function if modeled
correctly. Please, do not duplicate all tests that contain a function with it's
windows variants.
Hope this is more clear.
http://reviews.llvm.org/D18073
_
ariccio added a comment.
Here's my confusion: don't we //kinda// have to duplicate tests? After all
these functions are //essentially// duplicate functions.
http://reviews.llvm.org/D18073
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http
ariccio added a comment.
I'm sorry, I'm confused.
As an example, for `_wcsdup_dbg`, I've added `testWinWcsdupDbg`,
`testWinWcsdupDbgContentIsDefined`, and `testWinWideDbgLeakWithinReturn`. Which
ones should I drop for that API?
http://reviews.llvm.org/D18073
___
zaks.anna added a comment.
> > Also, we should not duplicate all of our tests. Instead, we should add a
> > single test per added API checking that it is modeling the operation we
> > think it
>
> > should model.
>
>
> So should I dump the _dbg versions from the tests?
No but do not
ariccio added a comment.
In http://reviews.llvm.org/D18073#372697, @zaks.anna wrote:
> Since we are adding support for so many new APIs that are only available on
> Windows, could you please condition checking them only when we build for
> Windows. You probably can look and Language Options to
zaks.anna added a comment.
Since we are adding support for so many new APIs that are only available on
Windows, could you please condition checking them only when we build for
Windows. You probably can look and Language Options to figure that out.
Also, we should not duplicate all of our tests.
ariccio created this revision.
ariccio added reviewers: zaks.anna, dcoughlin, aaron.ballman.
ariccio added a subscriber: cfe-commits.
As discussed...
http://reviews.llvm.org/D18073
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
llvm/tools/clang/test/Analysis/malloc.c
34 matches
Mail list logo
