On Jan 18, 2013, at 6:43 PM, Jordan Rose <[email protected]> wrote:
>> Bring it on! (:
>
> *cracks knuckles*
>
> Sorry I haven't been following your thread so closely; been drawn away by
> other things. I agree that adding the PointerEscapeKind enum is the right way
> to go, and the patch is pretty straightforward. So, a lot of spot comments...
>
>
> + const enum PointerEscapeKind Kind) {
>
> No "const" needed for value types, and conventionally no "enum" in LLVM-style
> C++. This shows up in a couple of places.
>
>
> +/// \brief Describes the different reasons a pointer escapes
> +/// during analysis.
> +enum PointerEscapeKind {
> + /// A pointer escapes due to binding its value to a location,
> + /// preventing further reasoning on the pointer.
> + PSK_EscapeOnBind = 1,
>
> This makes it sound like this happens for any location, which isn't true. How
> about "a location the analyzer can't track"?
>
>
> + /// The pointer has been passed to a function call directly.
> + PSK_DirectEscapeOnCall,
> +
> + /// The pointer has been passed to a function indirectly.
> + /// For example, the pointer is accessable through an
> + /// argument to a function.
> + PSK_IndirectEscapeOnCall,
>
> Typo: "accessable"
>
>
> + ///The reason for pointer escape is unknown. For example, a checker
> + ///invalidates a region and intends the invalidation to cause a
> + ///pointer escape event.
> + PSK_EscapeOther
>
> Missing spaces after the slashes. Also, I feel like this is too explicit—if a
> region is invalidated and the checker writer doesn't think about whether the
> contents escape, this still happens. How about "For example, a region
> containing this pointer is invalidated."?
>
> +};
>
> (I do want to say "nice comments", though! Thank you for putting in the time
> to document everything.)
>
>
> + /// \param Kind The kind of escape the pointers have undergone.
>
> Clever use of English. Simpler: "How the symbols have escaped."
>
>
> ProgramStateRef checkPointerEscape(ProgramStateRef State,
> - const InvalidatedSymbols &Escaped,
> - const CallEvent *Call) const {
> + const InvalidatedSymbols &Escaped,
> + const CallEvent *Call,
> + const enum PointerEscapeKind Kind) const {
>
> Please don't change all the indentation; before the parameters lined up, now
> they don't. I realize that the last line didn't fit, but so far in the
> analyzer we've been just tweaking that one line. If you want to change all
> the lines, please put the first parameter on its own line and indent
> everything twice only.
>
> ...although the removal of "const" and "enum" may make this short enough to
> fit.
>
>
> - if (Call && doesNotFreeMemory(Call, State))
> + if ((Kind == PSK_DirectEscapeOnCall ||
> + Kind == PSK_IndirectEscapeOnCall) &&
> + doesNotFreeMemory(Call, State)) {
>
> This is not correct. Before, this branch was only taken if the Kind is
> PSK_DirectEscapeOnCall. Indirect escapes can still possibly free memory
> (although it's unlikely).
>
Jordan,
I think the condition is correct. In fact, this is the whole point of this
commit.
For example, previously, we would assume that a call to
foo_that_does_not_free_memory(p+1), could free memory.
Branden, can you add a test case that shows this? I know the test cases in the
second patch rely on this, but a self contained example would make the change
explicit. (Ex: you could call a function from a system header and pass it p+1.)
>
> - if (Call && guaranteedNotToCloseFile(*Call))
> + if ((Kind == PSK_DirectEscapeOnCall ||
> + Kind == PSK_IndirectEscapeOnCall) &&
> + guaranteedNotToCloseFile(*Call)) {
>
> Ditto.
>
>
> + assert((Kind == PSK_DirectEscapeOnCall ||
> + Kind == PSK_IndirectEscapeOnCall) ?
> + Call != NULL : true);
>
> What Anna said about implication.
>
>
> - EscapedSymbols,
> - /*CallEvent*/ 0);
> + EscapedSymbols,
> + /*CallEvent*/ 0,
> + PSK_EscapeOnBind);
>
> - *Invalidated, 0);
> + *Invalidated,
> + 0,
> + PSK_EscapeOther);
>
> These do fit in 80 columns. Please do not reformat arbitrarily.
>
> I'll review the malloc patch in a separate e-mail.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
