[clang] Remove assumption about SourceLocation alignment.

[simon_tatham]

Summary

This is part of a patch series working towards the ability to make
SourceLocation into a 64-bit type to handle larger translation units.

In ObjCMethodDecl, an array of pointers (parameters) and an array of
SourceLocations are stored end to end in a single allocated piece of
memory. The offset within that memory where the second array begins
was being computed in a really simple way, based on the assumption
that pointers had at least as much alignment requirement as
SourceLocations. But when pointers and SourceLocations can each be
either 32 or 64 bits, this won't be a reliable assumption any more.

This patch separates the two types of data into two separate
allocations, so that there's no need to do a tricky offset computation.

[tmatheson]

clang/lib/AST/DeclObjC.cpp

880–882

Since we know the number of parameters and number of sellocs when we do the allocation, is there a reason we can't we split it into two separate allocations and reduce the complexity?

[simon_tatham]

Split up the allocations as suggested.

[simon_tatham]

... and removed an unused function from the previous version.

[simon_tatham]

clang/lib/AST/DeclObjC.cpp

31

Oops, before anyone else points it out, this #include is also now unnecessary.

[tmatheson]

LGTM, but it would be good to have someone else comment on the increased number of allocations (maybe @rsmith or @efriedma?)

clang/include/clang/AST/DeclObjC.h

199–200

I don't think you need the const_cast

[simon_tatham]

clang/include/clang/AST/DeclObjC.h

199–200

I put it in because I got a compile error otherwise!

It's because of the double indirection. You don't need a cast to turn a Foo * into a const Foo *. But here, we're turning a Foo * into a const Bar *, where Foo and Bar are pointer types to things of different constness.

[simon_tatham]

Addressed two nits.

clang/include/clang/AST/DeclObjC.h

199–200

... ok, no, removing the const_cast now, I don't get a compile error. In that case there must have been something else weird about the version of the code where I had to add it, but I can't reconstruct what.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

clang/include/clang/AST/DeclObjC.h

 
 private:
   /// Return type of this method.
   QualType MethodDeclType;
 
   /// Type source information for the return type.
   TypeSourceInfo *ReturnTInfo;
 
-  /// Array of ParmVarDecls for the formal parameters of this method
-  /// and optionally followed by selector locations.
-  void *ParamsAndSelLocs = nullptr;
+  /// Array of the formal parameters of this method.
+  ParmVarDecl **Params = nullptr;
   unsigned NumParams = 0;
 
+  /// Source locations for the selector identifiers.
+  SourceLocation *SelLocs = nullptr;
+
   /// List of attributes for this method declaration.
   SourceLocation DeclEndLoc; // the location of the ';' or '{'.
 
   /// The following are only used for method definitions, null otherwise.
   LazyDeclStmtPtr Body;
 
   /// SelfDecl - Decl for the implicit self parameter. This is lazily
   /// constructed by createImplicitParams.
private:
   }
 
   bool hasStandardSelLocs() const {
     return getSelLocsKind() != SelLoc_NonStandard;
   }
 
   /// Get a pointer to the stored selector identifiers locations array.
   /// No locations will be stored if HasStandardSelLocs is true.
-  SourceLocation *getStoredSelLocs() {
-    return reinterpret_cast<SourceLocation *>(getParams() + NumParams);
-  }
-  const SourceLocation *getStoredSelLocs() const {
-    return reinterpret_cast<const SourceLocation *>(getParams() + NumParams);
-  }
+  SourceLocation *getStoredSelLocs() { return SelLocs; }
+  const SourceLocation *getStoredSelLocs() const { return SelLocs; }
 
-  /// Get a pointer to the stored selector identifiers locations array.
-  /// No locations will be stored if HasStandardSelLocs is true.
-  ParmVarDecl **getParams() {
-    return reinterpret_cast<ParmVarDecl **>(ParamsAndSelLocs);
-  }
-  const ParmVarDecl *const *getParams() const {
-    return reinterpret_cast<const ParmVarDecl *const *>(ParamsAndSelLocs);
-  }
+  /// Get a pointer to the parameter array.
+  ParmVarDecl **getParams() { return Params; }
+  const ParmVarDecl *const *getParams() const { return Params; }
 
   /// Get the number of stored selector identifiers locations.
   /// No locations will be stored if HasStandardSelLocs is true.
   unsigned getNumStoredSelLocs() const {
     if (hasStandardSelLocs())
       return 0;
     return getNumSelectorLocs();
   }

clang/lib/AST/DeclObjC.cpp

 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <cstring>
 #include <queue>
 #include <utility>
 
 using namespace clang;

 void ObjCMethodDecl::setAsRedeclaration(const ObjCMethodDecl *PrevMethod) {
   assert(PrevMethod);
   getASTContext().setObjCMethodRedeclaration(PrevMethod, this);
   setIsRedeclaration(true);
   PrevMethod->setHasRedeclaration(true);
 }
 
 void ObjCMethodDecl::setParamsAndSelLocs(ASTContext &C,
-                                         ArrayRef<ParmVarDecl*> Params,
-                                         ArrayRef<SourceLocation> SelLocs) {
-  ParamsAndSelLocs = nullptr;
-  NumParams = Params.size();
-  if (Params.empty() && SelLocs.empty())
-    return;
-
-  static_assert(alignof(ParmVarDecl *) >= alignof(SourceLocation),
-                "Alignment not sufficient for SourceLocation");
+                                         ArrayRef<ParmVarDecl *> ParamsIn,
+                                         ArrayRef<SourceLocation> SelLocsIn) {
+  Params = nullptr;
+  NumParams = ParamsIn.size();
+  SelLocs = nullptr;
+
+  if (!ParamsIn.empty()) {
+    Params = C.Allocate<ParmVarDecl *>(ParamsIn.size());
+    std::copy(ParamsIn.begin(), ParamsIn.end(), Params);
+  }
 
-  unsigned Size = sizeof(ParmVarDecl *) * NumParams +
-                  sizeof(SourceLocation) * SelLocs.size();
-  ParamsAndSelLocs = C.Allocate(Size);
-  std::copy(Params.begin(), Params.end(), getParams());
-  std::copy(SelLocs.begin(), SelLocs.end(), getStoredSelLocs());
+  if (!SelLocsIn.empty()) {
+    SelLocs = C.Allocate<SourceLocation>(SelLocsIn.size());
+    std::copy(SelLocsIn.begin(), SelLocsIn.end(), SelLocs);
+  }
 }
 
 void ObjCMethodDecl::getSelectorLocs(
                                SmallVectorImpl<SourceLocation> &SelLocs) const {
   for (unsigned i = 0, e = getNumSelectorLocs(); i != e; ++i)
     SelLocs.push_back(getSelectorLoc(i));
 }