Storing the endloc is not necessary (and was done incorrectly)

Author: cor3ntin

clang/include/clang/AST/Expr.h

@@ -2911,11 +2911,10 @@ class CallExpr : public Expr {
   // itself is 24 bytes. To avoid having to recompute or store the offset of the
   // trailing objects, we put it at 32 bytes (such that it is suitable for all
   // subclasses) We use the 8 bytes gap left for instances of CallExpr to store
-  // the begin and end source locations. Caching the begin source location in
-  // particular as a significant impact on perf as getBeginLoc is assumed to be
-  // cheap.
+  // the begin source location, which has a significant impact on perf as
+  // getBeginLoc is assumed to be cheap.
   // The layourt is as follow:
-  // CallExpr | Begin | End | Trailing Objects
+  // CallExpr | Begin | 4 bytes left | Trailing Objects
   // CXXMemberCallExpr | Trailing Objects
   // A bit in CallExprBitfields indicates if source locations are presents.
 
@@ -3051,9 +3050,9 @@ class CallExpr : public Expr {
     CallExprBits.ExplicitObjectMemFunUsingMemberSyntax = V;
     // Because the source location may be different for explicit
     // member, we reset the cached values.
-    if (CallExprBits.HasTrailingSourceLocs) {
-      CallExprBits.HasTrailingSourceLocs = false;
-      updateTrailingSourceLocs();
+    if (CallExprBits.HasTrailingSourceLoc) {
+      CallExprBits.HasTrailingSourceLoc = false;
+      updateTrailingSourceLoc();
     }
   }
 
@@ -3216,19 +3215,14 @@ class CallExpr : public Expr {
   SourceLocation getRParenLoc() const { return RParenLoc; }
   void setRParenLoc(SourceLocation L) { RParenLoc = L; }
 
-  template <unsigned N> SourceLocation getTrailingSourceLoc() const {
-    static_assert(N <= 1);
-    assert(CallExprBits.HasTrailingSourceLocs && "No trailing source loc");
-    static_assert(sizeof(CallExpr) <=
-                  offsetToTrailingObjects + 2 * sizeof(SourceLocation));
-    return *reinterpret_cast<const SourceLocation *>(
-        reinterpret_cast<const char *>(this) + sizeof(CallExpr) +
-        sizeof(SourceLocation) * N);
-  }
-
   SourceLocation getBeginLoc() const {
-    if (CallExprBits.HasTrailingSourceLocs)
-      return getTrailingSourceLoc<0>();
+    if (CallExprBits.HasTrailingSourceLoc) {
+        assert(CallExprBits.HasTrailingSourceLoc && "No trailing source loc");
+        static_assert(sizeof(CallExpr) <=
+                      offsetToTrailingObjects + sizeof(SourceLocation));
+        return *reinterpret_cast<const SourceLocation *>(
+            reinterpret_cast<const char *>(this) + sizeof(CallExpr));
+    }
 
     if (usesMemberSyntax()) {
       if (auto FirstArgLoc = getArg(0)->getBeginLoc(); FirstArgLoc.isValid()) {
@@ -3239,22 +3233,17 @@ class CallExpr : public Expr {
   }
 
   SourceLocation getEndLoc() const {
-    if (CallExprBits.HasTrailingSourceLocs)
-      return getTrailingSourceLoc<0>();
-
-    SourceLocation end = getRParenLoc();
-    if (end.isInvalid() && getNumArgs() > 0 && getArg(getNumArgs() - 1))
-      end = getArg(getNumArgs() - 1)->getEndLoc();
-    return end;
+    return getRParenLoc();
   }
 
 private:
+  friend class ASTStmtReader;
   bool hasTrailingSourceLoc() const {
-    return CallExprBits.HasTrailingSourceLocs;
+    return CallExprBits.HasTrailingSourceLoc;
   }
 
-  void updateTrailingSourceLocs() {
-    assert(!CallExprBits.HasTrailingSourceLocs &&
+  void updateTrailingSourceLoc() {
+    assert(!CallExprBits.HasTrailingSourceLoc &&
            "Trailing source loc already set?");
     assert(getStmtClass() == CallExprClass &&
            "Calling setTrailingSourceLocs on a subclass of CallExpr");
@@ -3264,8 +3253,7 @@ class CallExpr : public Expr {
     SourceLocation *Locs = reinterpret_cast<SourceLocation *>(
         reinterpret_cast<char *>(this) + sizeof(CallExpr));
     new (Locs) SourceLocation(getBeginLoc());
-    new (Locs + 1) SourceLocation(getEndLoc());
-    CallExprBits.HasTrailingSourceLocs = true;
+    CallExprBits.HasTrailingSourceLoc = true;
   }
 
 public:

clang/include/clang/AST/Stmt.h

@@ -574,7 +574,7 @@ class alignas(void *) Stmt {
     /// Indicates that SourceLocations are cached as
     /// Trailing objects. See the definition of CallExpr.
     LLVM_PREFERRED_TYPE(bool)
-    unsigned HasTrailingSourceLocs : 1;
+    unsigned HasTrailingSourceLoc : 1;
   };
 
   enum { NumCallExprBits = 25 };

clang/lib/AST/Expr.cpp

@@ -1494,7 +1494,7 @@ CallExpr::CallExpr(StmtClass SC, Expr *Fn, ArrayRef<Expr *> PreArgs,
   CallExprBits.HasFPFeatures = FPFeatures.requiresTrailingStorage();
   CallExprBits.IsCoroElideSafe = false;
   CallExprBits.ExplicitObjectMemFunUsingMemberSyntax = false;
-  CallExprBits.HasTrailingSourceLocs = false;
+  CallExprBits.HasTrailingSourceLoc = false;
 
   if (hasStoredFPFeatures())
     setStoredFPFeatures(FPFeatures);
@@ -1508,7 +1508,7 @@ CallExpr::CallExpr(StmtClass SC, unsigned NumPreArgs, unsigned NumArgs,
   CallExprBits.HasFPFeatures = HasFPFeatures;
   CallExprBits.IsCoroElideSafe = false;
   CallExprBits.ExplicitObjectMemFunUsingMemberSyntax = false;
-  CallExprBits.HasTrailingSourceLocs = false;
+  CallExprBits.HasTrailingSourceLoc = false;
 }
 
 CallExpr *CallExpr::Create(const ASTContext &Ctx, Expr *Fn,
@@ -1525,7 +1525,7 @@ CallExpr *CallExpr::Create(const ASTContext &Ctx, Expr *Fn,
   CallExpr *E =
       new (Mem) CallExpr(CallExprClass, Fn, /*PreArgs=*/{}, Args, Ty, VK,
                          RParenLoc, FPFeatures, MinNumArgs, UsesADL);
-  E->updateTrailingSourceLocs();
+  E->updateTrailingSourceLoc();
   return E;
 }
 

clang/lib/Serialization/ASTReaderStmt.cpp

@@ -1049,7 +1049,7 @@ void ASTStmtReader::VisitCallExpr(CallExpr *E) {
         FPOptionsOverride::getFromOpaqueInt(Record.readInt()));
 
   if (E->getStmtClass() == Stmt::CallExprClass)
-    E->updateTrailingSourceLocs();
+    E->updateTrailingSourceLoc();
 }
 
 void ASTStmtReader::VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {