[PATCH] D74970: [OpenMP] Refactor the analysis in checkMapClauseBaseExpression using StmtVisitor clause.

[Alexey Bataev via Phabricator via cfe-commits]

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