[clang] remove ClassScopeFunctionSpecializationDecl

[sdkrystian]

This removes the ClassScopeFunctionSpecializationDecl Decl node, and instead uses DependentFunctionTemplateSpecializationInfo to handle such declarations. DependentFunctionTemplateSpecializationInfo is also changed to store a const ASTTemplateArgumentListInfo* to be more in line with FunctionTemplateSpecializationInfo.

This also changes FunctionDecl::isFunctionTemplateSpecialization to return true for dependent specializations, and FunctionDecl::getTemplateSpecializationKind/FunctionDecl::getTemplateSpecializationKindForInstantiation to return TSK_ExplicitSpecialization for non-friend dependent specializations (the same behavior as dependent class scope ClassTemplateSepcializationDecl & VarTemplateSepcializationDecl).

[cor3ntin]

@erichkeane Opinions?

[erichkeane]

I guess my first question is 'why'? My second is 'if this is unnecessary, why was it here in the first place?`. I'd like some level of historical analysis here/perhaps the knowledge of @zygoloid.

[sdkrystian]

@erichkeane

I guess my first question is 'why'?

Per the comment you mentioned, it is redundant. Switching to DependentFunctionTemplateSpecializationInfo also results in constructs such as:

template<typename T>
struct A {
  template<>
  void f();
};

being diagnosed prior to instantiation. Class scope explicit specializations currently exist in their own broken little corner of the frontend, so unifying their representation in the AST would be the first step towards properly fixing them.

My second is 'if this is unnecessary, why was it here in the first place?`.

ClassScopeFunctionSpecializationDecl was originally added in this commit to "parse MSVC standard C++ headers, MFC and ATL code." Back then, class scope explicit specializations were a Microsoft extension, but since CWG727 they are valid ISO C++. As for why this was not originally implemented using DependentFunctionTemplateSpecializationInfo, I cannot say.

[erichkeane]

I didn't spend much time in Serialization, so hopefully someone else can take a look, but in general I think I'm warming toward this.

[shafik]

Mostly minor fixes and or questions.

[sdkrystian]

Ping @erichkeane & @shafik -- I left comments responding to your feedback

[erichkeane]

I don't have any comments, though someone with knowledge of teh clang-tools-extra stuff would be nice to take a look, as well as someone familiar with the serialization. @AaronBallman .

ALSO, this might need a release note and does this need to be mailed to the 'breaking changes' vendor list since it is significantly going to change the AST?

[AaronBallman]

The changes should definitely have a release note so that downstreams can be aware of the changes. This isn't really a potentially breaking change, but we should alert clang-vendors in case a downstream actually cares.

[sdkrystian]

@AaronBallman

Can you be sure to file an issue or add a test case + FIXME comment so we don't lose track of the needed follow-up work, or are you planning to do that work immediately after this lands?

I will address that once this gets merged

[AaronBallman]

CC @llvm/clang-vendors for awareness of the AST changing to remove a node.

[sdkrystian]

Updated the release notes. I didn't go into exhaustive detail, but I can add more if anyone here deems it necessary @erichkeane @AaronBallman

[AaronBallman]

LGTM aside from a minor suggestion with the release note.

[AaronBallman]

Do you need someone to land these changes on your behalf?

[sdkrystian]

@AaronBallman I don't have write access, so that would be appreciated 🙂. I was putting off rebasing incase there were more responses to my comments, but I presume that there won't be so I will get it rebased now

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[erichkeane]

If Aaron doesn't get to it, feel free to ping me on Monday. Its too late on a Friday for me to be able to merge this for you, but monday morning would be fine for me.