danakj wrote: > Hi! Thank you for digging into this! Sorry for the delay. > > > The new UnsafeBufferUsageCtorAttrGadget gadget explicitly avoids matching > > against the std::span(ptr, size) constructor because that is handled by > > SpanTwoParamConstructorGadget and we never want two gadgets to match the > > same thing (and this is guarded by asserts). > > Hmm at a glance I'm not sure this should really be illegal. It's a big > problem when fixable gadgets overlap, because this means that they'll try to > fix the same code in two incompatible ways. I don't immediately see why this > would be a problem for warning gadgets that don't try to fix anything. This > just means that the code is non-compliant for two different reasons, which is > theoretically fine. I also tried to reproduce your assert and I didn't hit > it; which one are you running into?
https://github.com/llvm/llvm-project/blob/2ff43ce87e66d9324370e35ea6743ef57400c76e/clang/lib/Analysis/UnsafeBufferUsage.cpp#L1373-L1374 These assert that exactly one gadget matched. I think it's kinda worthwhile keeping the warnings independent too, so I don't see this as a bad thing. If we were to mark the span ctor as `[[clang::unsafe_buffer_usage]]` for instance, it the span-specific warning gives a better output still, and generating two warnings for the same piece of code is noisy. > > To handle this we allow the gadget to control if the warning is general (it > > calls handleUnsafeBufferUsage()) or is a std-container-specific warning (it > > calls handleUnsafeOperationInContainer()). > > Ooo I love this. > > My initial thinking was, just make the base Gadget class "public" (move it > into `UnsafeBufferUsage.h` so that the handler could see it) and make the > handler switch over the gadget's `getKind()` to produce specialized messages > for each gadget. This would help us unscrew the rest of the `Reporter` code > so that it also didn't have to guess by statement kind. > > Your design can grow into that too - make a separate handler method for each > gadget kind (or group of kinds), and it preserves even more encapsulation. > Additionally it throws away `getBaseStmt()` in favor of a more "integrated" > solution. I'm a big fan, let's keep this. :+1: https://github.com/llvm/llvm-project/pull/91777 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
