jdenny marked an inline comment as done. jdenny added inline comments.
================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068 + unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok)); + if (Type < OMPC_MAP_MODIFIER_unknown) + return OMPC_MAP_MODIFIER_unknown; + return static_cast<OpenMPMapModifierKind>(Type); ---------------- ABataev wrote: > jdenny wrote: > > ABataev wrote: > > > jdenny wrote: > > > > ABataev wrote: > > > > > Why do we need this? > > > > When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return > > > > either an `OpenMPMapModifierKind` or `OpenMPMapClauseKind` depending on > > > > `Tok`. Thus, without this change, both `isMapModifier` and `isMapType` > > > > can return either of those despite the difference in their names and > > > > documentation. > > > > > > > > I found that, when `Parser::parseMapTypeModifiers` ignores `present` as > > > > if it's not a modifier because OpenMP < 5.1, then `isMapType` later > > > > returns `OMPC_MAP_MODIFIER_present` and causes the following assert to > > > > fail in `Sema::ActOnOpenMPVarListClause`: > > > > > > > > ``` > > > > assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown && > > > > "Unexpected map modifier."); > > > > ``` > > > > > > > > To me, the most obvious solution is to fix `isMapType` and > > > > `isMapModifier`. Fortunately, looking in `OpenMPKinds.h`, the > > > > enumerator values in `OpenMPMapModifierKind` and `OpenMPMapClauseKind` > > > > are disjoint so we can tell which we have. > > > Can we have something like: > > > ``` > > > if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present) > > > return OMPC_MAP_MODIFIER_unknown; > > > ``` > > > or extend `getOpenMPSimpleClauseType` function with the version parameter > > > and check there is modifier is allowed and return `unknown` if it is not > > > compatible with provided version? > > As far as I can tell, my changes general fix this bug in `isMapType` and > > `isMapModifier`. It makes them behave as documented. The suggestions > > you're making only fix them for the case of `present`. Why is that better? > It is too general. I think it may mask some issues in future. That's why I > would suggest to tweak it for `present` only. Or, even better, extend > `getOpenMPSimpleClauseType` function to handle this modifiers more correctly > for each particular version rather than fix it here. > Or, even better, extend getOpenMPSimpleClauseType function to handle this > modifiers more correctly for each particular version rather than fix it here. One way to implement what I think you mean is to add an additional parameter to `getOpenMPSimpleClauseType` to request either the clause's associated type or that type's modifiers. Unless I missed a clause, this parameter would affect only map, defaultmap, and schedule clauses. For other clauses, this parameter would be ignored. Is that what you have in mind? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83061/new/ https://reviews.llvm.org/D83061 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
