NoQ added a subscriber: rnkovacs. NoQ added a comment. We should add at least some tests for the other three functions.
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:278 +def CastValueChecker : Checker<"CastValue">, + HelpText<"Model casts">, + Documentation<NotDocumented>; ---------------- "Model implementation of custom RTTIs." ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:250 + /// A shorthand version of getNoteTag that accepts a plain note. + /// ---------------- Great! I recognize the need for such function now. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:25-27 + using CastCheck = + std::function<void(const CastValueChecker *, const CallExpr *, + DefinedOrUnknownSVal, CheckerContext &)>; ---------------- Charusso wrote: > NoQ wrote: > > Kinda nice, but a simple pointer-to-member would have been sufficient and > > much more lightweight: > > > > ```lang=c++ > > using CastCheck = void (CastValueChecker::*)(const CallExpr *, > > DefinedOrUnknownSVal, CheckerContext &); > > ``` > > > > (not sure, i don't fully understand the performance implications of using > > `std::function`) > It took me an hour to realize I have to push the class as the signature of > the function to create a member-callback. I think it is not that lightweight, > but it is modern. I believe it is made for that purpose when you could write > `Foo[13]` or `*(13 + P)`, where the latter creeps me out. > > Thanks for the copy-pasteable code, I really enjoy to see what do you think > exactly, but this is the first time when I would pick my idea. (I would had > recommended that modern way in your patch just I thought it is not working > with methods.) Ok then! ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:58-59 + QualType Ty = Cast->getType(); + if (const CXXRecordDecl *CD = Ty->getPointeeCXXRecordDecl()) + return CD->getName(); + ---------------- I suspect that this may crash if `CD` is an anonymous structure. So you should use `getNameAsString()` instead. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:61 + + return Ty.getAsString(); +} ---------------- Use after free! `QualType::getAsString()` returns a temporary `std::string`. You're returning a `StringRef` that outlives the string it refers to. The solution is usually to return the `std::string` by value. //*summons @rnkovacs*// Generally, i'd rather bail out on this branch. If we're seeing a dyn_cast of something that //isn't a class//, we're already doing something wrong. ================ Comment at: clang/test/Analysis/cast-value.cpp:3 +// RUN: -analyzer-checker=core,apiModeling.CastValue,debug.ExprInspection \ +// RUN: -verify=logic %s +// RUN: %clang_analyze_cc1 \ ---------------- Nice! I guess putting them into separate files would have been easier this time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64374/new/ https://reviews.llvm.org/D64374 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
