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

[Lingda Li via Phabricator via cfe-commits]

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

Sure I'll add a codegen test with mapper. Thanks!



================
Comment at: include/clang/AST/OpenMPClause.h:4193
                         ArrayRef<SourceLocation> MapModifiersLoc,
+                        NestedNameSpecifierLoc MapperQualifierLoc,
+                        DeclarationNameInfo MapperIdInfo,
----------------
ABataev wrote:
> 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?
None of these locations are for mapper, so I think it's not good to combine them with mapper.
Those numbers (`NumVars`, `NumUniqueDeclarations`, `NumComponentLists`, `NumComponents`) are not for mapper either. `NumVars` can be viewed as partially for mapper, but its main purpose is for the number of list items in map clause. So logically, I think they should not be combined with mapper.
What do you think?


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

https://reviews.llvm.org/D58074