[cfe-commits] r130492 - in /cfe/trunk: include/clang/AST/Type.h lib/AST/Type.cpp lib/Sema/SemaChecking.cpp test/SemaCXX/warn-non-pod-memset.cpp

[Douglas Gregor]

On Apr 29, 2011, at 2:46 AM, Chandler Carruth wrote:

> Author: chandlerc
> Date: Fri Apr 29 04:46:08 2011
> New Revision: 130492
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=130492&view=rev
> Log:
> Relax the non-POD memset warning to use the less restrictive C++11
> definition of POD. Specifically, this allows certain non-aggregate
> types due to their data members being private.
> 
> The representation of C++11 POD testing is pretty gross. Any suggestions
> for improvements there are welcome. Especially the name
> 'isCXX11PODType()' seems truly unfortunate.
> 
> Modified:
>    cfe/trunk/include/clang/AST/Type.h
>    cfe/trunk/lib/AST/Type.cpp
>    cfe/trunk/lib/Sema/SemaChecking.cpp
>    cfe/trunk/test/SemaCXX/warn-non-pod-memset.cpp
> 
> Modified: cfe/trunk/include/clang/AST/Type.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=130492&r1=130491&r2=130492&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Type.h (original)
> +++ cfe/trunk/include/clang/AST/Type.h Fri Apr 29 04:46:08 2011
> @@ -1179,6 +1179,12 @@
>   /// (C++0x [basic.types]p9)
>   bool isTrivialType() const;
> 
> +  /// isCXX11PODType() - Return true if this is a POD type according to the
> +  /// more relaxed rules of the C++11 standard, regardless of the current
> +  /// compilation's language.
> +  /// (C++0x [basic.types]p9)
> +  bool isCXX11PODType() const;
> +
>   /// Helper methods to distinguish type categories. All type predicates
>   /// operate on the canonical type, ignoring typedefs and qualifiers.
> 
> 
> Modified: cfe/trunk/lib/AST/Type.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Type.cpp?rev=130492&r1=130491&r2=130492&view=diff
> ==============================================================================
> --- cfe/trunk/lib/AST/Type.cpp (original)
> +++ cfe/trunk/lib/AST/Type.cpp Fri Apr 29 04:46:08 2011
> @@ -943,6 +943,52 @@
>   return false;
> }
> 
> +// This is effectively the intersection of isTrivialType and hasStandardLayout.
> +// We implement it dircetly to avoid redundant conversions from a type to

Typo "dircetly".

> +// a CXXRecordDecl.
> +bool Type::isCXX11PODType() const {
> +  if (isIncompleteType())
> +    return false;

Type::isPODType() handles incomplete arrays of POD types differently. Unless you have a reason to have a different result, I suggest following it. 

> +  // C++11 [basic.types]p9:
> +  //   Scalar types, POD classes, arrays of such types, and cv-qualified
> +  //   versions of these types are collectively called trivial types.
> +  const Type *BaseTy = getBaseElementTypeUnsafe();
> +  assert(BaseTy && "NULL element type");
> +  if (BaseTy->isScalarType()) return true;
> +  if (const RecordType *RT = BaseTy->getAs<RecordType>()) {
> +    if (const CXXRecordDecl *ClassDecl =
> +        dyn_cast<CXXRecordDecl>(RT->getDecl())) {
> +      // C++11 [class]p10:
> +      //   A POD struct is a non-union class that is both a trivial class [...]
> +      // C++11 [class]p5:
> +      //   A trivial class is a class that has a trivial default constructor
> +      if (!ClassDecl->hasTrivialConstructor()) return false;
> +      //   and is trivially copyable.
> +      if (!ClassDecl->isTriviallyCopyable()) return false;
> +
> +      // C++11 [class]p10:
> +      //   A POD struct is a non-union class that is both a trivial class and
> +      //   a standard-layout class [...]
> +      if (!ClassDecl->hasStandardLayout()) return false;
> +
> +      // C++11 [class]p10:
> +      //   A POD struct is a non-union class that is both a trivial class and
> +      //   a standard-layout class, and has no non-static data members of type
> +      //   non-POD struct, non-POD union (or array of such types). [...]
> +      //
> +      // We don't directly query the recursive aspect as the requiremets for
> +      // both standard-layout classes and trivial classes apply recursively
> +      // already.
> +    }
> +
> +    return true;
> +  }

You're not accounting for vector types here. How about following the implementation of Type::isPODType() more closely, or (better yet) factoring out the common checks so there's one isPODImpl(bool CXX0xSemantics)?

> +  // No other types can match.
> +  return false;
> +}
> +
> bool Type::isPromotableIntegerType() const {
>   if (const BuiltinType *BT = getAs<BuiltinType>())
>     switch (BT->getKind()) {
> 
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=130492&r1=130491&r2=130492&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Apr 29 04:46:08 2011
> @@ -1812,21 +1812,34 @@
> 
>   const Expr *Dest = Call->getArg(0)->IgnoreParenImpCasts();
> 
> +  // The type checking for this warning is moderately expensive, only do it
> +  // when enabled.
> +  if (getDiagnostics().getDiagnosticLevel(diag::warn_non_pod_memset,
> +                                          Dest->getExprLoc()) ==
> +      Diagnostic::Ignored)
> +    return;
> +
>   QualType DestTy = Dest->getType();
>   if (const PointerType *DestPtrTy = DestTy->getAs<PointerType>()) {
>     QualType PointeeTy = DestPtrTy->getPointeeType();
> -    if (!PointeeTy->isPODType() && !PointeeTy->isVoidType()) {
> -      DiagRuntimeBehavior(
> -        Dest->getExprLoc(), Dest,
> -        PDiag(diag::warn_non_pod_memset)
> -          << PointeeTy << Call->getCallee()->getSourceRange());
> -
> -      SourceRange ArgRange = Call->getArg(0)->getSourceRange();
> -      DiagRuntimeBehavior(
> -        Dest->getExprLoc(), Dest,
> -        PDiag(diag::note_non_pod_memset_silence)
> -          << FixItHint::CreateInsertion(ArgRange.getBegin(), "(void*)"));
> -    }
> +    if (PointeeTy->isVoidType())
> +      return;
> +
> +    // Check the C++11 POD definition regardless of language mode; it is more
> +    // relaxed than earlier definitions and we don't want spurrious warnings.

Typo "spurrious".

	- Doug