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

[Richard Smith]

This is looking really good. Some fairly minor comments...

================
Comment at: include/clang/Sema/Sema.h:8539-8541
@@ +8538,5 @@
+  ///
+  /// If pointee is initialized with array form of 'new', it should be deleted
+  /// with array form delete. If at least one \a matching new-expression is
+  /// found, the function will return false.
+  /// \param DE delete-expression to analyze
----------------
This should probably say "with the array form of 'delete'."

Also, the function's return type is `void`; it won't return `false`.

================
Comment at: lib/Sema/Sema.cpp:1489-1492
@@ +1488,5 @@
+
+void Sema::getMismatchingDeleteExpressions(
+    llvm::DenseMap<FieldDecl *, DeleteLocs> &Exprs) {
+  Exprs = DeleteExprs;
+}
----------------
Can you avoid making a copy of the `DenseMap` here?

================
Comment at: lib/Sema/SemaExprCXX.cpp:2246-2247
@@ +2245,4 @@
+
+  /// \brief Checks whether pointee of \p DE is initialized with mismatching
+  /// \brief form of \a new.
+  /// \param DE delete-expression to check.
----------------
`\brief` only goes on the first line.

================
Comment at: lib/Sema/SemaExprCXX.cpp:2271
@@ +2270,3 @@
+  /// point where delete-expression is encountered, then a warning will be
+  /// issued immediately. If eturn value is \c AnalyzeLater at the point where
+  /// delete-expression is seen, then member will be analyzed at the end of
----------------
Typo "eturn"

================
Comment at: lib/Sema/SemaExprCXX.cpp:2348
@@ +2347,3 @@
+  assert(E != nullptr && "Expected a valid initializer expression");
+  E = E->IgnoreImpCasts();
+  const CXXNewExpr *NE = dyn_cast<const CXXNewExpr>(E);
----------------
`IgnoreParenImpCasts` would seem better here; you don't find parenthesized new-expressions here.

================
Comment at: lib/Sema/SemaExprCXX.cpp:2409-2411
@@ +2408,5 @@
+  assert(Field != nullptr &&
+         "Analysis must be run from Sema::ActOnCXXDelete, with pointer to "
+         "CXXDeleteExpr or from Sema::ActOnEndOfTranslationUnit, where 'Field' "
+         "is non-null");
+  const CXXRecordDecl *RD = cast<const CXXRecordDecl>(Field->getParent());
----------------
This seems like overspecification; just say that we require a field or a delete expression.

================
Comment at: lib/Sema/SemaExprCXX.cpp:2467-2468
@@ +2466,4 @@
+  case MismatchingNewDeleteDetector::MemberInitMismatches: {
+    DiagnoseMismatchedNewDelete(Detector.Field, DE->getLocStart(),
+                                DE->isArrayForm());
+    for (const auto *NE : Detector.NewExprs)
----------------
Maybe pass `Detector.NewExprs` into `DiagnoseMismatchedNewDelete` and have it produce the notes as well as the error? That'd remove a bit of duplication between this function and the next one.

================
Comment at: lib/Serialization/ASTWriter.cpp:4347
@@ +4346,3 @@
+  SemaRef.getMismatchingDeleteExpressions(DeleteExprs);
+  for (const auto &DeleteExprsInfo : DeleteExprs) {
+    AddDeclRef(DeleteExprsInfo.first, DeleteExprsToAnalyze);
----------------
This writes out the map in a nondeterministic order. Maybe use an `llvm::MapVector`?

http://reviews.llvm.org/D4661