[llvm] r230094 - Remove some unnecessary unreachables in favor of (sometimes implicit) assertions

[Philip Reames]

Sorry for the delay in response.

On 02/20/2015 05:00 PM, David Blaikie wrote:
> On Fri, Feb 20, 2015 at 4:41 PM, Philip Reames 
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
>     Thanks for the cleanup.  Responses inline to some parts.
>
>     On 02/20/2015 03:44 PM, David Blaikie wrote:
>
>         Author: dblaikie
>         Date: Fri Feb 20 17:44:24 2015
>         New Revision: 230094
>
>         URL: http://llvm.org/viewvc/llvm-project?rev=230094&view=rev
>         Log:
>         Remove some unnecessary unreachables in favor of (sometimes
>         implicit) assertions
>
>         Also simplify some else-after-return cases including some standard
>         algorithm convenience/use.
>
>         Modified:
>              llvm/trunk/include/llvm/IR/Instructions.h
>              llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>
>         Modified:
>         llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>         URL:
>         http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230094&r1=230093&r2=230094&view=diff
>         ==============================================================================
>         ---
>         llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>         (original)
>         +++
>         llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>         Fri Feb 20 17:44:24 2015
>         @@ -150,17 +150,16 @@ static bool isLiveGCReferenceAt(Value &V
>           static bool isAggWhichContainsGCPtrType(Type *Ty) {
>             if (VectorType *VT = dyn_cast<VectorType>(Ty))
>               return isGCPointerType(VT->getScalarType());
>         -  else if (ArrayType *AT = dyn_cast<ArrayType>(Ty)) {
>         +  if (ArrayType *AT = dyn_cast<ArrayType>(Ty))
>               return isGCPointerType(AT->getElementType()) ||
>                      isAggWhichContainsGCPtrType(AT->getElementType());
>         -  } else if (StructType *ST = dyn_cast<StructType>(Ty)) {
>         -    bool UnsupportedType = false;
>         -    for (Type *SubType : ST->subtypes())
>         -      UnsupportedType |=
>         -          isGCPointerType(SubType) ||
>         isAggWhichContainsGCPtrType(SubType);
>         -    return UnsupportedType;
>         -  } else
>         -    return false;
>         +  if (StructType *ST = dyn_cast<StructType>(Ty))
>         +    return std::any_of(ST->subtypes().begin(),
>         ST->subtypes().end(),
>         +                       [](Type *SubType) {
>         +                         return isGCPointerType(SubType) ||
>         + isAggWhichContainsGCPtrType(SubType);
>         +                       });
>         +  return false;
>
>     I feel the resulting code is less readable.  I realize that these
>     are return statements inside the conditions, but I'd still prefer
>     the other pattern.  This is effectively as close to a match clause
>     as C++ can get.  I also release I'm in conflict of the letter of
>     the coding standard.  However, this is a case where I believe the
>     coding standard is wrong because it's producing less readable code.
>
>
> What is it about this code do you think is a readability hit compared 
> to other instances of this style guide requirement?
The original code was trying to be explicit about the fact that it was 
trying to cover every case in a union of possible types. Similar to 
using a switch case instead of an if-else chain, documenting the fact 
that if another case (type) gets added, the code needs updated.

A clearer form of the original would have been:
if (Ty->isAggregateTy())
   return ...;

// Handle all the aggregate types
if (VectorType *VT = dyn_cast<VectorType>(Ty))
   return ...
else if (ArrayType *AT = dyn_cast<ArrayType>(Ty))
   return ...
else if (StructType *ST = dyn_cast<StructType>(Ty))
   return ...
else
   llvm_unreachable("unknown aggregate type");

The difference is minor enough I don't really care, but I really do 
believe the above is more clear about intent than your suggestion of:
if (VectorType *VT = dyn_cast<VectorType>(Ty))
   return ...
if (ArrayType *AT = dyn_cast<ArrayType>(Ty))
   return ...
if (StructType *ST = dyn_cast<StructType>(Ty))
   return ...
return ...


Having said that, even if you agree, I don't think it's worth the effort 
to actually change it now.  It's a really minor difference.

Philip



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150303/73c08e87/attachment.html>