gribozavr accepted this revision.
gribozavr added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/Sema/SemaInit.cpp:6581
+ if (!Callee->getIdentifier()) {
+ auto OO = Callee->getOverloadedOperator();
+ return OO == OverloadedOperatorKind::OO_Subscript ||
----------------
xazax.hun wrote:
> xazax.hun wrote:
> > gribozavr wrote:
> > > xazax.hun wrote:
> > > > gribozavr wrote:
> > > > > mgehre wrote:
> > > > > > xazax.hun wrote:
> > > > > > > If we want to relax the warnings to give more results we could
> > > > > > > extend the checking of these overloaded operators for annotated
> > > > > > > types. But this would imply that the user need to have the
> > > > > > > expected semantics for those types and can only suppress false
> > > > > > > positives by removing some gsl:::owner/poinnter annotations.
> > > > > > I see those options:
> > > > > > - Either gsl::Owner implies some specific form of those operators
> > > > > > (and if that does not hold for a class, then one should not
> > > > > > annotate it with gsl::Owner)
> > > > > > - or gsl::Owner only implies some specific behavior for the
> > > > > > "gsl::Pointer constructed from gsl::Owner" case and everything else
> > > > > > requires additional annotation
> > > > > > I expect that we will experiment a bit in the future to see what
> > > > > > works well for real-world code.
> > > > > I understand the difficulty, but I don't think it is appropriate to
> > > > > experiment by ourselves -- these attributes are defined in a spec,
> > > > > and if something is not clear, the spec should be clarified.
> > > > This is exactly what is going to happen but I think it would be
> > > > unfortunate to stall the progress until the new version of the spec
> > > > materializes. The idea is to keep the implementations and the specs in
> > > > sync, but as Herb has other projects too, it takes some time to channel
> > > > the experience back into the spec. As the current version of the
> > > > warnings found true positives in real world projects and we have yet to
> > > > see any false positives I would prefer to move forward to maximize
> > > > utility.
> > > >
> > > >
> > > I don't understand how different implementations can ever converge in
> > > that case.
> > >
> > > If this language extension is not sufficiently designed yet, maybe it is
> > > not ready for inclusion in Clang?
> > >
> > >
> > The MSVC implementation does not support user defined annotations yet, so
> > we are the first one to ask such questions like is it valid for an user to
> > annotate a type as gsl::Pointer and have an overloaded deref operator with
> > functionality other than accessing the pointee. We already forwarded these
> > concerns to Herb, and he promised to clarify these things in the paper.
> > Once it is clarified, MSVC will also follow it. Since this code will not
> > reach the wider audience until Clang 10 is released and it is pretty easy
> > to change this detail I do not see the justification to postpone the
> > inclusion.
> >
> > If we postpone the inclusion over and over we will never get enough
> > experience from real world users to ever have enough confidence.
> Ok, after discussing this with Herb the conclusion is the following. The
> paper does not have any requirements for any of the methods for Pointers or
> Owners. If a method has a semantics that is not a good match with the default
> rules the user can annotate the methods. As method annotations are coming
> later, the right approach is to only rely on the semantics of these operators
> for STL types for now, exactly what is implemented in this patch.
>
> And again, just to make it clear, the reason why we want to add this first
> rather than the annotations because the latter is a rather large patch and we
> want to gain more real world experience first before committing to a specific
> approach. Yet, all of the true positives we found so far needs these
> assumptions about STL types, so it really is useful to have this until we add
> the support for annotations.
Okay, since this code is introducing new behavior only for `std` and builtin
types, I think we can do it even though it is not in the spec.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65127/new/
https://reviews.llvm.org/D65127
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
