[Openmp-commits] [PATCH] D101030: [OpenMP] Overhaul `declare target` handling

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2134-2151
+    Sema::DeclareTargetContextInfo *DTCI =
+        new Sema::DeclareTargetContextInfo(DKind, DTLoc);
+    if (HasClauses)
+      ParseOMPDeclareTargetClauses(*DTCI);
 
     // Skip the last annot_pragma_openmp_end.
     ConsumeAnyToken();
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > Add a RAII to control these new/delete or just create local var
> > > Local variable and RAII do not always work, there is a delete in the `end_declare_target` below as well.
> > > 
> > > 
> > But here you have new/delete pair within a single block. Either you can put the ar into stack/RAII or some extra checks are missing. Maybe must be something like this:
> > ```
> > if (DKind == OMPD_declare_target)
> >   delete DTCI;
> > ```
> > ?
> If this is a directive without implicit mappings, the DTCI will be deleted right away. It is just used to collect the clauses (which include explicit mappings). If this is a directive with implicit mappings, which we cannot tell from the DKind alone, the DTCI will be stored in Sema until we reach the corresponding end directive. For the former case we could do stack/RAII, but not for the latter case, the scope is left and DTCI survives. I'll look into this again and try to avoid new/delete, feel free to look at the rest of the patch.
Everything else looks good, just need to clarify the question with this new/delete. I missed `if (HasImplicitMappings)` check, so maybe this is correct but better to double-check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101030