[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71241#1788003 <https://reviews.llvm.org/D71241#1788003>, @jdoerfert wrote:

> In D71241#1787652 <https://reviews.llvm.org/D71241#1787652>, @hfinkel wrote:
>
> > In D71241#1787571 <https://reviews.llvm.org/D71241#1787571>, @ABataev wrote:
> >
> > > In D71241#1787265 <https://reviews.llvm.org/D71241#1787265>, @hfinkel wrote:
> > >
> > > > In D71241#1786959 <https://reviews.llvm.org/D71241#1786959>, @jdoerfert wrote:
> > > >
> > > > > In D71241#1786530 <https://reviews.llvm.org/D71241#1786530>, @ABataev wrote:
> > > > >
> > > > > > Most probably, we can use this solution without adding a new expression. `DeclRefExpr` class can contain 2 decls: FoundDecl and the Decl being used. You can use FoundDecl to point to the original function and used decl to point to the function being called in this context. But at first, we need to be sure that we can handle all corner cases correctly.
> > > > >
> > > > >
> > > > > What new expression are you talking about?
> > > >
> > > >
> > > > To be clear, I believe he's talking about the new expression that I proposed we add in order to represent this kind of call. If that's not needed, and we can use the FoundDecl/Decl pair for that purpose, that should also be considered.
> > > >
> > > > > This solution already does point to both declarations, as shown here: https://reviews.llvm.org/D71241#1782504
> > >
> > >
> > > Exactly. We need to check if the `MemberRefExpr` can do this too to handle member functions correctly.
> > >  And need to wait for opinion from Richard Smith about the design. We need to be sure that it won't break compatibility with C/C++ in some corner cases. Design bugs are very hard to solve and I want to be sure that we don't miss anything. And we provide full compatibility with both C and C++.
> >
> >
> > We do need to be careful here. For cases with FoundDecl != Decl, I think that the typo-correction cases look like they probably work, but there are a few places where we make semantic decisions based on the mismatch, such as:
> >
> > In SemaTemplate.cpp below line 512, we have (this is in C++03-specific code):
> >
> >   } else if (!Found.isSuppressingDiagnostics()) {
> >     //   - if the name found is a class template, it must refer to the same
> >     //     entity as the one found in the class of the object expression,
> >     //     otherwise the program is ill-formed.
> >     if (!Found.isSingleResult() ||
> >         getAsTemplateNameDecl(Found.getFoundDecl())->getCanonicalDecl() !=
> >             OuterTemplate->getCanonicalDecl()) {
> >       Diag(Found.getNameLoc(),
> >            diag::ext_nested_name_member_ref_lookup_ambiguous)
> >   
> >
> > and in SemaExpr.cpp near line 2783, we have:
> >
> >   // If we actually found the member through a using declaration, cast
> >   // down to the using declaration's type.
> >   //
> >   // Pointer equality is fine here because only one declaration of a
> >   // class ever has member declarations.
> >   if (FoundDecl->getDeclContext() != Member->getDeclContext()) {
> >     assert(isa<UsingShadowDecl>(FoundDecl));
> >     QualType URecordType = Context.getTypeDeclType(
> >                            cast<CXXRecordDecl>(FoundDecl->getDeclContext()));
>
>
> Could you specify what behavior you expect, or what the test casees would look like?
>
> For the record:
>  OpenMP basically says, if you have a call to a (base)function that has variants with contexts that match at the call site, call the variant with the highest score. The variants are specified by a //variant-func-id//, which is a base language identifier or C++ //template-id//. For C++, the variant declaration is identified by *performing the base language lookup rules on the //variant-func-id// with arguments that correspond to the base function argument types*.


No need to worry about lookup in C++, ADL lookup is implemented already.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71241/new/

https://reviews.llvm.org/D71241