[PATCH] Detect mismatching 'new' and 'delete' uses
Richard Smith
richard at metafoo.co.uk
Mon Sep 29 16:39:02 PDT 2014
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
More information about the cfe-commits
mailing list