rjmccall added inline comments.
================
Comment at: clang/include/clang/AST/DeclCXX.h:1013
CXXMethodDecl *getLambdaStaticInvoker() const;
+ CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const;
+
----------------
Probably worth clarifying in the comment which invoker is returned by the
no-arguments variant.
================
Comment at: clang/lib/AST/DeclCXX.cpp:1550
+ return FTy->getCallConv() == CC;
+ });
+
----------------
This seems both rather more complicated and rather more expensive than a simple
loop which assigns to a local pointer.
================
Comment at: clang/lib/AST/DeclCXX.cpp:1568
+ return llvm::find_if(Invokers, MDEqual) != Invokers.end();
+ }
+
----------------
Can you clarify how this method is semantically different from:
```
assert(MD->getParent() == this);
return isLambda() && MD->getName() == getLambdaStaticInvokerName();
```
other than probably inadvertently working with a method from a completely
different class?
================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:2268
+ } else if (const auto *AT = dyn_cast_or_null<AutoType>(
+ ResultType->getContainedAutoType())) {
Out << '?';
----------------
So, this is changing the mangling of lambda conversion operators. Is this what
MSVC does?
================
Comment at: clang/lib/Sema/SemaLambda.cpp:1467
+ llvm::SmallVector<CallingConv, 2> Conventions =
+ getLambdaConversionFunctionCallConv(S, CallOpProto);
+
----------------
Can we make this call a callback or something rather than returning an array
that almost always has one element?
================
Comment at: clang/lib/Sema/SemaLambda.cpp:1278
+ if (CallOpCC == DefaultMember)
+ return DefaultFree;
+ return CallOpCC;
----------------
erichkeane wrote:
> rsmith wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > rjmccall wrote:
> > > > > ...I made this comment in my first review, but Phabricator threw it
> > > > > away.
> > > > >
> > > > > The attributes let you explicitly request the default method CC,
> > > > > right? I think you need to check for an explicit attribute rather
> > > > > than just checking CC identity. There should be an AttributedType in
> > > > > the sugar.
> > > > They do, but I can't seem to find a way to find them. The calling
> > > > convention is already merged into the functiontype by the time we get
> > > > here, the AttributedType isn't accessible.
> > > >
> > > > So it seems we don't distinguish between "modified by attribute",
> > > > "modified-default by command line", and "modified-default by
> > > > TargetInfo."
> > > >
> > > > That said, I somewhat think this is the right thing to do anyway. If
> > > > you're on a platform where the default call convention is different
> > > > between a free-function and member-function, I'd think that this is
> > > > what you MEAN...
> > > The `AttributedType` should be present in the type on the
> > > `TypeSourceInfo` for the call operator. It might not be present on the
> > > type retrieved by `getType()`, though.
> > >
> > > Concretely, what targets have different calling conventions for member
> > > versus non-member functions, and what do those calling conventions do
> > > differently? (For example, if the default member calling convention
> > > treats `this` differently, it doesn't seem reasonable to apply that to
> > > the static invoker...)
> > Answering my own question: the only case where the default member calling
> > convention is different from the default non-member calling convention is
> > for MinGW on 32-bit x86, where members use `thiscall` by default (which
> > passes the first parameter in `%ecx`).
> >
> > Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static
> > invoker using the `thiscall` calling convention? Intuitively I'd say no,
> > but there's not really anything specific to member functions in `thiscall`
> > -- it just means that we pass the first argument in a register. (However,
> > the first argument of the lambda and the first argument of its static
> > invoker are different, so it's still somewhat unclear whether inheriting
> > `thiscall` is appropriate. But usually for any given lambda only one of the
> > two is actually used.)
> >
> > But there's another quirk here: the default non-member calling convention
> > can be set on the command line with `-fdefault-calling-conv=` (and a few
> > other flags such as `-mrtd`). This doesn't affect member functions by
> > default. So what should happen if this is compiled with
> > `-fdefault-calling-conv=stdcall` (assuming the default calling convention
> > is otherwise `cdecl`):
> >
> > ```
> > auto *p0 = [] [[clang::stdcal]] {};
> > auto *p1 = [] {};
> > auto *p2 = [] [[clang::cdecl]] {};
> > ```
> >
> > `p0` seems easy: the default non-member calling convention and the explicit
> > calling convention are the same. The invoker should be `stdcall`.
> >
> > For `p1`, the default member calling convention is `cdecl` but the default
> > non-member calling convention is `stdcall`. In this case, conformance
> > requires us to use `stdcall` for the pointer type, because `p1` is required
> > to have type `void (*)()`, which is a `stdcall` function pointer in this
> > configuration.
> >
> > For `p2`, the call operator's calling convention has been explicitly set to
> > the default member calling convention. I think in this case I'd expect a
> > `cdecl` static invoker.
> >
> > So I think I'm inclined to say we should always apply an explicit calling
> > convention to both the call operator and the static invoker -- that seems
> > like the simplest and easiest to explain rule, and probably matches user
> > expectations most of the time, especially given the observation that you're
> > usually writing a lambda only for one or the other of the call operator and
> > the static invoker, so if you explicitly write a calling convention
> > attribute, you presumably want it to apply to the part of the lambda's
> > interface that you're using.
> > Answering my own question: the only case where the default member calling
> > convention is different from the default non-member calling convention is
> > for MinGW on 32-bit x86, where members use `thiscall` by default (which
> > passes the first parameter in `%ecx`).
> >
> > Is it reasonable for `[] [[clang::thiscall]] {}` to result in a static
> > invoker using the `thiscall` calling convention? Intuitively I'd say no,
> > but there's not really anything specific to member functions in `thiscall`
> > -- it just means that we pass the first argument in a register. (However,
> > the first argument of the lambda and the first argument of its static
> > invoker are different, so it's still somewhat unclear whether inheriting
> > `thiscall` is appropriate. But usually for any given lambda only one of the
> > two is actually used.)
> >
> > But there's another quirk here: the default non-member calling convention
> > can be set on the command line with `-fdefault-calling-conv=` (and a few
> > other flags such as `-mrtd`). This doesn't affect member functions by
> > default. So what should happen if this is compiled with
> > `-fdefault-calling-conv=stdcall` (assuming the default calling convention
> > is otherwise `cdecl`):
> >
> > ```
> > auto *p0 = [] [[clang::stdcal]] {};
> > auto *p1 = [] {};
> > auto *p2 = [] [[clang::cdecl]] {};
> > ```
> >
> > `p0` seems easy: the default non-member calling convention and the explicit
> > calling convention are the same. The invoker should be `stdcall`.
> >
> > For `p1`, the default member calling convention is `cdecl` but the default
> > non-member calling convention is `stdcall`. In this case, conformance
> > requires us to use `stdcall` for the pointer type, because `p1` is required
> > to have type `void (*)()`, which is a `stdcall` function pointer in this
> > configuration.
> >
> > For `p2`, the call operator's calling convention has been explicitly set to
> > the default member calling convention. I think in this case I'd expect a
> > `cdecl` static invoker.
> >
> > So I think I'm inclined to say we should always apply an explicit calling
> > convention to both the call operator and the static invoker -- that seems
> > like the simplest and easiest to explain rule, and probably matches user
> > expectations most of the time, especially given the observation that you're
> > usually writing a lambda only for one or the other of the call operator and
> > the static invoker, so if you explicitly write a calling convention
> > attribute, you presumably want it to apply to the part of the lambda's
> > interface that you're using.
>
> Do we have a good way to determining which of the 3 mechanisms the user used
> to set the calling convention? (Attribute vs -fdefault-calling-conv= vs just
> using the defualt?)
>
> It would be easy enough to ALWAYS have the static-invoker match the
> calling-convention of the operator(), as long as having it as a 'this' call
> isn't a problem, which given what you said above, I don't think thats a
> problem. The implementation is trivial, since it is just removing the
> condition above.
>
> I guess I'm just asking which you'd like, and if it is the "only have invoker
> match if user requested explicitly", if you had any hints on how to figure
> that out.
>
> Thanks!
I would guess that we probably have an AttributedType if and only if the user
wrote an attribute explicitly, but I don't know for sure if we make one in the
case where it's the only signature information in the lambda. I also don't
know if it survives the type-rewrite that happens after return-type inference.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89559/new/
https://reviews.llvm.org/D89559
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
