================ @@ -1641,6 +1641,17 @@ def warn_implicitly_retains_self : Warning < "block implicitly retains 'self'; explicitly mention 'self' to indicate " "this is intended behavior">, InGroup<DiagGroup<"implicit-retain-self">>, DefaultIgnore; +def warn_blocks_capturing_this : Warning<"block implicitly captures 'this'">, + InGroup<DiagGroup<"blocks-capturing-this">>, + DefaultIgnore; +def warn_blocks_capturing_reference + : Warning<"block implicitly captures a C++ reference">, + InGroup<DiagGroup<"blocks-capturing-reference">>, + DefaultIgnore; +def warn_blocks_capturing_raw_pointer + : Warning<"block implicitly captures a raw pointer">, ---------------- sdefresne wrote:
I initially developed this change because I was concerned about the number of occurrences of blocks capturing C pointers (or `this` or references) I was seeing during code reviews (or when investigating crashes). At this point this was just a feeling that there was a problem in our code base, but I had no hard data. Having developed this change, I was able to build the code base I'm familiar with (Chrome on iOS) with the three warnings enabled, and spent the last few days looking at all the warnings reported by the current change, trying to get an idea of the volume of true and false positive they would find. I'm still doing the analysis, but I can share some intermediate results. I saved all the build output, then collected all unique warnings triplet "warning-name, filename, line" and looks at each of them. This gives me a lower bound of the number of errors (as the same warning can happen multiple times per line if multiple pointers are captured). Numbers of occurrences per warning in production code (additionally in test code): - `-Wblocks-capturing-this`: 28 (571) - `-Wblocks-capturing-reference`: 7 (37) - `-Wblocks-capturing-raw-pointer`: 53 (174) Trying to categorize the 88 warnings in production - 53 are totally true positive, and would need to be fixed in our code base (60%), - 13 are captured blocks that are non-escaping blocks not marked as such (15%), - 7 are capturing pointers to static variables (8%), - 2 are capturing function pointers (2%) -- likely due to a bug in my implementation, - 2 are pointers to pointers and should have used `__block` variables anyway (2%). The last 11 warnings are in third-party code I'm not really familiar with. If we ignore those, this gives 55 true positive (I'm counting the 53 above and the 2 pointers to pointers here) out of 77 warnings, or 73% of true positive. Among the warnings I left in the false positive, it could be argued that some of them are not false positive but could be counted as true positive. For exemple, among the 7 pointers to static variables, only 4 of them could be deduced statically. The other 3 are blocks capturing `const char*` parameters where the function are only ever called with pointer to static string (this is something I know because I know the code base, but that the compiler cannot prove). I didn't have time to look at all the warnings in test code as there are so many warnings reported there (but I think many could be fixed by marking a few helper function as taking a non-escaping block). Given those numbers, I think those warning are valuable for the Chrome on iOS codebase. I am less familiar with other codebase, though the Chromium codebase is considered of a relatively good quality, so maybe the warnings could also be interesting in other code bases. If you think that despite those numbers the warnings are not interesting in other code bases, then I will consider implementing them as Chrome specific clang plugins (the [Chromium documentation](https://chromium.googlesource.com/chromium/src/+/main/docs/writing_clang_plugins.md) recommends first trying to get warning adopted by clang, and this is why I started this effort). https://github.com/llvm/llvm-project/pull/144388 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
