<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 06.03.2013 21:46, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:A8EAA2BB-635A-45F9-B85E-77A725D9C2AC@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
CC-ing the patch author!
<div><br>
</div>
<div>Also, </div>
<div><br>
</div>
<div>Could you split out the ObjC NoCopy + FreeWhenDone change
into a separate patch. It does not seem to be directly related
to the other changes. Also, I am not 100% sure what changes we
are making there. One part is refactoring, however, you've also
removed the check for the message calls from the
doesNotFreeMemory(). Do we expect any behavior changes from
this?</div>
<div>(Sorry if I've missed something; the patch is getting big.)</div>
</blockquote>
Splitted ObjC NoCopy + FreeWhenDone change, kept changes in
doesNotFreeMemory().<br>
The logic of doesNotFreeMemory() was broken - it treated all
'NoCopy' and 'FreeWhenDone==1' methods as freeing memory and unknown
to us. This lead to removal of RefState from the State and
impossibility for further alloc/dealloc matching analysis.<br>
<br>
Added the test that covers this change among other things:<br>
void testNew11(NSUInteger dataLength) {<br>
int *data = new int;<br>
NSData *nsdata = [NSData dataWithBytesNoCopy:data
length:sizeof(int) freeWhenDone:1]; // expected-warning{{Memory
allocated by 'new' should be deallocated by 'delete', not
+dataWithBytesNoCopy:length:freeWhenDone:}}<br>
}<br>
<blockquote
cite="mid:A8EAA2BB-635A-45F9-B85E-77A725D9C2AC@apple.com"
type="cite">
<div><br>
</div>
<div>Anna.</div>
<div><br>
<div>
<div>On Mar 5, 2013, at 10:53 PM, Anna Zaks <<a
moz-do-not-send="true" href="mailto:ganna@apple.com">ganna@apple.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto;
text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: auto; word-spacing: 0px;
-webkit-text-size-adjust: auto; -webkit-text-stroke-width:
0px;">Looks good. Thanks for working on this!<br>
<br>
- case OwnershipAttr::Returns:<br>
- State = MallocMemReturnsAttr(C, CE, *i);<br>
- break;<br>
..<br>
+ case OwnershipAttr::Returns:<br>
+ State = MallocMemReturnsAttr(C, CE, *i);<br>
+ break;<br>
..<br>
Is the indentation the only diff here? If yes, please keep
the previous version; most of the case statements are not
indented in the llvm/clang codebases. We usually try to
keep the patches focused on the problem solved. If cleanup
of existing code is necessary, it should go into a
separate commit. <br>
<br>
- HelpText<"Check for memory leaks, double free, and
use-after-free problems. Assumes that all user-defined
functions which might free a pointer are annotated.">,<br>
+ HelpText<"Check for memory leaks, double free, and
use-after-free problems. Traces memory managed by
malloc()/free(). Assumes that all user-defined functions
which might free a pointer are annotated.">,<br>
This is getting too long.<br>
</div>
</blockquote>
</div>
</div>
</blockquote>
Haven't got anything better then to remove 'Traces memory managed by
malloc()/free().' sentence.<br>
Do you have an idea how to shorten this?<br>
<br>
<blockquote
cite="mid:A8EAA2BB-635A-45F9-B85E-77A725D9C2AC@apple.com"
type="cite">
<div>
<div>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto;
text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: auto; word-spacing: 0px;
-webkit-text-size-adjust: auto; -webkit-text-stroke-width:
0px;"><br>
Jordan had previously pointed out that you might need to
add 'new' and 'delete' to doesNotFreeMemOrKnown. I do not
see that changed. Has it been done or is it not necessary?<br>
</div>
</blockquote>
</div>
</div>
</blockquote>
The change gone to isMemFunction() that is called from
doesNotFreeMemOrKnown()<br>
<br>
<blockquote
cite="mid:A8EAA2BB-635A-45F9-B85E-77A725D9C2AC@apple.com"
type="cite">
<div>
<div>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto;
text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: auto; word-spacing: 0px;
-webkit-text-size-adjust: auto; -webkit-text-stroke-width:
0px;"><br>
Thanks,<br>
Anna.<br>
<br>
On Mar 5, 2013, at 6:25 PM, Jordan Rose <<a
moz-do-not-send="true"
href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>>
wrote:<br>
<br>
<blockquote type="cite"><br>
On Mar 5, 2013, at 5:12 , Anton Yartsev <<a
moz-do-not-send="true"
href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
wrote:<br>
<br>
<blockquote type="cite">Attached is the new patch with
unix.MismatchedDeallocator and cplusplus.NewDelete
splitted from unix.Malloc plus comments addressed and
several other improvements.<br>
</blockquote>
<br>
Awesome. At this point I think this is basically ready
to go in. Anna?<br>
<br>
(We need to update SATestBuild.py and
lib/Frontend/CompilerInvocation.cpp to enable the
"cplusplus" package again.)<br>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
Have not found any suitable place in
lib/Frontend/CompilerInvocation.cpp<br>
Did you mean to update clang/lib/Driver/Tools.cpp ?<br>
Attached patch contain the supposed change.<br>
<br>
<blockquote
cite="mid:A8EAA2BB-635A-45F9-B85E-77A725D9C2AC@apple.com"
type="cite">
<div>
<div>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto;
text-align: start; text-indent: 0px; text-transform: none;
white-space: normal; widows: auto; word-spacing: 0px;
-webkit-text-size-adjust: auto; -webkit-text-stroke-width:
0px;">
<blockquote type="cite"><br>
<br>
<blockquote type="cite">+ delete p; //
expected-warning{{Memory allocated by operator new[]
should be deallocated by operator delete[], not
operator delete}}<br>
</blockquote>
<br>
<br>
Bikeshedding again on the warning text: do we really
need the word "operator" in there? Maybe just putting
quotes around the operator name would be good enough:<br>
<br>
</blockquote>
<br>
+1<span class="Apple-converted-space"> </span><br>
It's best to keep the diagnostics as concise as possible.<span
class="Apple-converted-space"> </span><br>
<br>
<blockquote type="cite">"Memory allocated by 'new[]'
should be deallocated by 'delete[]', not 'delete'".<br>
<br>
That's this part of printAllocDeallocName (which may
have been suggested by me):<br>
<br>
<blockquote type="cite">+ if (const CXXNewExpr *NE =
dyn_cast<CXXNewExpr>(E)) {<br>
+ os << *NE->getOperatorNew();<br>
+ return true;<br>
+ }<br>
+<br>
+ if (const CXXDeleteExpr *DE =
dyn_cast<CXXDeleteExpr>(E)) {<br>
+ os << *DE->getOperatorDelete();<br>
+ return true;<br>
+ }<br>
</blockquote>
<br>
It looks like there's a function getOperatorSpelling
that will get the name without the word "operator". (It
makes sense to leave in the "operator" when it appears
in an explicit call expr, just not a normal expression
or the "expected" case.<br>
</blockquote>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true"
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>