[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 14 09:18:10 PST 2020
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
More information about the cfe-commits
mailing list