[PATCH] D148330: [clang] Do not crash on undefined template partial specialization

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.


================
Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:134
                                          "specifier in SFINAE context?");
-            if (!hasReachableDefinition(PartialSpec))
+            if (PartialSpec->hasDefinition() &&
+                !hasReachableDefinition(PartialSpec))
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148330