[PATCH] D58074: [OpenMP 5.0] Parsing/sema support for map clause with mapper modifier

Thu Feb 14 09:44:49 PST 2019

ABataev added a comment.

In D58074#1398187 <https://reviews.llvm.org/D58074#1398187>, @lildmh wrote:

> Hi Alexey,
>
> Again thanks for your review! The codegen completely ignores any mapper related info for now, so it should not crash map clause codegen. It also passed the regression test, so map clause codegen should be fine.

Hi Lingda, no problems!
It would be good to have at least one codegen test with the mapper construct to prove that it does not crash the codegen. We don't have any codegen test for this new construct.

================
Comment at: include/clang/AST/OpenMPClause.h:4193
                         ArrayRef<SourceLocation> MapModifiersLoc,
+                        NestedNameSpecifierLoc MapperQualifierLoc,
+                        DeclarationNameInfo MapperIdInfo,
----------------
lildmh wrote:
> ABataev wrote:
> > Also would be good to pack the parameters into the structure.
> Among these parameters:
> 1. 2 of them are for modifiers. Since mapper can also be used in `to` and `from` clauses, it is not good to combine mapper info with them.
> 2. 2 of them are mapper info. I can combine `MapperQualifierLoc` and `MapperIdInfo` into a structure. It's not a big saving though (only reduce one parameter). Do you want me to do that?
> 3. 4 of them are number of vars... It seems not a good idea to combine them into one structure.
> 5. The rest are locations. They are kinda all over the place. It doesn't seem a good idea to combine them as well.
1. The same structure must be used for `to` and `from` clauses, not a problem.
2-4. you can combine it with the number of vars and locations into one structure, no?

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

https://reviews.llvm.org/D58074