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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15319
       }
+      return Visit(E);
+    }
----------------
`return RelevantExpr || Visit(E);`?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15346
       }
+      return Visit(E);
+    }
----------------
`return RelevantExpr || Visit(E);`?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15358-15359
+      Expr::EvalResult Result;
+      if (AE->isValueDependent())
+        return false;
+      if (AE->getIdx()->EvaluateAsInt(Result, SemaRef.getASTContext())) {
----------------
1. Need to check `AE->getIdx()`, not `AE`.
2. Why return `false` here? I would say, just skip the check.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15374
 
-      QualType CurType =
-          OMPArraySectionExpr::getBaseOriginalType(E).getCanonicalType();
+    return E && Visit(E);
+  }
----------------
`return RelevantExpr || Visit(E);`?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15423-15424
+      Expr::EvalResult ResultL;
+      if (OASE->isValueDependent())
+        return false;
+      if (OASE->getLength()->EvaluateAsInt(ResultR,
----------------
1. Need to check `OASE->getLength()`, not `OASE`.
2. Why return `false` here? I would say, just skip the check.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15434
       }
-
-      // Record the component - we don't have any declaration associated.
-      CurComponents.emplace_back(CurE, nullptr);
-    } else {
-      if (!NoDiagnose) {
-        // If nothing else worked, this is not a valid map clause expression.
-        SemaRef.Diag(
-            ELoc, diag::err_omp_expected_named_var_member_or_array_expression)
-            << ERange;
+      if (OASE->getLowerBound() && OASE->getLowerBound()->EvaluateAsInt(
+          ResultL, SemaRef.getASTContext())) {
----------------
Need a check for value-dependent `OASE->getLowerBound()`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15448
+    Components.emplace_back(OASE, nullptr);
+    return RelevantExpr || Visit(E->IgnoreParenImpCasts());
   }
----------------
Just `Visit(E)`;


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15450
   }
+  bool VisitCXXThisExpr(CXXThisExpr *CTE) { return true; }
+  bool VisitStmt(Stmt *) {
----------------
Do you really need this function?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15467-15470
+// Return the expression of the base of the mappable expression or null if it
+// cannot be determined and do all the necessary checks to see if the expression
+// is valid as a standalone mappable expression. In the process, record all the
+// components of the expression.
----------------
Use `\\\` style of comment here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74970/new/

https://reviews.llvm.org/D74970