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