[PATCH] D72811: [WIP][OPENMP5.0] allow lvalue for motion clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15201
+namespace {
+class LocatorChecker final : public StmtVisitor<LocatorChecker, bool> {
+  OMPClauseMappableExprCommon::MappableExprComponentList Components;
----------------
Seems to me, you're skipping many expressions, which could be supported. Did you try to test it with casting expressions and others?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15203-15205
+  DeclRefExpr *DRE;
+  CXXThisExpr *CTE;
+  size_t DerefCnt;
----------------
Use default initializers for fields.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15213
+  bool VisitDeclRefExpr(DeclRefExpr *E) {
+    if (const auto *VD = dyn_cast<VarDecl>(E->getDecl())) {
+      const clang::Type *PT = E->getType().getTypePtr();
----------------
Better to use early exit where possible to reduce structural complexity of the functions.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15215-15216
+      const clang::Type *PT = E->getType().getTypePtr();
+      const std::string TypeStr = PT->getCanonicalTypeInternal().getAsString();
+      size_t PtrCnt = std::count(TypeStr.begin(), TypeStr.end(), '*');
+      // Normally we want to add Components if DerefCnt == PtrCnt, for example:
----------------
This really looks ugly. Need to find a better solution.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15233
+      //  PtrCnt for B: 0
+      if (DerefCnt <= PtrCnt) {
+        pushComponentIfNoBaseDecl(E, E->getDecl());
----------------
Why do you need to count the number of dereferences? And have this comparison here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15368
 
     if (auto *CurE = dyn_cast<DeclRefExpr>(E)) {
+      if (!(isa<VarDecl>(CurE->getDecl())))
----------------
If you have the visitor class, you can replace all this code with an analysis in the visitor.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15369
     if (auto *CurE = dyn_cast<DeclRefExpr>(E)) {
-      if (!isa<VarDecl>(CurE->getDecl()))
+      if (!(isa<VarDecl>(CurE->getDecl())))
         return nullptr;
----------------
Restore original code


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15554
+        LocatorChecker Checker;
+        if (Checker.Visit(OrigExpr)) {
+          llvm::copy(Checker.getComponents(),
----------------
General question about several cases. How we're going to support them?
1. (a ? b : c). 
2. __builtin_choose_expr(a, b, c).
3. a = b.
4. a?:b
5. __builtin_convertvector(x, ty)
6. (int&)a
7. v.xy, where v is an extended vector
8. a.b, where b is a bitfield
9. __builtin_bit_cast(v, ty)
10. const_cast<ty &>(a)
11. dynamic_cast<ty &>(a)
12. reinterpret_cast
13. static_cast
14. typeid() (also is an lvalue).
15. __uuidof(*comPtr)
16. lambda calls
17. User defined literals



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15555
+        if (Checker.Visit(OrigExpr)) {
+          llvm::copy(Checker.getComponents(),
+                     std::back_inserter(CurComponents));
----------------
Better to pass `CurComponents` as an argument at the construction of the checker to avoid extra filling/copying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72811