Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed.
In D35068#811436 <https://reviews.llvm.org/D35068#811436>, @NoQ wrote: > I wonder how noisy this check is - did you test it on large codebases? > Because these functions are popular, and in many cases it'd be fine to use > insecure functions, i wonder if it's worth it to have this check on by > default. Like, if it's relatively quiet - it's fine, but if it'd constitute > 90% of the analyzer's warnings on popular projects, that'd probably not be > fine. In D35068#1049530 <https://reviews.llvm.org/D35068#1049530>, @george.karpenkov wrote: > @koldaniel Have you evaluated this checker? On which codebases? Were the > warnings real security issues, or were they mostly spurious? The code seems > fine, but I'm not sure whether it should be in `security` or in `alpha`. Sorry, didn't read the discussion, there are some fair points in the quoted comments. In D35068#1069880 <https://reviews.llvm.org/D35068#1069880>, @koldaniel wrote: > I've evaluated this checker on LLVM+Clang, there were only a few (about 15) > warnings, because of the C11 flag check at the beginning of the checker > body. However, if this check was removed, number of the warnings would be > increased significantly. I wouldn't say the findings were real security > issues, most of the warnings were about usages of deprecated functions, which > has not been considered unsecure (but which may cause problems if the code is > modified in an improper way in the future). My problem is that LLVM+Clang isn't really a C (neither a C11) project, and I think judging this checker on it is a little misleading. Could you please test it on some C11 projects? I think tmux uses C11. In D35068#1361195 <https://reviews.llvm.org/D35068#1361195>, @xazax.hun wrote: > I think this is quiet coding guideline specific check which is useful for a > set of security critical projects. As this is an opt in kind of check, I > think it does no harm to have it upstream. I do generally agree with this statement, but I'd be more comfortable either landing it in alpha or seeing some other results. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D35068/new/ https://reviews.llvm.org/D35068 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
