================
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;
+
----------------
ismailp wrote:
> rsmith 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 const!
ructor'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.
> Could you please explain the rationale for your point (b)?
The idea is: if you haven't seen all the constructors yet, but you have seen a
constructor that used the right kind of `new`, then there's no need to queue
the expression; the `delete` is OK.
> If no constructor initializes the member, we check in-class initializer (if
> exists).
This should probably be: if any constructor does not explicitly initialize the
member, check the in-class initializer. (But it's probably redundant, since the
constructor's initializer list will have been extended to contain a reference
to the in-class initializer in this case.)
> AddAPInt(APInt(1, get<2>(tuple)), DeleteExprsToAnalyze);
This is wasteful; `DeleteExprsToAnalyze` is a `SmallVector` of integers. Just
do `DeleteExprsToAnalyze.push_back(get<2>(tuple));`. Other than that, this
looks like the right approach.
http://reviews.llvm.org/D4661
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
