donat.nagy added a comment. In D152436#4437822 <https://reviews.llvm.org/D152436#4437822>, @steakhal wrote:
> In D152436#4437692 <https://reviews.llvm.org/D152436#4437692>, @donat.nagy > wrote: > >> Personally I think it's completely acceptable if the analyzer sometimes >> emits bug reports that are true positives but lack a message [...] > > I must admit that I'm in the other camp. To clarify my position the "sometimes" in my comment should've been "rarely" or something closer to that. I don't want to release confusing garbage, I'm just trying to optimize sum(helpfulness of checkers) and I feel that the last few steps towards perfect helpfulness are often disproportionately expensive. If a code contains two bugs, then two rough but understandable warnings are more valuable than one well-polished friendly warning + one completely missed bug (because we didn't have time to write a checker for it). --- @balazske: Do I understand it correctly that the "create note that's shown when the return value is marked as interesting" (the option 2 that I suggested) adds / will add clarifying notes to the fileno/fstat error reports and the ftell issue <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=xerces_v3.2.3_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=4ab640064066880ac7031727869c92f4&report-id=1932564&report-filepath=%2aThreadTest.cpp>? Did you see any real-life examples where this option 2 does not provide useful notes? If not, then I think we can accept that option 2 is a sufficient solution for these issues. In addition to this, there are these issues noted by @Szelethus: In D152436#4408199 <https://reviews.llvm.org/D152436#4408199>, @Szelethus wrote: > In D152436#4405558 <https://reviews.llvm.org/D152436#4405558>, @balazske > wrote: > >> link >> <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=e7c81b8628bb636f9087a14b446dcb00&report-id=1930482&report-filepath=%2aclient.c> >> The function `open` is not modeled in `StdCLibraryFunctionsChecker`, it >> should not return less than -1 but this information is not included now. >> link >> <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=twin_v0.8.1_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=ed57f448095d9ba67d047a45898d1ebb&report-id=1930547&report-filepath=%2alibTw.c> >> `socket` can not return less than -1 but this function is not modeled >> currently. > > These should be a rather painless fix, right? > > [...] > >> link >> <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclibraryfunctions_test&is-unique=on&page=2&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=ebc6fce212f7238143e8f97b1d231abf&report-id=1932035&report-filepath=%2arelcache.c> >> `fwrite` with 0 buffer and 0 size should not be an error, this is not >> checked now. > > Some discussion for that: D140387#inline-1360054 > <https://reviews.llvm.org/D140387#inline-1360054>. There is a FIXME in the > code for it -- not sure how common this specific issue is, but we did stumble > on it in an open source project... how painful would it be to fix this? If it's not too difficult, then please upload a new version of this patch that implements "option 2" (i.e. produces notes when the return value of the std library function is marked as interesting) + handles the three requests of @Szelethus. I hope that you can do this and then this review can be concluded quickly. If there are significant obstacles (or I misunderstood the situation), then please reply and discuss the next steps. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152436/new/ https://reviews.llvm.org/D152436 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
