This should be a good starter task, as the behavior after the refactoring should not change. There is no new functionality to be added. The main challenge would be to get the compiler building properly and being careful while making individual changes.

Please feel free to ask for help or code review (I'm varungandhi-apple on GitHub). 🙂

@swift-ci create

Comment by Matei Oprea (JIRA)

Let's try to fix this. Any hints? I know that it might be difficult jumping from SR-13237 to this, but I'll try.

I'm not sure what other hints I can provide, I wrote down what I thought might help in the description. � Is something in the description unclear? Happy to answer more specific questions.

Comment by Matei Oprea (JIRA)

Does it makes sense to use totalSizeToAlloc when context.Allocate only allocs the size of type?

E.g.:

void *mem = ctx.Allocate(sizeof(DifferentiableAttr), alignof(DifferentiableAttr));

@dan-zheng, Matei ran into the question above ^. I would prefer that we consistently use one pattern to allocate throughout the codebase, where we specify all the trailing objects with their counts, even if the counts are zero. So these two pieces of code would change slightly:

// Before
DifferentiableAttr::create(ASTContext &context, ...)

  size_t size = totalSizeToAlloc<ParsedAutoDiffParameter>(parameters.size());
  void *mem = context.Allocate(size, alignof(DifferentiableAttr));

DifferentiableAttr::create(AbstractFunctionDecl *original, ...)

  void *mem = ctx.Allocate(sizeof(DifferentiableAttr),
                           alignof(DifferentiableAttr));

// After (ignore formatting/line-width)
DifferentiableAttr::create(ASTContext &context, ...)

  unsigned size = totalSizeToAlloc<DifferentiableAttr, ParsedAutoDiffParameter>(0, parameters.size());
  void *mem = context.Allocate(size, alignof(DifferentiableAttr)); 

DifferentiableAttr::create(AbstractFunctionDecl *original, ...)

  size_t size = totalSizeToAlloc<DifferentiableAttr, ParsedAutoDiffParameter>(1, 0); 
  void *mem = ctx.Allocate(size, alignof(DifferentiableAttr));

Arguably, this also makes it easier to compare the two calls at a glance.

Does this sound like a reasonable change?

Yes, that sounds reasonable!

Comment by Matei Oprea (JIRA)

Just curious, shouldn't be like:

DifferentiableAttr *
DifferentiableAttr::create(AbstractFunctionDecl *original, bool implicit,
                           SourceLoc atLoc, SourceRange baseRange, bool linear,
                           IndexSubset *parameterIndices,
                           GenericSignature derivativeGenSig) {
  auto &ctx = original->getASTContext();
  
  size_t size = totalSizeToAlloc<ParsedAutoDiffParameter>(0); 
  void *mem = ctx.Allocate(size, alignof(DifferentiableAttr)); 

Since the definition says that it includes the size of the base object.

Also, what I'm trying to understand is: why doesn't the code compile when I'm passing it totalSizeToAlloc<DifferentiableAttr>(0); since I'm trying to alloc sizeOf(DifferentiableAttr), not sizeOf(ParsedAutoDiffParameter), right?

You're right mateioprea (JIRA User) , good catch! It should be totalSizeToAlloc<ParsedAutoDiffParameter>(arg);. where arg = 0 or arg = parameters.size().

> Also, what I'm trying to understand is: why doesn't the code compile when I'm passing it totalSizeToAlloc<DifferentiableAttr>(0);

There is an explanation in the comment above additionalSizeToAlloc which shares a similar type signature as totalSizeToAlloc

  /// Returns the size of the trailing data, if an object were
  /// allocated with the given counts (The counts are in the same order
  /// as the template arguments). This does not include the size of the
  /// base object.  The template arguments must be the same as those
  /// used in the class; they are supplied here redundantly only so
  /// that it's clear what the counts are counting in callers.
  template <typename... Tys>
  static constexpr typename std::enable_if<
      std::is_same<Foo<TrailingTys...>, Foo<Tys...>>::value, size_t>::type
  additionalSizeToAlloc(typename trailing_objects_internal::ExtractSecondType<
                        TrailingTys, size_t>::type... Counts) {

  template <typename... Tys>
  static constexpr typename std::enable_if<
      std::is_same<Foo<TrailingTys...>, Foo<Tys...>>::value, size_t>::type
  totalSizeToAlloc(typename trailing_objects_internal::ExtractSecondType<
                   TrailingTys, size_t>::type... Counts) {

Comment by Matei Oprea (JIRA)

Perfect, thank you. I already have some of the changes done (with tests passing). I'll get back to you when everything is done.

Meanwhile, maybe this is not on your side, but I've tried to also fix SR-11900 and ping Brent on twitter too, but no answer. Can you help with this one too? Or it's not on your side?

> Meanwhile, maybe this is not on your side, but I've tried to also fix SR-11900 and ping Brent on twitter too, but no answer

I'm not familiar with the details of that bug report.

I don't think we have a policy on this, but speaking for myself, I would prefer that someone contact me using the bug reporting channel and check back in a few days if they don't get a response (sometimes I might be busy with other work and miss something), or ask for help from other contributors in the Compiler Development category of the Swift forums, instead of pinging me on Twitter.

Comment by Matei Oprea (JIRA)

Understood. Thanks!

Comment by Matei Oprea (JIRA)

Hi Varun,

Since some of the compiler classes do not extend the TrailingObjects class as a friend class, I assumed that this requires more than a "StarterBug" experience.

If this fix should cover all manual allocations, even the classes that do not extend TrailingObjects, I'll try to do it.

I've also created a PR and mentioned you.

Also, ran the tests on my local env and all passed.

Thanks.

PR link: #33256

(I've answered the questions in the PR).