[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:4967
+template <typename T> static auto getTemplateDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)
----------------
shafik wrote:
> shafik wrote:
> > Can we guarantee that `D->getTemplatedDecl()` will always return a valid pointer? 
> What other types besides `CXXRecordDecl` do we expect here? 
> Can we guarantee that D->getTemplatedDecl() will always return a valid pointer?

Actually, we always call this function on a `ClassTemplateDecl` or on a `FunctionTemplateDecl` which should return with a valid pointer. Still it is a good idea to put an assert at the beginning of the function, so I just did that.
```
  assert(D->getTemplatedDecl() && "Should be called on templates only");
```

> What other types besides CXXRecordDecl do we expect here?

`FunctionDecl` can be there too when we call this on a `FunctiomTemplateDecl` param.



================
Comment at: lib/AST/ASTImporter.cpp:5544
   // type, and in the same context as the function we're importing.
+  // FIXME Split this into a separate function.
   if (!LexicalDC->isFunctionOrMethod()) {
----------------
shafik wrote:
> Would it make sense to do the split into a separate function in the PR?
I'd rather do that in another patch where we could address the similar issues in other visit functions too. Actually, most of the visit functions start with a lookup and then continue with a loop over the lookup results; this part could be split into a separate function everywhere.


================
Comment at: lib/AST/ASTImporter.cpp:5595
+      auto *PrevTemplated =
+          FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
+      if (TemplatedFD != PrevTemplated)
----------------
shafik wrote:
> Can we guarantee that `FoundByLookup->getTemplatedDecl()` will always return a valid pointer? 
This is a good catch, thanks.
The `FoundByLookup` variable's type is `FunctionTemplateDecl`. Either the found Decl is created by the parser then it must have set up a templated decl, or it is created by the ASTImporter and we should have a templated decl set.
In the second case, however, we may not be 100% sure that the templated is really set.
So I have added an assert here too.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58494/new/

https://reviews.llvm.org/D58494