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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits