Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread George Karpenkov via cfe-commits 
Pushed the fix. Again, sorry for the trouble.

> On Nov 30, 2018, at 11:34 AM, George Karpenkov via cfe-commits 
>  wrote:
> 
> Good idea about using the mailing list, thanks, I’ll do that!
> 
>> On Nov 30, 2018, at 11:30 AM, Aaron Ballman  wrote:
>> 
>> On Fri, Nov 30, 2018 at 2:26 PM George Karpenkov  
>> wrote:
>>> 
>>> Thanks I’ll take a look.
>>> 
>>> BTW when reverting could you use “git revert” or mention manually the 
>>> phabricator revision being reverted,
>>> and apply reverts atomically?
>>> I (and many others) work exclusively using a git monorepo, so I don’t even 
>>> have a straightforward way to lookup what "r347951” is.
>> 
>> Given that I work exclusively in svn, I won't be using git revert. :-)
>> I can add the phab revision to the commit log when possible, but I
>> expect to continue to use svn revisions until the git transition takes
>> place. FWIW, you can use the mailing lists to look up what r347951
>> (such as 
>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20181126/252804.html).
>> Sorry for the troubles, though!
>> 
>> ~Aaron
>> 
>>> 
>>> Thanks!
>>> 
 On Nov 30, 2018, at 11:20 AM, Aaron Ballman  wrote:
 
 On Fri, Nov 30, 2018 at 2:19 PM George Karpenkov  
 wrote:
> 
> Thanks and sorry about the trouble. I’ll recommit with size_t.
 
 No worries, it happens! FYI, I also had to commit r348023 as part of
 the reverts.
 
 ~Aaron
> 
> On Nov 30, 2018, at 10:56 AM, Aaron Ballman  
> wrote:
> 
> On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
> cfe-commits  wrote:
> 
> 
> NoQ added inline comments.
> 
> 
> 
> Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
> +
> +  static void * operator new(unsigned long size);
> +
> 
> NoQ wrote:
> 
> I think we should use `size_t` as much as possible, because this may 
> otherwise have weird consequences on platforms on which `size_t` is not 
> defined as `unsigned long`. Not sure if this checker is ran on such 
> platforms. But the test doesn't have the triple specified, so it runs 
> under the host triple, which may be arbitrary and cause problems on 
> buildbots.
> 
> I.e.,
> 
> ```
> typedef __typeof(sizeof(int)) size_t;
> // use size_t
> ```
> 
> http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp
> 
> 
> I reverted r347949 through r347951 in r348020 to get the bots back to 
> green.
> 
> ~Aaron
> 
> 
> 
> Repository:
> rC Clang
> 
> CHANGES SINCE LAST ACTION
> https://reviews.llvm.org/D55076/new/
> 
> https://reviews.llvm.org/D55076
> 
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> 
> 
>>> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread Artem Dergachev via cfe-commits 
Hmm, they don't have git-svn-id lines in the monorepo, which are handy 
for exactly that.


In the multirepos i usually just /search the git log for the svn commit 
(without "r") and it quickly matches the git-svn-id line, eg.:


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@347951 // <== 
this last part


On 11/30/18 11:34 AM, George Karpenkov wrote:

Good idea about using the mailing list, thanks, I’ll do that!


On Nov 30, 2018, at 11:30 AM, Aaron Ballman  wrote:

On Fri, Nov 30, 2018 at 2:26 PM George Karpenkov  wrote:

Thanks I’ll take a look.

BTW when reverting could you use “git revert” or mention manually the 
phabricator revision being reverted,
and apply reverts atomically?
I (and many others) work exclusively using a git monorepo, so I don’t even have a 
straightforward way to lookup what "r347951” is.

Given that I work exclusively in svn, I won't be using git revert. :-)
I can add the phab revision to the commit log when possible, but I
expect to continue to use svn revisions until the git transition takes
place. FWIW, you can use the mailing lists to look up what r347951
(such as 
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20181126/252804.html).
Sorry for the troubles, though!

~Aaron


Thanks!


On Nov 30, 2018, at 11:20 AM, Aaron Ballman  wrote:

On Fri, Nov 30, 2018 at 2:19 PM George Karpenkov  wrote:

Thanks and sorry about the trouble. I’ll recommit with size_t.

No worries, it happens! FYI, I also had to commit r348023 as part of
the reverts.

~Aaron

On Nov 30, 2018, at 10:56 AM, Aaron Ballman  wrote:

On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
cfe-commits  wrote:


NoQ added inline comments.



Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
+
+  static void * operator new(unsigned long size);
+

NoQ wrote:

I think we should use `size_t` as much as possible, because this may otherwise 
have weird consequences on platforms on which `size_t` is not defined as 
`unsigned long`. Not sure if this checker is ran on such platforms. But the 
test doesn't have the triple specified, so it runs under the host triple, which 
may be arbitrary and cause problems on buildbots.

I.e.,

```
typedef __typeof(sizeof(int)) size_t;
// use size_t
```

http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp


I reverted r347949 through r347951 in r348020 to get the bots back to green.

~Aaron



Repository:
rC Clang

CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55076/new/

https://reviews.llvm.org/D55076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits




___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov marked an inline comment as done.
george.karpenkov added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp:483-497
   case CE_Function:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
   case CE_CXXMember:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
   case CE_CXXMemberOperator:

NoQ wrote:
> It's actually just `Call.getDecl()` and you can turn this into a fall-through.
Call.getDecl() returns a Decl (gotta love Obj-C methods!).

I guess we can group all those cases, and cast the returned decl to 
FunctionDecl instead of casting the call.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55076/new/

https://reviews.llvm.org/D55076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread George Karpenkov via cfe-commits 
Good idea about using the mailing list, thanks, I’ll do that!

> On Nov 30, 2018, at 11:30 AM, Aaron Ballman  wrote:
> 
> On Fri, Nov 30, 2018 at 2:26 PM George Karpenkov  wrote:
>> 
>> Thanks I’ll take a look.
>> 
>> BTW when reverting could you use “git revert” or mention manually the 
>> phabricator revision being reverted,
>> and apply reverts atomically?
>> I (and many others) work exclusively using a git monorepo, so I don’t even 
>> have a straightforward way to lookup what "r347951” is.
> 
> Given that I work exclusively in svn, I won't be using git revert. :-)
> I can add the phab revision to the commit log when possible, but I
> expect to continue to use svn revisions until the git transition takes
> place. FWIW, you can use the mailing lists to look up what r347951
> (such as 
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20181126/252804.html).
> Sorry for the troubles, though!
> 
> ~Aaron
> 
>> 
>> Thanks!
>> 
>>> On Nov 30, 2018, at 11:20 AM, Aaron Ballman  wrote:
>>> 
>>> On Fri, Nov 30, 2018 at 2:19 PM George Karpenkov  
>>> wrote:
 
 Thanks and sorry about the trouble. I’ll recommit with size_t.
>>> 
>>> No worries, it happens! FYI, I also had to commit r348023 as part of
>>> the reverts.
>>> 
>>> ~Aaron
 
 On Nov 30, 2018, at 10:56 AM, Aaron Ballman  wrote:
 
 On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
 cfe-commits  wrote:
 
 
 NoQ added inline comments.
 
 
 
 Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
 +
 +  static void * operator new(unsigned long size);
 +
 
 NoQ wrote:
 
 I think we should use `size_t` as much as possible, because this may 
 otherwise have weird consequences on platforms on which `size_t` is not 
 defined as `unsigned long`. Not sure if this checker is ran on such 
 platforms. But the test doesn't have the triple specified, so it runs 
 under the host triple, which may be arbitrary and cause problems on 
 buildbots.
 
 I.e.,
 
 ```
 typedef __typeof(sizeof(int)) size_t;
 // use size_t
 ```
 
 http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp
 
 
 I reverted r347949 through r347951 in r348020 to get the bots back to 
 green.
 
 ~Aaron
 
 
 
 Repository:
 rC Clang
 
 CHANGES SINCE LAST ACTION
 https://reviews.llvm.org/D55076/new/
 
 https://reviews.llvm.org/D55076
 
 
 
 ___
 cfe-commits mailing list
 cfe-commits@lists.llvm.org
 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
 
 
>> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread Aaron Ballman via cfe-commits 
On Fri, Nov 30, 2018 at 2:26 PM George Karpenkov  wrote:
>
> Thanks I’ll take a look.
>
> BTW when reverting could you use “git revert” or mention manually the 
> phabricator revision being reverted,
> and apply reverts atomically?
> I (and many others) work exclusively using a git monorepo, so I don’t even 
> have a straightforward way to lookup what "r347951” is.

Given that I work exclusively in svn, I won't be using git revert. :-)
I can add the phab revision to the commit log when possible, but I
expect to continue to use svn revisions until the git transition takes
place. FWIW, you can use the mailing lists to look up what r347951
(such as 
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20181126/252804.html).
Sorry for the troubles, though!

~Aaron

>
> Thanks!
>
> > On Nov 30, 2018, at 11:20 AM, Aaron Ballman  wrote:
> >
> > On Fri, Nov 30, 2018 at 2:19 PM George Karpenkov  
> > wrote:
> >>
> >> Thanks and sorry about the trouble. I’ll recommit with size_t.
> >
> > No worries, it happens! FYI, I also had to commit r348023 as part of
> > the reverts.
> >
> > ~Aaron
> >>
> >> On Nov 30, 2018, at 10:56 AM, Aaron Ballman  wrote:
> >>
> >> On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
> >> cfe-commits  wrote:
> >>
> >>
> >> NoQ added inline comments.
> >>
> >>
> >> 
> >> Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
> >> +
> >> +  static void * operator new(unsigned long size);
> >> +
> >> 
> >> NoQ wrote:
> >>
> >> I think we should use `size_t` as much as possible, because this may 
> >> otherwise have weird consequences on platforms on which `size_t` is not 
> >> defined as `unsigned long`. Not sure if this checker is ran on such 
> >> platforms. But the test doesn't have the triple specified, so it runs 
> >> under the host triple, which may be arbitrary and cause problems on 
> >> buildbots.
> >>
> >> I.e.,
> >>
> >> ```
> >> typedef __typeof(sizeof(int)) size_t;
> >> // use size_t
> >> ```
> >>
> >> http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp
> >>
> >>
> >> I reverted r347949 through r347951 in r348020 to get the bots back to 
> >> green.
> >>
> >> ~Aaron
> >>
> >>
> >>
> >> Repository:
> >> rC Clang
> >>
> >> CHANGES SINCE LAST ACTION
> >> https://reviews.llvm.org/D55076/new/
> >>
> >> https://reviews.llvm.org/D55076
> >>
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >>
> >>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread George Karpenkov via cfe-commits 
Thanks I’ll take a look.

BTW when reverting could you use “git revert” or mention manually the 
phabricator revision being reverted,
and apply reverts atomically?
I (and many others) work exclusively using a git monorepo, so I don’t even have 
a straightforward way to lookup what "r347951” is.

Thanks!

> On Nov 30, 2018, at 11:20 AM, Aaron Ballman  wrote:
> 
> On Fri, Nov 30, 2018 at 2:19 PM George Karpenkov  wrote:
>> 
>> Thanks and sorry about the trouble. I’ll recommit with size_t.
> 
> No worries, it happens! FYI, I also had to commit r348023 as part of
> the reverts.
> 
> ~Aaron
>> 
>> On Nov 30, 2018, at 10:56 AM, Aaron Ballman  wrote:
>> 
>> On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
>> cfe-commits  wrote:
>> 
>> 
>> NoQ added inline comments.
>> 
>> 
>> 
>> Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
>> +
>> +  static void * operator new(unsigned long size);
>> +
>> 
>> NoQ wrote:
>> 
>> I think we should use `size_t` as much as possible, because this may 
>> otherwise have weird consequences on platforms on which `size_t` is not 
>> defined as `unsigned long`. Not sure if this checker is ran on such 
>> platforms. But the test doesn't have the triple specified, so it runs under 
>> the host triple, which may be arbitrary and cause problems on buildbots.
>> 
>> I.e.,
>> 
>> ```
>> typedef __typeof(sizeof(int)) size_t;
>> // use size_t
>> ```
>> 
>> http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp
>> 
>> 
>> I reverted r347949 through r347951 in r348020 to get the bots back to green.
>> 
>> ~Aaron
>> 
>> 
>> 
>> Repository:
>> rC Clang
>> 
>> CHANGES SINCE LAST ACTION
>> https://reviews.llvm.org/D55076/new/
>> 
>> https://reviews.llvm.org/D55076
>> 
>> 
>> 
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> 
>> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread Aaron Ballman via cfe-commits 
On Fri, Nov 30, 2018 at 2:19 PM George Karpenkov  wrote:
>
> Thanks and sorry about the trouble. I’ll recommit with size_t.

No worries, it happens! FYI, I also had to commit r348023 as part of
the reverts.

~Aaron
>
> On Nov 30, 2018, at 10:56 AM, Aaron Ballman  wrote:
>
> On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
> cfe-commits  wrote:
>
>
> NoQ added inline comments.
>
>
> 
> Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
> +
> +  static void * operator new(unsigned long size);
> +
> 
> NoQ wrote:
>
> I think we should use `size_t` as much as possible, because this may 
> otherwise have weird consequences on platforms on which `size_t` is not 
> defined as `unsigned long`. Not sure if this checker is ran on such 
> platforms. But the test doesn't have the triple specified, so it runs under 
> the host triple, which may be arbitrary and cause problems on buildbots.
>
> I.e.,
>
> ```
> typedef __typeof(sizeof(int)) size_t;
> // use size_t
> ```
>
> http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp
>
>
> I reverted r347949 through r347951 in r348020 to get the bots back to green.
>
> ~Aaron
>
>
>
> Repository:
>  rC Clang
>
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D55076/new/
>
> https://reviews.llvm.org/D55076
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread George Karpenkov via cfe-commits 
Thanks and sorry about the trouble. I’ll recommit with size_t.

> On Nov 30, 2018, at 10:56 AM, Aaron Ballman  wrote:
> 
> On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
> cfe-commits mailto:cfe-commits@lists.llvm.org>> 
> wrote:
>> 
>> NoQ added inline comments.
>> 
>> 
>> 
>> Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
>> +
>> +  static void * operator new(unsigned long size);
>> +
>> 
>> NoQ wrote:
>>> I think we should use `size_t` as much as possible, because this may 
>>> otherwise have weird consequences on platforms on which `size_t` is not 
>>> defined as `unsigned long`. Not sure if this checker is ran on such 
>>> platforms. But the test doesn't have the triple specified, so it runs under 
>>> the host triple, which may be arbitrary and cause problems on buildbots.
>>> 
>>> I.e.,
>>> 
>>> ```
>>> typedef __typeof(sizeof(int)) size_t;
>>> // use size_t
>>> ```
>> http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp
> 
> I reverted r347949 through r347951 in r348020 to get the bots back to green.
> 
> ~Aaron
> 
>> 
>> 
>> Repository:
>>  rC Clang
>> 
>> CHANGES SINCE LAST ACTION
>>  https://reviews.llvm.org/D55076/new/ 
>> 
>> https://reviews.llvm.org/D55076 
>> 
>> 
>> 
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org 
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
>> 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread Aaron Ballman via cfe-commits 
On Fri, Nov 30, 2018 at 9:37 AM Artem Dergachev via Phabricator via
cfe-commits  wrote:
>
> NoQ added inline comments.
>
>
> 
> Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
> +
> +  static void * operator new(unsigned long size);
> +
> 
> NoQ wrote:
> > I think we should use `size_t` as much as possible, because this may 
> > otherwise have weird consequences on platforms on which `size_t` is not 
> > defined as `unsigned long`. Not sure if this checker is ran on such 
> > platforms. But the test doesn't have the triple specified, so it runs under 
> > the host triple, which may be arbitrary and cause problems on buildbots.
> >
> > I.e.,
> >
> > ```
> > typedef __typeof(sizeof(int)) size_t;
> > // use size_t
> > ```
> http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp

I reverted r347949 through r347951 in r348020 to get the bots back to green.

~Aaron

>
>
> Repository:
>   rC Clang
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55076/new/
>
> https://reviews.llvm.org/D55076
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-30 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
+
+  static void * operator new(unsigned long size);
+

NoQ wrote:
> I think we should use `size_t` as much as possible, because this may 
> otherwise have weird consequences on platforms on which `size_t` is not 
> defined as `unsigned long`. Not sure if this checker is ran on such 
> platforms. But the test doesn't have the triple specified, so it runs under 
> the host triple, which may be arbitrary and cause problems on buildbots.
> 
> I.e.,
> 
> ```
> typedef __typeof(sizeof(int)) size_t;
> // use size_t
> ```
http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55076/new/

https://reviews.llvm.org/D55076



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

2018-11-29 Thread George Karpenkov via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC347949: [analyzer] RetainCountChecker: recognize that 
OSObject can be created directly… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55076?vs=175949=176021#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55076/new/

https://reviews.llvm.org/D55076

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
  test/Analysis/osobject-retain-release.cpp


Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -124,10 +124,8 @@
   }
 
   const IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-return getDefaultSummary();
 
-  StringRef FName = II->getName();
+  StringRef FName = II ? II->getName() : "";
 
   // Strip away preceding '_'.  Doing this here will effect all the checks
   // down below.
@@ -304,6 +302,9 @@
 
   if (FName == "retain")
 return getOSSummaryRetainRule(FD);
+
+  if (MD->getOverloadedOperator() == OO_New)
+return getOSSummaryCreateRule(MD);
 }
   }
 
@@ -491,9 +492,11 @@
   case CE_CXXConstructor:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
+  case CE_CXXAllocator:
+Summ = getFunctionSummary(cast(Call).getDecl());
+break;
   case CE_Block:
   case CE_CXXDestructor:
-  case CE_CXXAllocator:
 // FIXME: These calls are currently unsupported.
 return getPersistentStopSummary();
   case CE_ObjCMessage: {
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -137,6 +137,8 @@
 } else {
   os << "function call";
 }
+  } else if (const auto *NE = dyn_cast(S)){
+os << "Operator new";
   } else {
 assert(isa(S));
 CallEventManager  = CurrSt->getStateManager().getCallEventManager();
Index: test/Analysis/osobject-retain-release.cpp
===
--- test/Analysis/osobject-retain-release.cpp
+++ test/Analysis/osobject-retain-release.cpp
@@ -23,6 +23,9 @@
   static OSObject *getObject();
   static OSObject *GetObject();
 
+
+  static void * operator new(unsigned long size);
+
   static const OSMetaClass * const metaClass;
 };
 
@@ -62,6 +65,18 @@
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+unsigned int check_leak_explicit_new() {
+  OSArray *arr = new OSArray; // expected-note{{Operator new returns an 
OSObject of type struct OSArray * with a +1 retain count}}
+  return arr->getCount(); // expected-note{{Object leaked: allocated object of 
type struct OSArray * is not referenced later in this execution path and has a 
retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object 
of type struct OSArray *}}
+}
+
+unsigned int check_leak_factory() {
+  OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 
'OSArray::withCapacity' returns an OSObject of type struct OSArray * with a +1 
retain count}}
+  return arr->getCount(); // expected-note{{Object leaked: object allocated 
and stored into 'arr' is not referenced later in this execution path and has a 
retain count of +1}}
+  // expected-warning@-1{{Potential leak of an object 
stored into 'arr'}}
+}
+
 void check_get_object() {
   OSObject::getObject();
 }


Index: lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
===
--- lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
+++ lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
@@ -124,10 +124,8 @@
   }
 
   const IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-return getDefaultSummary();
 
-  StringRef FName = II->getName();
+  StringRef FName = II ? II->getName() : "";
 
   // Strip away preceding '_'.  Doing this here will effect all the checks
   // down below.
@@ -304,6 +302,9 @@
 
   if (FName == "retain")
 return getOSSummaryRetainRule(FD);
+
+  if (MD->getOverloadedOperator() == OO_New)
+return getOSSummaryCreateRule(MD);
 }
   }
 
@@ -491,9 +492,11 @@
   case CE_CXXConstructor:
 Summ = getFunctionSummary(cast(Call).getDecl());
 break;
+  case CE_CXXAllocator:
+Summ = getFunctionSummary(cast(Call).getDecl());
+break;
   case CE_Block:
   case CE_CXXDestructor:
-  case CE_CXXAllocator:
 // FIXME: These calls are currently unsupported.
 return getPersistentStopSummary();
   case