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: > > > > > 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? > I would also add a check for version compatibility if the modifier is > supported for the given version. But anyway, I would like to take a look at > what you have in mind I think I misread your previous comment. I believe I've implemented your suggestion now. 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
