[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:763
                           RecordDecl *RD, ImplicitParamDecl *Context,
                           CapturedRegionKind K, unsigned OpenMPLevel)
       : CapturingScopeInfo(Diag, ImpCap_CapturedRegion),
----------------
I would add a parameter for `OpenMPCaptureLevel` rather than use default value.


================
Comment at: clang/include/clang/Sema/Sema.h:9026
+  bool isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level,
+                             unsigned CaptureLevel = 0) const;
 
----------------
Do not use default value here, just set it to `0` in the call in SemaOpenMP.cpp. We use it there tat target level always and thus `CaptureLevel` is always `0` there.


================
Comment at: clang/lib/Sema/Sema.cpp:2108
 void Sema::PushCapturedRegionScope(Scope *S, CapturedDecl *CD, RecordDecl *RD,
                                    CapturedRegionKind K) {
+  CapturedRegionScopeInfo *CSI = new CapturedRegionScopeInfo(
----------------
I would suggest adding a new parameter for CapturedLevel and the same parameter to `ActOnCapturedRegionStart` function. And pass the value for `CapturedLevel` explicitly at `ActOnCapturedRegionStart` function call in SemaOpenMP.cpp rather than rely on this solution.
Also, for each separate OpenMP directive, this counter must start from `0`. Currently, I think, it may lead to crash if we have several directives. Something like:
```
#pragma omp parallel
#pragma omp target teams
...
```
I think it will work incorrectly for this example.


================
Comment at: clang/lib/Sema/Sema.cpp:2109
                                    CapturedRegionKind K) {
-  CapturingScopeInfo *CSI = new CapturedRegionScopeInfo(
+  CapturedRegionScopeInfo *CSI = new CapturedRegionScopeInfo(
       getDiagnostics(), S, CD, RD, CD->getContextParam(), K,
----------------
`CapturedRegionScopeInfo *`->`auto *`


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

https://reviews.llvm.org/D65835