aaronpuchert added inline comments.
================ Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h:277 + /// The kind of capability as specified by @ref CapabilityAttr::getName. + StringRef CapKind; + ---------------- aaron.ballman wrote: > Hrmmmm, I think this may actually be safe, but it does give me pause to store > a `StringRef` as anything other than a local (or param/return type): > https://llvm.org/docs/ProgrammersManual.html#the-stringref-class > > Can you double-check my thinking? This is fine because it's pulling the > `StringRef` from the `CapabilityAttr`, and that stores its name internally > and isn't released until the AST is destroyed. Exactly, aside from two places where I'm storing a string literal ("mutex" and "wildcard"), the common case is taking the `StringRef` (as returned by `CapabilityAttr::getName`) from the attribute. So that would be lifetime-coupled to the AST and should be safe for our analysis. Of course `StringRef` is implicitly constructible from other types, and we wouldn't want that. Especially `std::string` would be an issue. Perhaps we should prevent implicit conversions, and thus force the caller to pass a `StringRef` glvalue, by overloading with a deleted template? ``` template<typename T> CapabilityExpr(const til::SExpr *, T, bool) = delete; ``` Or maybe you've got a better idea. As for a justification: we're building lots of `CapabilityExpr`s, essentially every time we see a capability expression in the code (in attributes or as capability type method call arguments). Given that the kind is only used for actual warnings I wouldn't want us to spend of lot of time or memory on storing it. We could also store the original `Expr*` and extract on demand, but that seemed to me antithetical to translating into the TIL. Of course it would be slightly faster, but the current code (before this change) also extracts capability names eagerly without waiting for the need to arise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124131/new/ https://reviews.llvm.org/D124131 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
