<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Sorry for the delay in response.<br>
<br>
On 02/20/2015 05:00 PM, David Blaikie wrote:<br>
</div>
<blockquote
cite="mid:CAENS6Etic4RhbjvtkZmCWRuU6-jSt-fL38RWiAx+cyFp0Ej7Vw@mail.gmail.com"
type="cite">
<div dir="ltr">On Fri, Feb 20, 2015 at 4:41 PM, Philip Reames <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks
for the cleanup. Responses inline to some parts.
<div>
<div class="h5"><br>
On 02/20/2015 03:44 PM, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: dblaikie<br>
Date: Fri Feb 20 17:44:24 2015<br>
New Revision: 230094<br>
<br>
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project?rev=230094&view=rev"
target="_blank">http://llvm.org/viewvc/llvm-project?rev=230094&view=rev</a><br>
Log:<br>
Remove some unnecessary unreachables in favor of
(sometimes implicit) assertions<br>
<br>
Also simplify some else-after-return cases including
some standard<br>
algorithm convenience/use.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/IR/Instructions.h<br>
llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp<br>
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230094&r1=230093&r2=230094&view=diff"
target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=230094&r1=230093&r2=230094&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
(original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
Fri Feb 20 17:44:24 2015<br>
@@ -150,17 +150,16 @@ static bool
isLiveGCReferenceAt(Value &V<br>
static bool isAggWhichContainsGCPtrType(Type *Ty)
{<br>
if (VectorType *VT =
dyn_cast<VectorType>(Ty))<br>
return isGCPointerType(VT->getScalarType());<br>
- else if (ArrayType *AT =
dyn_cast<ArrayType>(Ty)) {<br>
+ if (ArrayType *AT =
dyn_cast<ArrayType>(Ty))<br>
return isGCPointerType(AT->getElementType())
||<br>
isAggWhichContainsGCPtrType(AT->getElementType());<br>
- } else if (StructType *ST =
dyn_cast<StructType>(Ty)) {<br>
- bool UnsupportedType = false;<br>
- for (Type *SubType : ST->subtypes())<br>
- UnsupportedType |=<br>
- isGCPointerType(SubType) ||
isAggWhichContainsGCPtrType(SubType);<br>
- return UnsupportedType;<br>
- } else<br>
- return false;<br>
+ if (StructType *ST =
dyn_cast<StructType>(Ty))<br>
+ return std::any_of(ST->subtypes().begin(),
ST->subtypes().end(),<br>
+ [](Type *SubType) {<br>
+ return
isGCPointerType(SubType) ||<br>
+
isAggWhichContainsGCPtrType(SubType);<br>
+ });<br>
+ return false;<br>
</blockquote>
</div>
</div>
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.</blockquote>
<div><br>
What is it about this code do you think is a readability
hit compared to other instances of this style guide
requirement? <br>
</div>
</div>
</div>
</div>
</blockquote>
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.<br>
<br>
A clearer form of the original would have been:<br>
if (Ty->isAggregateTy())<br>
return ...;<br>
<br>
// Handle all the aggregate types<br>
if (VectorType *VT = dyn_cast<VectorType>(Ty))<br>
return ...<br>
else if (ArrayType *AT = dyn_cast<ArrayType>(Ty))<br>
return ...<br>
else if (StructType *ST = dyn_cast<StructType>(Ty))<br>
return ...<br>
else<br>
llvm_unreachable("unknown aggregate type");<br>
<br>
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:<br>
if (VectorType *VT = dyn_cast<VectorType>(Ty))<br>
return ...<br>
if (ArrayType *AT = dyn_cast<ArrayType>(Ty))<br>
return ...<br>
if (StructType *ST = dyn_cast<StructType>(Ty))<br>
return ...<br>
return ...<br>
<br>
<br>
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.<br>
<br>
Philip<br>
<br>
<br>
<br>
</body>
</html>