shafik accepted this revision.
shafik added a comment.
LGTM
================
Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:134
"specifier in SFINAE context?");
- if (!hasReachableDefinition(PartialSpec))
+ if (PartialSpec->hasDefinition() &&
+ !hasReachableDefinition(PartialSpec))
----------------
Fznamznon wrote:
> Fznamznon wrote:
> > shafik wrote:
> > > So I see in another location we are basically checking
> > > `!isBeginDefined()` and in another place `hasCompleteDefinition()`. It is
> > > not clear to me if these are all basically checking the same thing or if
> > > we should figure out what is the right thing to check and do that
> > > everywhere.
> > I suppose you meant `isBeingDefined()`. So, IMO `isBeingDefined()` is not
> > exactly the same as having a definition, this is set to `true` when
> > `TagDecl::startDefinition` I suppose this should be done when parser meets
> > a definition so at this point it can't be or have a complete definition.
> > But at the places where `!isBeingDefined()` is checked prior
> > `hasReachableDefinition` I saw a pointer representing a definition checked
> > for not being null. Interestingly, `hasReachableDefinition()` early exits
> > if `isBeingDefined()` returns true, so probably this check additional
> > doesn't make sense at all.
> > Maybe we should just move both checks on having a definition and not being
> > in the process of definition under `hasReachableDefinition` after all. That
> > will require changing unrelated places, so I would prefer doing this
> > separately in any case.
> >
> > I can't grep `hasCompleteDefinition()` either, so I suppose you meant
> > `isCompleteDefinition()`, I think this is basically the same thing as
> > having definition and additionally the declaration through which the method
> > called is checked that IT IS the definition. I'm not sure we have to
> > require it here.
> >
> @shafik , are you satisfied with the explanation?
I think we are good.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148330/new/
https://reviews.llvm.org/D148330
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
