<div dir="ltr">I'm still working on a patch. Sorry for the delay, I'm currently traveling...</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 7, 2016 at 6:46 PM Łukasz Anforowicz <<a href="mailto:lukasza@google.com">lukasza@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg">On Mon, Oct 31, 2016 at 7:18 AM, Aaron Ballman <span dir="ltr" class="gmail_msg"><<a href="mailto:aaron.ballman@gmail.com" class="gmail_msg" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br class="gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-3133883551018558048gmail- gmail_msg">On Fri, Oct 28, 2016 at 12:17 PM, Manuel Klimek <<a href="mailto:klimek@google.com" class="gmail_msg" target="_blank">klimek@google.com</a>> wrote:<br class="gmail_msg">
> Playing around with the code, we have 2 problems we need to solve:<br class="gmail_msg">
> a) the confusion between type & qualType weirdness<br class="gmail_msg">
> That got in by accident, I think, though the getDecl() specialization that<br class="gmail_msg">
> behaves differently from the partial desugaring we have due to b)<br class="gmail_msg">
> b) the partial desugaring we do<br class="gmail_msg">
> This basically was born from the idea that some of the sugaring classes we<br class="gmail_msg">
> have are really really weird for devs (like TemplateSpecializationType) or<br class="gmail_msg">
> basically useless to match (like ElaboratedType, where we can match on the<br class="gmail_msg">
> NNS if we need to).<br class="gmail_msg">
><br class="gmail_msg">
> a) seems fixable by a more fundamental patch in HasDeclarationMatcher on<br class="gmail_msg">
> which I'm working<br class="gmail_msg">
> b) seems like something we should discuss<br class="gmail_msg">
<br class="gmail_msg">
</span>I agree, thank you for looking into this and working on it!<br class="gmail_msg">
<br class="gmail_msg">
~Aaron<br class="gmail_msg">
<div class="gmail_msg"><div class="m_-3133883551018558048gmail-h5 gmail_msg"><br class="gmail_msg">
><br class="gmail_msg">
> On Fri, Oct 28, 2016 at 4:27 PM Manuel Klimek <<a href="mailto:klimek@google.com" class="gmail_msg" target="_blank">klimek@google.com</a>> wrote:<br class="gmail_msg">
>><br class="gmail_msg">
>> Now with the right cfe-dev<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> On Fri, Oct 28, 2016 at 4:14 PM Manuel Klimek <<a href="mailto:klimek@google.com" class="gmail_msg" target="_blank">klimek@google.com</a>> wrote:<br class="gmail_msg">
>>><br class="gmail_msg">
>>> Somehow we lost cfe-dev on this mail thread :( :(<br class="gmail_msg">
>>><br class="gmail_msg">
>>> adding Aaron, who has also contributed to that part of the code<br class="gmail_msg">
>>><br class="gmail_msg">
>>> On Fri, Oct 28, 2016 at 4:11 PM Manuel Klimek <<a href="mailto:klimek@google.com" class="gmail_msg" target="_blank">klimek@google.com</a>> wrote:<br class="gmail_msg">
>>>><br class="gmail_msg">
>>>> On Fri, Oct 28, 2016 at 3:54 PM Łukasz Anforowicz <<a href="mailto:lukasza@google.com" class="gmail_msg" target="_blank">lukasza@google.com</a>><br class="gmail_msg">
>>>> wrote:<br class="gmail_msg">
>>>>><br class="gmail_msg">
>>>>> Some notes from me (that hopefully will helpful to consider during the<br class="gmail_msg">
>>>>> meeting):<br class="gmail_msg">
>>>>><br class="gmail_msg">
>>>>> I agree that forcing hasDeclaration to consider all subclasses of Types<br class="gmail_msg">
>>>>> (i.e. forcing via #include "clang/AST/TypeNodes.def" in the proposal below)<br class="gmail_msg">
>>>>> seems to be a good thing. The subset of types supported today seems to be<br class="gmail_msg">
>>>>> rather arbitrary (I think; please let me know if there is good reasoning +<br class="gmail_msg">
>>>>> a way to describe the currently chosen subset of supported types).<br class="gmail_msg">
>>>>> As a user of hasDeclaration, I have to "manually" drill through *some*<br class="gmail_msg">
>>>>> can paints today. For example in<br class="gmail_msg">
>>>>> <a href="https://codereview.chromium.org/2256913002/patch/180001/190001" rel="noreferrer" class="gmail_msg" target="_blank">https://codereview.chromium.org/2256913002/patch/180001/190001</a> I am manually<br class="gmail_msg">
>>>>> jumping over pointers and references (what I want to do is to tell whether a<br class="gmail_msg">
>>>>> member dereference will end up using one of "my" classdecls where I am<br class="gmail_msg">
>>>>> renaming methods and fields) - see |blink_qual_type_matcher| in that<br class="gmail_msg">
>>>>> Chromium code review. Manually stripping pointers and references is doable<br class="gmail_msg">
>>>>> but 1) it is easy to get wrong (i.e. strip only one level) and 2) it is<br class="gmail_msg">
>>>>> questionable whether other required stripping is not missed (i.e. auto<br class="gmail_msg">
>>>>> type?),<br class="gmail_msg">
>>>>> Today hasDeclaration doesn't match "the outermost can in our russian<br class="gmail_msg">
>>>>> doll" as suggested (or proposed) by Manual. Today desugaring is done in an<br class="gmail_msg">
>>>>> arbitrary order of types - some (deep) types will be preferred over other<br class="gmail_msg">
>>>>> (shallow) types as shown in an example in the separate doc. In the example,<br class="gmail_msg">
>>>>> even though the typeAliasDecl is the most shallow desugaring result, it will<br class="gmail_msg">
>>>>> *never* be matched because hasDeclaration will first try (and latch onto)<br class="gmail_msg">
>>>>> other types first.<br class="gmail_msg">
>>>>> "has" vs "hasDescendant" is not a perfectly valid analogy.<br class="gmail_msg">
>>>>> "hasDeclaration" is not like "has" today, because even today it can strip<br class="gmail_msg">
>>>>> multiple layers of sugar (see the example from the previous bullet item).<br class="gmail_msg">
>>>>><br class="gmail_msg">
>>>>> So:<br class="gmail_msg">
>>>>><br class="gmail_msg">
>>>>> I would vote to make "hasDeclaration" match at every desugaring level<br class="gmail_msg">
>>>>> (i.e. without splitting it into a "has" and a "hasDescendant"-like variant).<br class="gmail_msg">
>>>>> Having "hasDeclaration" and "eventuallyDesugarsToDeclaration"<br class="gmail_msg">
>>>>> ("hasDeepDeclaration"?) sounds like an okay option, but it seems to me that<br class="gmail_msg">
>>>>> even today 1) hasDeclaration code and 2) users of hasDeclaration, behave<br class="gmail_msg">
>>>>> like or expect behavior of "eventuallyDesugarsToDeclaration"<br class="gmail_msg">
>>>><br class="gmail_msg">
>>>> I'm still looking into where we made the fundamental error that we're<br class="gmail_msg">
>>>> running into those discussions now. There should be something simpler so<br class="gmail_msg">
>>>> that the behavior more clearly matches the design of the clang AST.<br class="gmail_msg">
>>>>><br class="gmail_msg">
>>>>> I hope that the discussion of "hasDeclaration" vs "hasDeepDeclaration"<br class="gmail_msg">
>>>>> can be independent from, and not block the fixes from<br class="gmail_msg">
>>>>> <a href="https://reviews.llvm.org/D24361" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D24361</a> (i.e. being able to land them in the next<br class="gmail_msg">
>>>>> 1-2 would be great).<br class="gmail_msg">
>>>><br class="gmail_msg">
>>>> As Richard noted on the doc, it really depends on which direction we<br class="gmail_msg">
>>>> want to go in, and whether the patch is a step in the right direction. Sorry<br class="gmail_msg">
>>>> :(<br class="gmail_msg">
>>>> But please continue pinging / making sure we drive this forward - I<br class="gmail_msg">
>>>> think this is important, and I'm happy to spend some time to make sure we<br class="gmail_msg">
>>>> get this right, and thanks for all the work you've already put into this, I<br class="gmail_msg">
>>>> really appreciate it, and I think it's helping us getting to the root of the<br class="gmail_msg">
>>>> problem.<br class="gmail_msg"></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Ping :-) Any progress here? (both in 1) the general direction discussion as well as 2) the short-term planning/impact for ElaboratedType handling in <a href="https://reviews.llvm.org/D24361" class="gmail_msg" target="_blank">https://reviews.llvm.org/D24361</a> ?) </div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_msg"><div class="m_-3133883551018558048gmail-h5 gmail_msg">
>>>><br class="gmail_msg">
>>>>><br class="gmail_msg">
>>>>><br class="gmail_msg">
>>>>> On Thu, Oct 27, 2016 at 1:12 AM, Manuel Klimek <<a href="mailto:klimek@google.com" class="gmail_msg" target="_blank">klimek@google.com</a>><br class="gmail_msg">
>>>>> wrote:<br class="gmail_msg">
>>>>>><br class="gmail_msg">
>>>>>> +ben & sam<br class="gmail_msg">
>>>>>><br class="gmail_msg">
>>>>>> On Wed, Oct 26, 2016 at 6:50 PM Łukasz Anforowicz <<a href="mailto:lukasza@google.com" class="gmail_msg" target="_blank">lukasza@google.com</a>><br class="gmail_msg">
>>>>>> wrote:<br class="gmail_msg">
>>>>>>><br class="gmail_msg">
>>>>>>> On Wed, Oct 26, 2016 at 9:46 AM, Łukasz Anforowicz<br class="gmail_msg">
>>>>>>> <<a href="mailto:lukasza@google.com" class="gmail_msg" target="_blank">lukasza@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> So - is it fair to say that the CL under review can proceed as-is<br class="gmail_msg">
>>>>>>>> (*) given that:<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> The ordering problem has existed *before* the CL under review<br class="gmail_msg">
>>>>>>>> (admittedly the CL under review adds 2 more cases to the list of problematic<br class="gmail_msg">
>>>>>>>> ordering scenarios)<br class="gmail_msg">
>>>>>>>> Below it seems that we have a promising direction for solving the<br class="gmail_msg">
>>>>>>>> ordering problem (the solution probably requires a slightly bigger CL - one<br class="gmail_msg">
>>>>>>>> that also tackles the question of what hasDeclaration should do about<br class="gmail_msg">
>>>>>>>> AutoType, PointerType, etc.)<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> -Lukasz<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> (*) I know that I need to add documentation changes to mention<br class="gmail_msg">
>>>>>>>> ElaboratedType<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> On Tue, Oct 25, 2016 at 5:02 PM, Richard Smith<br class="gmail_msg">
>>>>>>>> <<a href="mailto:richardsmith@google.com" class="gmail_msg" target="_blank">richardsmith@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>><br class="gmail_msg">
>>>>>>>>> On 25 October 2016 at 16:47, Łukasz Anforowicz <<a href="mailto:lukasza@google.com" class="gmail_msg" target="_blank">lukasza@google.com</a>><br class="gmail_msg">
>>>>>>>>> wrote:<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> I think I understand the concern - the concern is that desugaring<br class="gmail_msg">
>>>>>>>>>> done in HasDeclarationMatcher::matchesSpecialized(const QualType&, ...)<br class="gmail_msg">
>>>>>>>>>> *latches* onto desugared types in *arbitrary* order. I agree that this is a<br class="gmail_msg">
>>>>>>>>>> problem (most obvious for me for the typeAliasDecl and<br class="gmail_msg">
>>>>>>>>>> classTemplateSpecializationDecl case). I agree that the CL adds a new case<br class="gmail_msg">
>>>>>>>>>> to the set of ordered desugaring attempts done by this method (and therefore<br class="gmail_msg">
>>>>>>>>>> can be seen as making the old problem slightly worse).<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> I think this problem can be fixed by getting rid of the *order*<br class="gmail_msg">
>>>>>>>>>> inside HasDeclarationMatcher::matchesSpecialized(const QualType&, ...) -<br class="gmail_msg">
>>>>>>>>>> instead of:<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> if (auto *TD = Node->getAsTagDecl())<br class="gmail_msg">
>>>>>>>>>> return matchesDecl(TD, Finder, Builder);<br class="gmail_msg">
>>>>>>>>>> else if (auto *TT = Node->getAs<TypedefType>())<br class="gmail_msg">
>>>>>>>>>> return matchesDecl(TT->getDecl(), Finder, Builder);<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> /* the CL under review would add ElaboratedType and<br class="gmail_msg">
>>>>>>>>>> TemplateSpecializationType somewhere here */<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> ...<br class="gmail_msg">
>>>>>>>>>> else if (auto *ICNT = Node->getAs<InjectedClassNameType>())<br class="gmail_msg">
>>>>>>>>>> return matchesDecl(ICNT->getDecl(), Finder, Builder);<br class="gmail_msg">
>>>>>>>>>> return false;<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> have something like this:<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> if (auto *TD = Node->getAsTagDecl()) {<br class="gmail_msg">
>>>>>>>>>> if (matchesDecl(TD, Finder, Builder))<br class="gmail_msg">
>>>>>>>>>> return true;<br class="gmail_msg">
>>>>>>>>>> }<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> if (auto *TT = Node->getAs<TypedefType>()) {<br class="gmail_msg">
>>>>>>>>>> if (matchesDecl(TT->getDecl(), Finder, Builder))<br class="gmail_msg">
>>>>>>>>>> return true;<br class="gmail_msg">
>>>>>>>>>> }<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> if (auto *ET = Node->getAs<ElaboratedType>()) {<br class="gmail_msg">
>>>>>>>>>> if (matchesSpecialized(ET->getNamedType(), Finder, Builder))<br class="gmail_msg">
>>>>>>>>>> return true;<br class="gmail_msg">
>>>>>>>>>> }<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> if (auto *TST = Node->getAs<TemplateSpecializationType>()) {<br class="gmail_msg">
>>>>>>>>>> if (matchesSpecialized(*TST, Finder, Builder))<br class="gmail_msg">
>>>>>>>>>> return true;<br class="gmail_msg">
>>>>>>>>>> }<br class="gmail_msg">
>>>>>>>>>> ...<br class="gmail_msg">
>>>>>>>>>> if (auto *ICNT = Node->getAs<InjectedClassNameType>()) {<br class="gmail_msg">
>>>>>>>>>> if (matchesDecl(ICNT->getDecl(), Finder, Builder))<br class="gmail_msg">
>>>>>>>>>> return true;<br class="gmail_msg">
>>>>>>>>>> }<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> return false;<br class="gmail_msg">
>>>>>>>>><br class="gmail_msg">
>>>>>>>>><br class="gmail_msg">
>>>>>>>>> I think this is an interesting idea. It would certainly address my<br class="gmail_msg">
>>>>>>>>> concerns. (You'd actually want something slightly different from the above,<br class="gmail_msg">
>>>>>>>>> I think: given<br class="gmail_msg">
>>>>>>>>><br class="gmail_msg">
>>>>>>>>> typedef int A;<br class="gmail_msg">
>>>>>>>>> typedef A B;<br class="gmail_msg">
>>>>>>>>> B x;<br class="gmail_msg">
>>>>>>>>><br class="gmail_msg">
>>>>>>>>> ...<br class="gmail_msg">
>>>>>>>>> varDecl(hasType(qualType(hasDeclaration(typedefNameDecl().bind("TD")))))<br class="gmail_msg">
>>>>>>>>> should match x /twice/: once with TD pointing at A and once with it pointing<br class="gmail_msg">
>>>>>>>>> at B. So you probably want to repeatedly single-step desugar the type and<br class="gmail_msg">
>>>>>>>>> try extracting a declaration from each level of type sugar.)<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> Oh, good point - I haven't thought about this scenario. I think<br class="gmail_msg">
>>>>>>>> this direction (based on getAsSuger from Type.cpp) look promising (passes a<br class="gmail_msg">
>>>>>>>> test with 2 chained typedefs):<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> template <typename T, typename DeclMatcherT><br class="gmail_msg">
>>>>>>>> class HasDeclarationMatcher : public WrapperMatcherInterface<T> {<br class="gmail_msg">
>>>>>>>> ...<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> /// \brief Extracts the TagDecl of a QualType and returns whether<br class="gmail_msg">
>>>>>>>> the inner<br class="gmail_msg">
>>>>>>>> /// matcher matches on it.<br class="gmail_msg">
>>>>>>>> bool matchesSpecialized(const QualType &Node, ASTMatchFinder<br class="gmail_msg">
>>>>>>>> *Finder,<br class="gmail_msg">
>>>>>>>> BoundNodesTreeBuilder *Builder) const {<br class="gmail_msg">
>>>>>>>> if (Node.isNull())<br class="gmail_msg">
>>>>>>>> return false;<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> // Do not use getAs<TemplateTypeParmType> instead of the direct<br class="gmail_msg">
>>>>>>>> dyn_cast.<br class="gmail_msg">
>>>>>>>> // Calling getAs will return the canonical type, but that type<br class="gmail_msg">
>>>>>>>> does not<br class="gmail_msg">
>>>>>>>> // store a TemplateTypeParmDecl. We *need* the uncanonical type,<br class="gmail_msg">
>>>>>>>> if it is<br class="gmail_msg">
>>>>>>>> // available, and using dyn_cast ensures that.<br class="gmail_msg">
>>>>>>>> if (auto *TTP =<br class="gmail_msg">
>>>>>>>> dyn_cast<TemplateTypeParmType>(Node.getTypePtr())) {<br class="gmail_msg">
>>>>>>>> if (matchesDecl(TTP->getDecl(), Finder, Builder))<br class="gmail_msg">
>>>>>>>> return true;<br class="gmail_msg">
>>>>>>>> }<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> const Type* Cur = Node.getTypePtr();<br class="gmail_msg">
>>>>>>>> while (Cur) {<br class="gmail_msg">
>>>>>>>> switch (Cur->getTypeClass()) {<br class="gmail_msg">
>>>>>>>> #define ABSTRACT_TYPE(Class, Parent)<br class="gmail_msg">
>>>>>>>> #define TYPE(Class, Parent) \<br class="gmail_msg">
>>>>>>>> case Type::Class: { \<br class="gmail_msg">
>>>>>>>> const Class##Type *Ty = cast<Class##Type>(Cur); \<br class="gmail_msg">
>>>>>>>> if (matchesSpecialized(*Ty, Finder, Builder)) \<br class="gmail_msg">
>>>>>>>> return true; \<br class="gmail_msg">
>>>>>>>> if (!Ty->isSugared()) return false; \<br class="gmail_msg">
>>>>>>>> Cur = Ty->desugar().getTypePtr(); \<br class="gmail_msg">
>>>>>>>> break; \<br class="gmail_msg">
>>>>>>>> }<br class="gmail_msg">
>>>>>>>> #include "clang/AST/TypeNodes.def"<br class="gmail_msg">
>>>>>>>> }<br class="gmail_msg">
>>>>>>>> }<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> return false;<br class="gmail_msg">
>>>>>>>> }<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>> BTW: I have to note that the code above is potentially O(N^2) where N<br class="gmail_msg">
>>>>>>> is the length of the desugaring chain (because each desugaring step can<br class="gmail_msg">
>>>>>>> potentially call back into the QualType overload). This is obviously<br class="gmail_msg">
>>>>>>> suboptimal. We probably need to discuss this when we review / talk about<br class="gmail_msg">
>>>>>>> landing something like this.<br class="gmail_msg">
>>>>>><br class="gmail_msg">
>>>>>><br class="gmail_msg">
>>>>>> It's basically the same difference between has() and hasDescendant.<br class="gmail_msg">
>>>>>> I think<br class="gmail_msg">
>>>>>> a) if we go that route, we should make it explicit by having a matcher<br class="gmail_msg">
>>>>>> that has it in its name<br class="gmail_msg">
>>>>>> b) we need to be able to memoize at each step, so this needs to become<br class="gmail_msg">
>>>>>> part of the visitor interface of the matchers; given the sheer number of<br class="gmail_msg">
>>>>>> types this seems like it'll get rather complex<br class="gmail_msg">
>>>>>><br class="gmail_msg">
>>>>>> I'll make sure to discuss this with Richard etc al in person next week<br class="gmail_msg">
>>>>>> when we're at LLVM conf.<br class="gmail_msg">
>>>>>><br class="gmail_msg">
>>>>>>><br class="gmail_msg">
>>>>>>><br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> but it opens a whole bunch of questions - what to do with other<br class="gmail_msg">
>>>>>>>> types - for example should hasDeclaration "jump over / drill holes in paint<br class="gmail_msg">
>>>>>>>> cans" for:<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> AutoType, TypeOfExprType, DecltypeType, TypeOfType<br class="gmail_msg">
>>>>>>>> LValueReferenceType, ValueReferenceType<br class="gmail_msg">
>>>>>>>> PointerType, ReferenceType<br class="gmail_msg">
>>>>>>>> And all the other types where I had to add a matchSpecialized<br class="gmail_msg">
>>>>>>>> overload for so that things compile (some of these obviously don't have a<br class="gmail_msg">
>>>>>>>> decl [and therefore shouldn't match hasDeclaration(...)] - like BuiltinType,<br class="gmail_msg">
>>>>>>>> but I am listing all of them for completeness): AtomicType, PipeType,<br class="gmail_msg">
>>>>>>>> ObjCObjectPointerType, ObjCObjectType, PackExpansionType,<br class="gmail_msg">
>>>>>>>> DependentTemplateSpecializationType, DependentNameType, AttributedType,<br class="gmail_msg">
>>>>>>>> UnaryTransformType, SubstTemplateTypeParmPackType,<br class="gmail_msg">
>>>>>>>> SubstTemplateTypeParmType, AdjustedType, DecayedType, ParenType,<br class="gmail_msg">
>>>>>>>> FunctionNoProtoType, FunctionProtoType, ExtVectorType, VectorType,<br class="gmail_msg">
>>>>>>>> DependentSizedExtVectorType, DependentSizedArrayType, VariableArrayType,<br class="gmail_msg">
>>>>>>>> IncompleteArrayType, ConstantArrayType, MemberPointerType, BlockPointerType,<br class="gmail_msg">
>>>>>>>> ComplexType, BuiltinType,<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>> Also - I am not quite sure if I understand if special-casing of<br class="gmail_msg">
>>>>>>>> TemplateTypeParmType is needed. It probably is (because here we use<br class="gmail_msg">
>>>>>>>> dyn_cast rather than cast). At any rate drilling into details here can<br class="gmail_msg">
>>>>>>>> probably wait until a real review in Phabricator...<br class="gmail_msg">
>>>>>>>><br class="gmail_msg">
>>>>>>>>><br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> The code above passes all tests from the clang-test ninja target +<br class="gmail_msg">
>>>>>>>>>> results in the following behavior for the 2 cases pointed out in an email:<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {<br class="gmail_msg">
>>>>>>>>>> std::string input =<br class="gmail_msg">
>>>>>>>>>> "template<typename T>\n"<br class="gmail_msg">
>>>>>>>>>> "class Template {\n"<br class="gmail_msg">
>>>>>>>>>> " public:\n"<br class="gmail_msg">
>>>>>>>>>> " void Method() {}\n"<br class="gmail_msg">
>>>>>>>>>> "};\n"<br class="gmail_msg">
>>>>>>>>>> "template <typename U>\n"<br class="gmail_msg">
>>>>>>>>>> "void Function(Template<U> param) {\n"<br class="gmail_msg">
>>>>>>>>>> " param.Method();\n"<br class="gmail_msg">
>>>>>>>>>> "};\n";<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> // param type has-declaration matches class template decl?<br class="gmail_msg">
>>>>>>>>>> EXPECT_TRUE(matches(input, parmVarDecl(<br class="gmail_msg">
>>>>>>>>>> hasName("param"),<br class="gmail_msg">
>>>>>>>>>> hasType(qualType(<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> // param type has-declaration matches class decl?<br class="gmail_msg">
>>>>>>>>>> EXPECT_FALSE(matches(input, parmVarDecl(<br class="gmail_msg">
>>>>>>>>>> hasName("param"),<br class="gmail_msg">
>>>>>>>>>> hasType(qualType(<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));<br class="gmail_msg">
>>>>>>>>>> }<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> TEST(HasDeclaration, Case2FromEmail) {<br class="gmail_msg">
>>>>>>>>>> std::string input =<br class="gmail_msg">
>>>>>>>>>> "template<typename T>\n"<br class="gmail_msg">
>>>>>>>>>> "class Template {\n"<br class="gmail_msg">
>>>>>>>>>> " public:\n"<br class="gmail_msg">
>>>>>>>>>> " void Method() {}\n"<br class="gmail_msg">
>>>>>>>>>> "};\n"<br class="gmail_msg">
>>>>>>>>>> "void Function(Template<int> param) {\n"<br class="gmail_msg">
>>>>>>>>>> " param.Method();\n"<br class="gmail_msg">
>>>>>>>>>> "};\n";<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> // param type has-declaration matches class template decl?<br class="gmail_msg">
>>>>>>>>>> EXPECT_TRUE(matches(input, parmVarDecl(<br class="gmail_msg">
>>>>>>>>>> hasName("param"),<br class="gmail_msg">
>>>>>>>>>> hasType(qualType(<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> hasDeclaration(decl(classTemplateDecl(hasName("Template")))))))));<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> // param type has-declaration matches class decl?<br class="gmail_msg">
>>>>>>>>>> EXPECT_TRUE(matches(input, parmVarDecl(<br class="gmail_msg">
>>>>>>>>>> hasName("param"),<br class="gmail_msg">
>>>>>>>>>> hasType(qualType(<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> hasDeclaration(decl(cxxRecordDecl(hasName("Template")))))))));<br class="gmail_msg">
>>>>>>>>>> }<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> If the test expectations above look reasonable, then we can just<br class="gmail_msg">
>>>>>>>>>> discuss how to land the code changes above (e.g. as a single CL or 2 CLs - I<br class="gmail_msg">
>>>>>>>>>> would prefer 2 separate CLs). Otherwise, let's talk about the test<br class="gmail_msg">
>>>>>>>>>> expectations - what should they look like / how should they be different<br class="gmail_msg">
>>>>>>>>>> from the test snippet above?<br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>> On Tue, Oct 25, 2016 at 1:42 PM, Richard Smith<br class="gmail_msg">
>>>>>>>>>> <<a href="mailto:richardsmith@google.com" class="gmail_msg" target="_blank">richardsmith@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>> On 25 October 2016 at 01:57, Manuel Klimek <<a href="mailto:klimek@google.com" class="gmail_msg" target="_blank">klimek@google.com</a>><br class="gmail_msg">
>>>>>>>>>>> wrote:<br class="gmail_msg">
>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>> On Tue, Oct 25, 2016 at 3:25 AM Richard Smith<br class="gmail_msg">
>>>>>>>>>>>> <<a href="mailto:richardsmith@google.com" class="gmail_msg" target="_blank">richardsmith@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>> On 24 October 2016 at 16:41, Łukasz Anforowicz<br class="gmail_msg">
>>>>>>>>>>>>> <<a href="mailto:lukasza@google.com" class="gmail_msg" target="_blank">lukasza@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>> I am not sure if we are making progress, but so far, we have<br class="gmail_msg">
>>>>>>>>>>>>>> seen 1) test cases that are broken and fixed by the CL under review and 2)<br class="gmail_msg">
>>>>>>>>>>>>>> test cases that are not changed by the CL under review and 3) no test cases<br class="gmail_msg">
>>>>>>>>>>>>>> that are made worse by the CL under review. I've tried to gather the<br class="gmail_msg">
>>>>>>>>>>>>>> scenarios and the associated test cases in<br class="gmail_msg">
>>>>>>>>>>>>>> <a href="https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit" rel="noreferrer" class="gmail_msg" target="_blank">https://docs.google.com/document/d/1y1F6uKdulluYBFch680g6bmHe_zDe8mHFYjfXRoakNI/edit</a>.<br class="gmail_msg">
>>>>>>>>>>>>>> Could you please take a look and shout if I misrepresented anything (i.e.<br class="gmail_msg">
>>>>>>>>>>>>>> whether we agree that a specific test scenario shows a bug).<br class="gmail_msg">
>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>> I've also added some comments below / inline.<br class="gmail_msg">
>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 3:54 PM, Richard Smith<br class="gmail_msg">
>>>>>>>>>>>>>> <<a href="mailto:richardsmith@google.com" class="gmail_msg" target="_blank">richardsmith@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>> On 24 October 2016 at 15:25, Łukasz Anforowicz<br class="gmail_msg">
>>>>>>>>>>>>>>> <<a href="mailto:lukasza@google.com" class="gmail_msg" target="_blank">lukasza@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>> On Mon, Oct 24, 2016 at 2:21 PM, Richard Smith<br class="gmail_msg">
>>>>>>>>>>>>>>>> <<a href="mailto:richardsmith@google.com" class="gmail_msg" target="_blank">richardsmith@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>> On 24 October 2016 at 11:23, Łukasz Anforowicz<br class="gmail_msg">
>>>>>>>>>>>>>>>>> <<a href="mailto:lukasza@google.com" class="gmail_msg" target="_blank">lukasza@google.com</a>> wrote:<br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>>> I've run a few more experiments and I see that before any<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> changes:<br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>>> (1) Presence of ElaboratedType means that hasDeclaration<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> doesn’t match *anything*. This is clearly wrong. I still want to fix that.<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> (2) Both examples from Richard’s email consistently match<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> the class template decl (and not the class decl). If Richard’s expectations<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> are not met *prior* to my CL, then I don’t think these expectations should<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> be fixed by my CL.<br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>>> OTOH, maybe it is worth to open a bug to investigate this<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> more - personally I would argue that hasDeclaration should match *any* decl<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> - for example any of type alias / template class decl / class decl.<br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>>> (3) If desugaring can result in both<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> TemplateSpecializationType and TypedefType, then only one is arbitrarily<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> picked - see test case TypedefTypeAndTemplateSpecializationTypeAreBothHere.<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> Again - this problem exists *prior* to my CL.<br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>>> So - my plan right now is to just simplify/revert my<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> patches so they only fix (1). I don't think discussion of (2) should impact<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> the CL, because the behavior that disagrees with Richard's expectations is<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> happening *before* my changes. I will also not attempt to fix (3), because<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> this starts to interfere with (2) - sorry :-/.<br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>>> QUESTION: Does the plan from the previous paragraph sound<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> good to you?<br class="gmail_msg">
>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>> Yes; I agree that failing to look through ElaboratedType<br class="gmail_msg">
>>>>>>>>>>>>>>>>> here is just a bug.<br class="gmail_msg">
>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>> Ack. I've marked "elaborated type" section in the doc as<br class="gmail_msg">
>>>>>>>>>>>>>> something that is improved by the CL under review.<br class="gmail_msg">
>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>><br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
</div></div><span class="m_-3133883551018558048gmail- gmail_msg">>>>>>>>>>>>>>>>>>> Experiments before my changes were done with the following<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> unit tests:<br class="gmail_msg">
>>>>>>>>>>>>>>>>>><br class="gmail_msg">
</span>>>>>>>>>>>>>>>>>>> // This testcase shows that Richard’s expectations are not<br class="gmail_msg">
<span class="m_-3133883551018558048gmail-im m_-3133883551018558048gmail-HOEnZb gmail_msg">>>>>>>>>>>>>>>>>>> met *prior* to my CL.<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> // (i.e. the testcase below passes before my CL; the<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> expectations in the testcase<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> // are the opposite of Richard’s expectations).<br class="gmail_msg">
</span><div class="m_-3133883551018558048gmail-HOEnZb gmail_msg"><div class="m_-3133883551018558048gmail-h5 gmail_msg">>>>>>>>>>>>>>>>>>> TEST(HasDeclaration, Case1FromEmail) {<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> std::string input =<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> "template<typename T>\n"<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> "class Template {\n"<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> " public:\n"<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> " void Method() {}\n"<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> "};\n"<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> "template <typename U>\n"<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> "void Function(Template<U> param) {\n"<br class="gmail_msg">
>>>>>>>>>>>>>>>>>> " param.Method();\n"<br class="gmail_msg">
</div></div></blockquote></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><br class="gmail_msg"><br clear="all" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div>-- <br class="gmail_msg"><div class="m_-3133883551018558048gmail_signature gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg">Thanks,</div><div class="gmail_msg"><br class="gmail_msg"></div>Lukasz</div></div>
</div></div></blockquote></div>