[PATCH] D109635: [WIP][OpenMP] Support construct trait set for Clang

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:737
+  }
+  void eraseConstructTrait() { ConstructTraits.clear(); }
+
----------------
cchen wrote:
> jdoerfert wrote:
> > Modification was made to make it clear what happens but now I think this is not needed.
> > 
> > See below first. Maybe the interface should be "handleConstructTrait(Property, bool Insert)" and if Insert is false you verify it's the last one and delete it. You'd need to traverse them in opposite order though to verify, unsure if verification is therefore strictly necessary.
> For the delete part, I'm not sure how passing `Property` would work since I'm not deleting all the construct trait properties inside `ActOnOpenMPRegionEnd` function.
I think I've figured out a way to achieve what you suggest.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4491
                                       ArrayRef<OMPClause *> Clauses) {
+  DSAStack->eraseConstructTrait();
   if (DSAStack->getCurrentDirective() == OMPD_atomic ||
----------------
jdoerfert wrote:
> I know think you need to call something like `addDeclareVariantConstructTrait` again and do pop_back() instead of push_back() for each trait matching the DKind. Clearing, or erasing a fixed amount of traits, does not work. 
So you are saying that instead of removing all the construct property inside `ActOnOpenMPRegionEnd`, we should delete those construct properties more fine-grained? I'd like to do so and has tried putting the insert/delete logic inside more specific function such as `ActOnOpenMPParllell`, 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109635