[PATCH] D12169: Relax constexpr rules to improve __builtin_object_size's accuracy

[George Burgess IV via cfe-commits]

george.burgess.iv added inline comments.

================
Comment at: lib/AST/ExprConstant.cpp:4763
@@ +4762,3 @@
+  /// arithmetic.
+  bool UseStrictCastingRules;
+
----------------
rsmith wrote:
> This should be handled as an `EvaluationMode`.
Works for me.

================
Comment at: lib/AST/ExprConstant.cpp:4879-4907
@@ +4878,31 @@
+  // We need to adjust the array index we're handing off in that case.
+  QualType ArrayTy = Result.Designator.MostDerivedType;
+  if (!ArrayTy.isNull() && !ArrayTy->isIncompleteType() && ArrayTy != Pointee) {
+    CharUnits PointeeEltSize;
+    if (!HandleSizeof(Info, PExp->getExprLoc(), Pointee, PointeeEltSize))
+      return false;
+
+    CharUnits ArrayEltSize;
+    if (!HandleSizeof(Info, PExp->getExprLoc(), ArrayTy, ArrayEltSize))
+      return false;
+
+    // Additionally, because LValuePathEntry wasn't made to really handle byte
+    // offsets, we need to keep the LValue Offset up-to-date (so casting e.g.
+    // BaseToDerived works as intended), but we also need to lie a bit about the
+    // array index.
+    //
+    // Example:
+    // (char*)Foo.Bar[1] + 2 // (assuming sizeof(Foo.Bar[1]) > 2)
+    // Will have an index of 1 and an offset of offsetof(Foo.Bar[1]) + 2.
+    if (PointeeEltSize != ArrayEltSize) {
+      CharUnits ByteOffset = IndexOffset * PointeeEltSize + AddedOffset;
+      CharUnits Rem = CharUnits::fromQuantity(ByteOffset % ArrayEltSize);
+      Result.Offset += Rem - AddedOffset;
+      AddedOffset = Rem;
+      IndexOffset = ByteOffset / ArrayEltSize;
+    }
+
+    // HandleLValueArrayAdjustment takes the type of the array
+    Pointee = ArrayTy;
+  }
+
----------------
rsmith wrote:
> We should not do this except in your special new mode, and I'm not entirely convinced we should support this case at all. Just discarding the designator (as we would do anyway in constant-folding mode) seems like an acceptable response here. We really don't know what subobject is being referenced any more.
We need some way to support `BOS((char*)&Foo.Bar + 1, N)`, so this needs to go somewhere. I originally had it in its own method that was super similar to this one, and the duplication made me sad. Will re-duplicate and see how that looks.

FYI, if you'll look at the diff at L194 in p2-0x.cpp, we have an interesting feature in the non-patched impl where `B *b = (B*)((A*)&b + 1)` is at an offset of sizeof(A), but Designator says it's at b[1]. I assumed this behavior is a bug, and this kinda-fixes it. If it's intended behavior, I'll absolutely find a better way to handle this.

================
Comment at: lib/AST/ExprConstant.cpp:6355-6356
@@ +6354,4 @@
+bool OutermostMemberEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
+  Result = E->getType();
+  return true;
+}
----------------
rsmith wrote:
> I think handling this through the normal pointer evaluator, but with a different `EvaluationMode`, is a better approach. (We need some representation for an `LValueBase` to represent "unknown base", but everything else should fall through pretty naturally.)
Sounds great to me -- I thought another visitor seemed heavyweight, but didn't consider adding another `EvaluationMode`. Will look into swapping approaches.


http://reviews.llvm.org/D12169