martong added a comment.
@shafik I have updated the patch with ODR handling strategies as per our
discusson.
For the record I copy our discussion here:
On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour <yaghmour.sha...@gmail.com>
wrote:
> Apologies, I missed this earlier!
>
> On Wed, Aug 21, 2019 at 2:44 AM Gábor Márton <martongab...@gmail.com> wrote:
> >
> > Hi Shafik,
> >
> > > Right now you will end up w/ an arbitrary one of them but we do want to
> support
> > a way to choose between them eventually.
> >
> > Actually, right now (if the patch is not merged) we end up having both of
> them in the AST. Some parts of the AST reference the existing definition,
> while some other parts reference the new definition. Also, the regular lookup
> will find both definitions.
> >
> > If the patch is merged then only the first (the existing) definition is
> kept and an error is reported.
> >
> > > AFAICT this would prevent such a solution. At least that is how the
> > new test case for RecordDecl make it appear
> >
> > Yes, you will never be able to remove an existing node from the AST, so I
> don't think an either-or choosing mechanism is feasible. But you may be able
> to decide whether you want to add the new and conflicting node. And you may
> be able to use a different name for the new conflicting node. This may help
> clients to see which node they are using.
> > I want to create an additional patch which builds on this patch. Here is
> the essence of what I'd like to have:
> > if (!ConflictingDecls.empty()) {
> > Expected<DeclarationName> Resolution = Importer.HandleNameConflict(
> > Name, DC, Decl::IDNS_Member, ConflictingDecls.data(),
> > ConflictingDecls.size());
> > if (Resolution)
> > Name = Resolution.get();
> > else
> > return Resolution.takeError();
> > }
> > Consider the "else" branch. I'd like to have such an "else" branch
> everywhere. The point is to use the result of HandleNameConflict (if it is
> set to something). This way it is possible to create any kind of ODR handling
> by overwriting HandleNameConflict in the descendant classes.
> >
> > We identified 3 possible strategies so far:
> >
> > Conservative. In case of name conflict propagate the error. This should be
> the default behavior.
> > Liberal. In case of name conflict create a new node with the same name and
> do not propagate the error.
> > Rename. In case of name conflict create a new node with a different name
> (e.g. with a prefix of the TU's name). Do not propagate the error.
> >
> >
> > If we add a new conflicting node beside the exiting one, then some clients
> of the AST which rely on lookup will be confused. The CTU client does not
> rely on lookup so that is not really a problem there, but I don't know what
> this could cause with LLDB. Perhaps the renaming strategy could work there
> too.
> > The Conservative and Liberal strategies are very easy to implement, and I
> am going to create patches for that if we have consensus.
>
> We know currently we do have cases where we have ODR violations w/
> RecordDecl due to use of an opaque struct in the API headers and a
> concrete instance internally e.g.:
>
> //opaque
> struct A {
>
> char buf[16];
>
> };
>
> //concrete
> struct A {
>
> double d;
> int64_t x;
>
> };
>
> and we don't want this to become an error.
>
> I think we would at least one the choice of Conservative or Liberal to
> be configurable and maybe LLDB would default to Liberal. This would
> enable us to keep the status quo and not break existing cases any
> worse than they already are.
>
> I would prefer that would be done in this PR since I don't want to be
> in a situation where we branch internally and we have this change but
> not the follow-up change.
>
>> > I don't see how like you comment says this does not effect CXXRecordDecl
> >
> > In my comment I do not say that CXXRecordDecls are exceptions from the
> general way of handling ODR violations.
> > The comment is about that we shall not report ODR errors if we are not
> 100% sure that we face one.
> > So we should report an ODR error only if the found Decl and the newly
> imported Decl have the same kind.
> > I.e. both are CXXRecordDecls.
> > For example, let's assume we import a CXXRecordDecl and we find an
> existing ClassTemplateDecl with the very same Name.
> > Then we should not report an ODR violation.
>
> Thank you for the clarification, I misunderstood the comment, now it
> makes more sense.
>
>> Thanks,
> > Gabor
> >
> >
> > On Mon, Aug 19, 2019 at 5:46 PM Shafik Yaghmour
> <yaghmour.sha...@gmail.com> wrote:
> >>
> >> Have a nice vacation :-)
> >>
> >> On Mon, Aug 19, 2019 at 8:40 AM Gábor Márton <martongab...@gmail.com>
> wrote:
> >> >
> >> > Hi Shafik,
> >> >
> >> > I'll have an answer for you on Wednesday, I'm on vacation until then.
> >> >
> >> > Thanks,
> >> > Gábor
> >> >
> >> > On Sat, 17 Aug 2019, 04:30 Shafik Yaghmour, <yaghmour.sha...@gmail.com>
> wrote:
> >> >>
> >> >> Hello Gábor,
> >> >>
> >> >> I was looking at;
> >> >>
> >> >> https://reviews.llvm.org/D59692
> >> >>
> >> >> again and I appreciate you added the new tests.
> >> >>
> >> >> I don't know how I missed this before but we would probably like to
> >> >> support ODR violations in some reasonable manner. For example we have
> >> >> one API that is using an opaque struct for the public API but the
> >> >> private API used a non-opaque definition.
> >> >>
> >> >> I would have to come up with some sort of test case which is hard to
> >> >> do, or at least I did not have success last time I tried. Right now
> >> >> you will end up w/ an arbitrary one of them but we do want to support
> >> >> a way to choose between them eventually.
> >> >>
> >> >> AFAICT this would prevent such a solution. At least that is how the
> >> >> new test case for RecordDecl make it appear and I don't see how like
> >> >> you comment says this does not effect CXXRecordDecl.
> >> >>
> >> >> -Shafik
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59692/new/
https://reviews.llvm.org/D59692
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
