[cfe-dev] Representing the allocation in CXXNewExpr

[Sebastian Redl]

Hi Jordan,

>    // Was the usage ::new, i.e. is the global new to be used?
>    bool GlobalNew : 1;
> -  // Do we allocate an array? If so, the first SubExpr is the size expression.
> -  bool Array : 1;
>    // If this is an array allocation, does the usual deallocation
>    // function for the allocated type want to know the allocated size?
>    bool UsualArrayDeleteWantsSize : 1;
> -  // The number of placement new arguments.
> -  unsigned NumPlacementArgs : 13;


With the number of bits needed by flags down this much, is it now feasible to sink this into the shared bitfield of Stmt?

> +  // FIXME: Should these be removed?
>    typedef ExprIterator arg_iterator;
>    typedef ConstExprIterator const_arg_iterator;


Really depends on who uses them and what usage patterns these places have. Does it make sense for them to iterate over the placement-new arguments?

> --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h
> +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ObjCMessage.h
> @@ -196,10 +196,18 @@ public:
>  
>    /// Check if the callee is declared in the system header.
>    bool isInSystemHeader() const {
> -    if (const Decl *FD = getDecl()) {
> -      const SourceManager &SM =
> -        State->getStateManager().getContext().getSourceManager();
> -      return SM.isInSystemHeader(FD->getLocation());
> +    if (const Decl *D = getDecl()) {
> +      SourceLocation Loc = D->getLocation();
> +      if (Loc.isValid()) {
> +        const SourceManager &SM =
> +          State->getStateManager().getContext().getSourceManager();
> +        return SM.isInSystemHeader(Loc);        
> +      } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
> +        // Operator new and operator delete may be implicitly declared.
> +        // We can't just check /any/ implicit declarations, though, because
> +        // C89 allows any function call to create an implicit declaration.
> +        return FD->isOverloadedOperator() && FD->isImplicit();
> +      }
>      }
>      return false;
>    }


This looks unrelated.

> +    // FIXME: Can the array size ever throw?
> +    if (const Expr *ArraySize = cast<CXXNewExpr>(this)->getArraySize())
> +      CT = MergeCanThrow(CT, ArraySize->CanThrow(C));


Of course it can throw.
new int[callSomeFunction()]

> +  } else if (Kind == OO_New || Kind == OO_Array_New) {
> +    // FIXME: Should the array brackets/array size be included for new[]?
> +    if (getRParenLoc().isValid())
> +      // There are explicit placement args.
> +      return SourceRange(getArg(0)->getSourceRange().getBegin(), getRParenLoc());
> +    else
> +      // There are no explicit placement args.
> +      return getArg(0)->getSourceRange();


I think it would be better, but perhaps inconsistent with non-array new, since I don't think we include the type range there, do we?

> --- a/lib/AST/StmtPrinter.cpp
> +++ b/lib/AST/StmtPrinter.cpp
> @@ -1154,6 +1154,8 @@ void StmtPrinter::VisitCXXOperatorCallExpr(CXXOperatorCallExpr *Node) {
>      OS << '[';
>      PrintExpr(Node->getArg(1));
>      OS << ']';
> +  } else if (Kind == OO_New || Kind == OO_Array_New) {
> +    llvm_unreachable("should be handled within CXXNewExpr");
>    } else if (Node->getNumArgs() == 1) {
>      OS << OpStrings[Kind] << ' ';
>      PrintExpr(Node->getArg(0));
> @@ -1407,13 +1409,17 @@ void StmtPrinter::VisitCXXNewExpr(CXXNewExpr *E) {
>    if (E->isGlobalNew())
>      OS << "::";
>    OS << "new ";
> -  unsigned NumPlace = E->getNumPlacementArgs();
> -  if (NumPlace > 0) {
> +  if (E->getNumPlacementArgs() > 0) {
>      OS << "(";
> -    PrintExpr(E->getPlacementArg(0));
> -    for (unsigned i = 1; i < NumPlace; ++i) {
> -      OS << ", ";
> -      PrintExpr(E->getPlacementArg(i));
> +    bool NeedComma = false;
> +    for (CXXNewExpr::arg_iterator P = E->placement_arg_begin(),
> +                               PEnd = E->placement_arg_end();
> +         P != PEnd; ++P) {
> +      if (NeedComma)
> +        OS << ", ";
> +      else
> +        NeedComma = true;
> +      PrintExpr(*P);
>      }
>      OS << ") ";
>    }


I think it would make more sense to leave the printing of the new and the placement arguments to the CXXOperatorCallExpr. But I'm not very sure about this opinion.

It appears that there are quite a few places that get more complicated with this, e.g. the need to represent the allocation size expression in the AST. We need to find a way to improve this.

Sebastian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120330/6a839198/attachment.html>