[PATCH] D67978: [OpenMP 5.0] Fix user-defined mapper lookup in sema for arrays

[Lingda Li via Phabricator via cfe-commits]

lildmh marked an inline comment as done.
lildmh added a comment.

In D67978#1683166 <https://reviews.llvm.org/D67978#1683166>, @ABataev wrote:

> In D67978#1683146 <https://reviews.llvm.org/D67978#1683146>, @lildmh wrote:
>
> > HI Alexey, the ast print test is already there. Because I didn't check the mapper for array type before, such code will always not report any error, and ast print test is correct. Codegen test belongs to the other patch I released. It fits that patch much better.
>
>
> How is this possible? If we did not have support for the array type, we could not have correct handling of such types in successful tests.


The ast print for array with mapper was correct because the mapper id is still with the array type. Without this patch, the problem is it will not look up the mapper declaration associated with the id, and as a result, the codegen is not correct. I found this problem when I tested the codegen.



================
Comment at: lib/Sema/SemaOpenMP.cpp:14751
+    assert(Type->getAsArrayTypeUnsafe() && "Expect to get a valid array type");
+    Type = Type->getAsArrayTypeUnsafe()->getElementType().getCanonicalType();
+  }
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > Why do you want canonical type here? I think it is wrong. It drops all language sugar like typedefs etc. But typedefs are not supported in mappers, right?
> > I didn't see that the spec says typedef is not supported in mappers, so I suppose it should be supported. So I think `getCanonicalType` is necessary here?
> Do we check the canonicaL type by default, for non-array type?
Yes, we use canonical type for non-array type, for instance, line 14935 in this file.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67978