ABataev added inline comments.
================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2343-2344
+ // pointer
+ if (Arg.back() < OMPC_DEFAULTMAP_MODIFIER_unknown)
+ Arg.back() = OMPC_DEFAULTMAP_MODIFIER_unknown;
KLoc.push_back(Tok.getLocation());
----------------
cchen wrote:
> ABataev wrote:
> > Is this possible at all? I think it must an assertion rather this kind of
> > trick.
> This is definitely possible since the `getOpenMPSimpleClauseType` function in
> OpenMPKinds.cpp, parse defaultmap modifier and defaultmap kind in a same
> stringswitch statement, so for `defaultmap(scalar`, it will set the
> defaultmap modifier to be 0, however, the unknown is 3
> (OMPC_DEFAULTMAP_MODIFIER_unknown == OMPC_DEFAULTMAP_unknown).
Ok, I see. Then store the result in the temp var and change it there rather use
`Arg.back() = ...`.I still don't like this solution but the redesign is the
different problem.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:659
+ DefaultMapImplicitBehavior DMIB = getDefaultDMIB(DMVC);
+ return (DMIB == DMIB_unspecified) || (DMIB == DMIB_firstprivate) ||
+ (DMIB == DMIB_default);
----------------
Why `DMIB_firstprivate` is the default behavior? This is true only for scalars
and pointers. Plus, this function is used in the context that does not match
its name. Better to rename it to something like `mustBeFirstprivate` or
something similar.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:663
+ bool
+ isDefaultmapDefaultBehaviorAtLevel(unsigned Level,
+ DefaultMapVariableCategory DMVC) const {
----------------
Same here
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2824
+static DefaultMapVariableCategory getVariableCategoryFromDecl(ValueDecl *VD) {
+ if (VD->getType().getNonReferenceType()->isAnyPointerType())
----------------
`const ValueDecl *`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2841-2860
+ switch (DMIB) {
+ case DMIB_alloc:
+ Kind = OMPC_MAP_alloc;
+ break;
+ case DMIB_to:
+ Kind = OMPC_MAP_to;
+ break;
----------------
Why inner switch for the same variable `DMIB`?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4451
+ SmallVector<Expr *, 4> ImplicitMaps[OMPC_MAP_delete];
+ for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+ ArrayRef<Expr *> ImplicitMap =
----------------
Use preincrement instead of postincrement.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4476
}
- if (!ImplicitMaps.empty()) {
- CXXScopeSpec MapperIdScopeSpec;
- DeclarationNameInfo MapperId;
- if (OMPClause *Implicit = ActOnOpenMPMapClause(
- llvm::None, llvm::None, MapperIdScopeSpec, MapperId,
- OMPC_MAP_tofrom, /*IsMapTypeImplicit=*/true, SourceLocation(),
- SourceLocation(), ImplicitMaps, OMPVarListLocTy())) {
- ClausesWithImplicit.emplace_back(Implicit);
- ErrorFound |=
- cast<OMPMapClause>(Implicit)->varlist_size() !=
ImplicitMaps.size();
- } else {
- ErrorFound = true;
+ for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+ if (!ImplicitMaps[I].empty()) {
----------------
Use range-based loop here.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4477
+ for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+ if (!ImplicitMaps[I].empty()) {
+ CXXScopeSpec MapperIdScopeSpec;
----------------
Simplify inner complexity here by using `continue;` if the condition is true.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16393-16416
+static DefaultMapImplicitBehavior
+getImplicitBehaviorFromModifier(OpenMPDefaultmapClauseModifier M) {
+ switch (M) {
+ case OMPC_DEFAULTMAP_MODIFIER_alloc:
+ return DMIB_alloc;
+ case OMPC_DEFAULTMAP_MODIFIER_to:
+ return DMIB_to;
----------------
Do we need types `DefaultMapImplicitBehavior` and `DefaultMapVariableCategory`
if the map 1 to 1 to `OpenMPDefaultmapClauseModifier` and
`OpenMPDefaultmapClauseKind`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69204/new/
https://reviews.llvm.org/D69204
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
