[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from some small nits.



================
Comment at: clang/include/clang/Basic/Attr.td:186-192
+// an OMPTraitInfo object. The structure of an OMPTraitInfo object is a
+// tree as defined below:
+//
+//   OMPTraitInfo     := {list<OMPTraitSet>}
+//   OMPTraitSet      := {Kind, list<OMPTraitSelector>}
+//   OMPTraitSelector := {Kind, Expr, list<OMPTraitProperty>}
+//   OMPTraitProperty := {Kind}
----------------
Rather than describing the internal data structure form, I am hoping for comments that show the format of the user-facing pragma arguments. e.g., what information is in what position. Basically, something so I can mentally map from "OMPTraitsInfoArgument" to "oh, that's right, the arguments are specified in this order with these types" (roughly -- a general idea of the argument is all I'm looking for).


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1379
   // Parse inner context selectors.
-  SmallVector<Sema::OMPCtxSelectorData, 4> Data;
-  if (!parseOpenMPContextSelectors(Loc, Data)) {
-    // Parse ')'.
-    (void)T.consumeClose();
-    // Need to check for extra tokens.
-    if (Tok.isNot(tok::annot_pragma_openmp_end)) {
-      Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
-          << getOpenMPDirectiveName(OMPD_declare_variant);
-    }
-  }
+  OMPTraitInfo *TI = new OMPTraitInfo();
+  parseOMPContextSelectors(Loc, *TI);
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > aaron.ballman wrote:
> > > What is responsible for freeing this memory?
> > Will check and fix if needed.
> I made sure all OMPTraitInfo are freed, I think. If an `OMPDeclareVariantAttr` is build its taking owenership and calls delete in the destructor.
That is reasonable enough -- any chance we can easily turn it into a `std::unique_ptr<>` and then get rid of the destructor entirely? I don't insist, but it would be cleaner, to my thinking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71830