jfb accepted this revision. jfb added inline comments.
================ Comment at: clang/include/clang/AST/Decl.h:1472 + /// Do we need to emit an exit-time destructor for this variable? + bool isNoDestroy(const ASTContext &) const; ---------------- erik.pilkington wrote: > jfb wrote: > > rsmith wrote: > > > jfb wrote: > > > > This is only valid for static variables, right? It's probably better to > > > > document the function name that way, e.g. `isStaticWithNoDestroy`. > > > I think the question (and hence current function name) is meaningful for > > > any variable, but it just happens that the answer will always be "no" for > > > non-static variables (for now, at least). Are you concerned that people > > > will think calls to this function are missing from codepaths that only > > > deal with automatic storage duration variables with the current name? > > Yeah it seems like "this variable has no destructor". I guess even trivial > > automatic variables have a (trivial) destructor, so maybe it's not > > ambiguous? Up to y'all :) > If anyone has a strong preference I'd be happy either way! Considering what Richard pointed out below w.r.t. naming convention, this is good with me. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5931 + handleSimpleAttributeWithExclusions<NoDestroyAttr, AlwaysDestroyAttr>(S, D, A); + } +} ---------------- erik.pilkington wrote: > jfb wrote: > > This allows a variable with both attributes :) > > Looking at the code above it seems like having both attributes is > > equivalent to no-destroy. > `handleSimpleAttributeWithExclusions` handles that automagically, i.e.: > ``` > $ cat t.cpp > [[clang::no_destroy]] [[clang::always_destroy]] int x; > $ ~/dbg-bld/bin/clang t.cpp > t.cpp:1:25: error: 'always_destroy' and 'no_destroy' attributes are not > compatible > [[clang::no_destroy]] [[clang::always_destroy]] int x; > ^ > t.cpp:1:3: note: conflicting attribute is here > [[clang::no_destroy]] [[clang::always_destroy]] int x; > ^ > 1 error generated. > ``` > I'll add a test to prove this though... Ah nice! https://reviews.llvm.org/D50994 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
