[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits
Toby Allsopp via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Jul 9 13:50:45 PDT 2017
toby-allsopp added a comment.
In https://reviews.llvm.org/D35046#802838, @EricWF wrote:
> I think the test could be improved. First could you add the test within `test/SemaCXX/coroutines.cpp`? Second could you add some negative tests that check the diagnostics generated when you don't provide a specialization of coroutine traits (ie that the old behaviour of not including the class type now produces diagnostics). Could you also add tests for const/volatile and lvalue/rvalue qualified member functions?
Will do, thanks for the suggestions.
================
Comment at: lib/Sema/SemaCoroutine.cpp:85
+ if (MD->isInstance()) {
+ QualType T = MD->getThisType(S.Context);
+ Args.addArgument(TemplateArgumentLoc(
----------------
EricWF wrote:
> This seems wrong to me.
>
> `getThisType` returns the type of the `this` parameter as specified under [class.this] but according to the coroutines spec the type of the parameter should be the type of the `implicit object parameter`, which is specified under [[http://eel.is/c++draft/over.match.funcs#4 | (over.match.funcs) p4 ]].
Oh wow, that's a howler. I will check my tests against MSVC here. Thanks.
================
Comment at: lib/Sema/SemaCoroutine.cpp:441
+ return false;
+ }();
----------------
EricWF wrote:
> Huh, I've never seen lambdas used like this before but I really like it.
It's a sometimes controversial style called immediately-invoked lambda/function expression (IILE or IIFE). I saw other instances of it in this file (or possibly some other file in clang) so I'm assuming it's acceptable style.
https://reviews.llvm.org/D35046
More information about the cfe-commits
mailing list