lildmh marked 12 inline comments as done.
lildmh added inline comments.
================
Comment at: lib/AST/OpenMPClause.cpp:1469
void OMPClausePrinter::VisitOMPFromClause(OMPFromClause *Node) {
if (!Node->varlist_empty()) {
----------------
ABataev wrote:
> Better to split this patch into 2: one for `to` clause and another one for
> `from` clause
Hi Alexey,
Thanks for the quick review! The code for `to` and `from` clauses are almost
identical, so I prefer to have them in the same patch. Besides I don't have
commit privilege, so it causes more time to me to have multiple patches. But if
you think it's crucial to split it. I can do it. What do you think?
================
Comment at: lib/Sema/SemaOpenMP.cpp:13061
MapperIdScopeSpec.getWithLocInContext(SemaRef.Context), MapperId,
- /*ADL=*/false, /*Overloaded=*/true, URS.begin(), URS.end());
+ /*ADL=*/true, /*Overloaded=*/true, URS.begin(), URS.end());
}
----------------
ABataev wrote:
> Better to fix this in a separate patch.
I can do that.
================
Comment at: lib/Sema/SemaOpenMP.cpp:13356
- // Try to find the associated user-defined mapper.
- ExprResult ER = buildUserDefinedMapperRef(
- SemaRef, DSAS->getCurScope(), *MapperIdScopeSpec, *MapperId,
- Type.getCanonicalType(), UnresolvedMapper);
- if (ER.isInvalid())
- continue;
- MVLI.UDMapperList.push_back(ER.get());
- } else {
- MVLI.UDMapperList.push_back(nullptr);
}
----------------
ABataev wrote:
> Are you sure about this change? This might trigger some asserts.
Yes. The original code before the mapper patch is like this. By moving it out,
`to` and `from` clauses will also call `buildUserDefinedMapperRef` check the
mapper as well.
================
Comment at: lib/Sema/SemaOpenMP.cpp:13413
- // If the identifier of user-defined mapper is not specified, it is
"default".
- if (!MapperId.getName() || MapperId.getName().isEmpty()) {
----------------
ABataev wrote:
> Again, better to do this in a separate patch
This modification is for `to` and `from` clauses, so they also have a default
mapper name. I think it is appropriate to have this piece in this patch.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58523/new/
https://reviews.llvm.org/D58523
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
