xazax.hun marked an inline comment as done.
xazax.hun added inline comments.
================
Comment at: clang/lib/Sema/SemaInit.cpp:6581
+ if (!Callee->getIdentifier()) {
+ auto OO = Callee->getOverloadedOperator();
+ return OO == OverloadedOperatorKind::OO_Subscript ||
----------------
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.
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
