[PATCH] Detect mismatching 'new' and 'delete' uses

[Ismail Pazarbasi]

================
Comment at: include/clang/Sema/Sema.h:395-396
@@ -392,1 +394,4 @@
 
+  /// \brief All delete-expressions within the translation unit.
+  llvm::SmallPtrSet<const CXXDeleteExpr *, 4> DeleteExprs;
+
----------------
Richard Smith wrote:
> This is not a reasonable way to track the relevant state here.
> 
> The interesting cases are `VarDecl`s and `FieldDecl`s. For `VarDecl`s, at the point where we see the `delete`, you can immediately check whether you have an new-expression as an initializer (and don't worry about the obscure cases where an initializer comes after the use). For `FieldDecl`s, perform the check where you see the `delete`, and if you see either (a) no constructor with a matching `new`, or (b) only constructors with the wrong kind of `new`, then add your `delete` to a list to check at the end of the TU.
> 
> That way, in almost all cases we can deal with the diagnostic immediately rather than at end of TU. (This avoids building up a big set of all delete expressions.)
> 
> You'll also need to figure out how you're going to handle serialization/deserialization here, in the case where you hit one of these "not sure" cases in a PCH or similar. To that end, you should probably store a list of `{SourceLocation, FieldDecl}` pairs, or -- better -- a map from `FieldDecl` to the first source location where we saw a `delete`. (You'll need one extra bit to describe whether it was `delete` or `delete[]` I suppose...).
I have implemented the first half of the patch, slightly different than your suggestion. We diagnose this immediately for `VarDecl`, as you said. For `FieldDecl`s, delete expression is queued, if at least one constructor isn't defined (yet). Otherwise (if all constructor definitions are visible), warning is issued in `ActOnCXXDelete` right before function returns. Could you please explain the rationale for your point (b)? If we can see all constructors while analyzing a `CXXDeleteExpr`, we can reason about constructors and in-class initializers immediately. If no constructor initializes the member, we check in-class initializer (if exists). If all constructors initialize member with mismatching type of `new`, then we can diagnose them all immediately, instead of postponing it to the end of translation unit. I can think of only one case (aside from PCH case) that requires postponing this analysis to the end of translation unit; that is, when we cannot see at least one constructor's definition.

I have a few questions regarding PCH/external source. I have no prior experience with ASTReader/Writer at all. Sorry, if my questions don't make any sense. Do you mean we should handle following case:
  // header; compiled as pch.
  #ifndef header
  struct X {
    int *a;
    ~X() { delete a; }  // not sure
    X();
   };
   #endif

  // source file, includes above pch
  #include "header"
  X::X() : a(new int[1]) { }

We encounter with a delete-expr in a PCH. We can't prove anything, because we can't see definition of `X::X()` yet. Even if there was an in-class initializer, that wouldn't help, exactly for the same reason. `X::X()` will be visible only at the end of translation unit. Therefore, we must write this information somewhere in PCH so that we can load it at the end of translation unit.

I have followed `UndefinedButUsed` as a serialization/deserialization example. As far as I understood, I need to write a record, and read it back. I blindly followed your suggestion here :) This record contains, as you said, `FieldDecl`, `SourceLocation` and a bit indicating array form. I put them in an `std::tuple<FieldDecl *, SourceLocation, bool>`. Can that "bit" be represented as `APInt(width=1)`? I did, roughly:
  RecordData DeleteExprsToAnalyze;
  AddDeclRef(get<0>(tuple), DeleteExprsToAnalyze);
  AddSource(get<1>(tuple), DeleteExprsToAnalyze);
  AddAPInt(APInt(1, get<2>(tuple)), DeleteExprsToAnalyze);
  // ...
  if (!DeleteExprsToAnalyze.empty())
    Stream.EmitRecord(DELETE_EXPRS_TO_ANALYZE, DeleteExprsToAnalyze);

I am trying the PCH part now. I would like to learn the right way, as I might be doing something completely wrong.

http://reviews.llvm.org/D4661