[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Shafik, thanks for addressing the comments. This looks good to me now!



================
Comment at: lib/AST/ASTImporter.cpp:3042
 
+  if (FoundByLookup) {
+    if (auto *MD = dyn_cast<CXXMethodDecl>(FoundByLookup)) {
----------------
shafik wrote:
> martong wrote:
> > It is not trivial why we add this block to the code, so I think a comment would be really appreciated here.
> > I was thinking about something like this:
> > ```
> > +  // We do not allow more than one in-class prototype of a function.  This is
> > +  // because AST clients like VTableBuilder asserts on this.  VTableBuilder
> > +  // assumes that only one function can override a function. Building a redecl
> > +  // chain would result there are more than one function which override the
> > +  // base function (even if they are part of the same redecl chain inside the
> > +  // derived class.)
> > ```
> Just for clarification, from a C++ perspective, I would say "in-class declaration" rather than "in-class prototype" and therefore `VTableBuilder` assumes only one in class declaration and building a redecal chain would result in more than one in-class declaration being present. 
> 
> Does that makes sense?
Yes, absolutely. Please change the comment as you suggest.


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

https://reviews.llvm.org/D56936