donat.nagy requested changes to this revision. donat.nagy added a comment. This revision now requires changes to proceed.
As I started to review the code of the follow-up commit D153776 <https://reviews.llvm.org/D153776>, I noticed a dangling `StringRef` bug which belongs to this review. Moreover, as a minor remark I'd note that LLVM has a dedicated class for storing string literal constants (btw I learned this from @Szelethus yesterday). ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1303 + // use it as a note tag. + StringRef Note = Case.getNote(); + if (Summary.getInvalidationKd() == EvalCallAsPure) { ---------------- This variable will be captured and stored by the lambda functions, so it should own the memory area of the string. In the current code this did not cause problems because this StringRef points to the memory area of a string literal (which stays valid); but if later changes introduced dynamically generated note tags, then this code would dump random memory garbage. By the way, a very similar issue survived almost four years in CheckerContext.h, I'm fixing it in D153889. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1318 + if (Node->succ_size() > 1) + return Note.str(); + return ""; ---------------- Also update this when you change the type of `Note` to `std::string`. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1687-1688 + const char *GenericSuccessMsg = "Assuming that the call is successful"; + const char *GenericFailureMsg = "Assuming that the call fails"; + ---------------- Consider using the `StringLiteral` subclass of `StringRef` which is designed for this kind of application (and determines the length of the string in compile time instead of calling `strlen` during a runtime `const char * → StringRef` conversion): https://llvm.org/doxygen/classllvm_1_1StringLiteral.html (After the suggested change, also update the code that uses these variables!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153612/new/ https://reviews.llvm.org/D153612 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
