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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:2108
 void Sema::PushCapturedRegionScope(Scope *S, CapturedDecl *CD, RecordDecl *RD,
                                    CapturedRegionKind K) {
+  CapturedRegionScopeInfo *CSI = new CapturedRegionScopeInfo(
----------------
ABataev wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > 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.
> > > I think the check that `OpenMPLevel` is the same prevents the problem you mention.
> > Ah, yes, missed this. Anyway, better to specify this explicitly as function parameters, I think, rather than trying to evaluate it here this way. We eventually may change the handling of captured regions in OpenMP, better to have everything in SemaOpenMP.cpp
> BTW, here you can use the parameter with the default value `0`.
OK.

`CaptureLevel` or `CapturedLevel`?  Do you care?


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

https://reviews.llvm.org/D65835