ABataev 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); ---------------- 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 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
