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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
