aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM with the extra safety measure added. I'm happy to review again if you'd like, but don't require it. ================ Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h:277 + /// The kind of capability as specified by @ref CapabilityAttr::getName. + StringRef CapKind; + ---------------- aaronpuchert wrote: > 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. > 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? I think that's likely a good safety measure, good suggestion! > As for a justification: Oh, I already thought it was well-motivated, I just wanted to make sure I wasn't wrong about the lack of lifetime issues. :-) 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
