ABataev added inline comments.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15238
+ Sema &SemaRef;
+ OpenMPClauseKind CKind;
+ OMPClauseMappableExprCommon::MappableExprComponentList &Components;
----------------
Default initializer here
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15240
+ OMPClauseMappableExprCommon::MappableExprComponentList &Components;
+ bool NoDiagnose;
+ const Expr *RelevantExpr{nullptr};
----------------
Default initializer
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15241-15243
+ const Expr *RelevantExpr{nullptr};
+ bool AllowUnitySizeArraySection{true};
+ bool AllowWholeSizeArraySection{true};
----------------
Better to use ` = <init>;` here. Just a nit.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15258
+ if (!isa<VarDecl>(DRE->getDecl()))
+ return false;
+ RelevantExpr = DRE;
----------------
`emitErrorMsg()` here?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15266
+ bool VisitMemberExpr(MemberExpr *ME) {
+ Expr *E = cast<Expr>(ME);
+ Expr *BaseE = ME->getBase()->IgnoreParenImpCasts();
----------------
No need `cast` here.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15267
+ Expr *E = cast<Expr>(ME);
+ Expr *BaseE = ME->getBase()->IgnoreParenImpCasts();
----------------
Better to use `IgnoreParenCasts()` to handle explicit casting to the base class.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283
+ return false;
+ return Visit(E->IgnoreParenImpCasts());;
+ }
----------------
cchen wrote:
> @ABataev, in the previous patch you mentioned that we don't need
> `IgnoreParenImpCasts()` here, but I think I was just trying to do what line
> 15232 did.
Just `Visit(E)`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15299
+ return false;
+ return Visit(E->IgnoreParenImpCasts());;
+ }
----------------
Just `Visit(E)`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15317
}
+ return Visit(E->IgnoreParenImpCasts());;
+ }
----------------
Just `Visit(E)`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15332
+ Components.emplace_back(ME, FD);
+ return RelevantExpr || Visit(E->IgnoreParenImpCasts());
+ }
----------------
Just `Visit(E)`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15344
}
+ return Visit(E->IgnoreParenImpCasts());
+ }
----------------
`Visit(E)`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15354
- // Record the component.
- CurComponents.emplace_back(CurE, FD);
- } else if (auto *CurE = dyn_cast<ArraySubscriptExpr>(E)) {
- E = CurE->getBase()->IgnoreParenImpCasts();
-
- if (!E->getType()->isAnyPointerType() && !E->getType()->isArrayType()) {
- if (!NoDiagnose) {
- SemaRef.Diag(ELoc, diag::err_omp_expected_base_var_name)
- << 0 << CurE->getSourceRange();
- return nullptr;
+ if (const auto *TE = dyn_cast<CXXThisExpr>(E)) {
+ Expr::EvalResult Result;
----------------
`E->IgnoreParensCasts()`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15356
+ Expr::EvalResult Result;
+ if (AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
+ if (!Result.Val.getInt().isNullValue()) {
----------------
Need to check that `AE->getIdx()` is not value dependent, otherwise it may crash
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15370
- if (const auto *TE = dyn_cast<CXXThisExpr>(E)) {
- Expr::EvalResult Result;
- if (CurE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
- if (!Result.Val.getInt().isNullValue()) {
- SemaRef.Diag(CurE->getIdx()->getExprLoc(),
- diag::err_omp_invalid_map_this_expr);
- SemaRef.Diag(CurE->getIdx()->getExprLoc(),
- diag::note_omp_invalid_subscript_on_this_ptr_map);
- }
- }
- RelevantExpr = TE;
- }
-
- // Record the component - we don't have any declaration associated.
- CurComponents.emplace_back(CurE, nullptr);
- } else if (auto *CurE = dyn_cast<OMPArraySectionExpr>(E)) {
- assert(!NoDiagnose && "Array sections cannot be implicitly mapped.");
- E = CurE->getBase()->IgnoreParenImpCasts();
+ return E && Visit(E->IgnoreParenImpCasts());
+ }
----------------
Just `Visit(E)`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15419-15428
+ if (OASE->getLength()->EvaluateAsInt(ResultR,
+ SemaRef.getASTContext())) {
+ if (!ResultR.Val.getInt().isOneValue()) {
+ SemaRef.Diag(OASE->getLength()->getExprLoc(),
+ diag::err_omp_invalid_map_this_expr);
+ SemaRef.Diag(OASE->getLength()->getExprLoc(),
+ diag::note_omp_invalid_length_on_this_ptr_mapping);
----------------
Also, need to be sure that the expressions here are not value-dependent.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15444-15473
+ bool VisitUnaryOperator(UnaryOperator *UO) {
+ emitErrorMsg();
+ return false;
+ }
+ bool VisitCXXThisExpr(CXXThisExpr *CTE) {
+ return true;
+ }
----------------
Just add:
```
boo VisitStmt(Stmt *) {
emitErrorMsg();
return false;
}
```
instead of all these functions, returning `false` as a result.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15497-15499
+ } else {
+ return nullptr;
+ }
----------------
No need for `else` here, just unconditional `return nullptr`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74970/new/
https://reviews.llvm.org/D74970
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
