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