[PATCH] D127803: Generate the capture for field when the field is used in openmp region with implicit default in the member function.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:201
+      const FieldDecl *FD = nullptr;
+      size_t Sz = -1;
+      VarDecl *VD = nullptr;
----------------
ABataev wrote:
> What is Sz here? Better to give a better name and add a description for the struct and all fields
How about use StackLevel.  Sorry use 0 instead


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1143
+        break;
+      Sz--;
+    }
----------------
ABataev wrote:
> Is it possible to have an overflow here?
I don't think that is possible.  when I != EndI and Stacklevel should > 0  Or if I == EndI and Stacklevel should be 0

I add assert to check boundary condition be ensure:
 assert((StackLevel > 0 && I != EndI) || (StackLevel == 0 && I == EndI));





================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1148
+    for (const auto &IFD : I->ImplicitDefaultFirstprivateFDs)
+      if (IFD.FD == FD && IFD.Sz == Sz)
+        return IFD.VD;
----------------
ABataev wrote:
> What if Sz == -1?
Should not be 0.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1176
+      }
+      Sz--;
+    }
----------------
ABataev wrote:
> What about overflow here?
Add assert for boundary check.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:17480
     DeclRefExpr *Ref = nullptr;
-    if (!VD && !CurContext->isDependentContext())
-      Ref = buildCapture(*this, D, SimpleRefExpr, /*WithInit=*/false);
-    DSAStack->addDSA(D, RefExpr->IgnoreParens(), OMPC_private, Ref);
+    if (!VD && !CurContext->isDependentContext()) {
+      auto *FD = dyn_cast<FieldDecl>(D);
----------------
ABataev wrote:
> A check here not for curcontext dependent but for FD being dependent?
I can not this to work.  Since the private copy only build under only build non-dependent context. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127803