On Jan 29, 2014, at 7:45 AM, Alexander Kornienko <[email protected]> wrote:
> On Sat, Jan 25, 2014 at 7:50 AM, Ted Kremenek <[email protected]> wrote: > Hi Alex, > > Thank you for working on this. I’m going to review the patch in its > entirety. I’m glad to see this information get pushed through, but I’m a bit > concerned about the approach. > > A few high bits: > > (1) This patch modifies every call to registerChecker<T>() just to pass > through an extra string. This seems like excessive boilerplate. > > (2) The name ‘CheckerNameWrapper’ is a bit too precise to the point of being > confusing. The “Wrapper” part has no meaning unless you look at the > implementation of the class. If we want to have a custom type here, > “CheckerName” seems to capture the intention more succinctly. I’m also not > certain if adding this extra type is necessary; a StringRef is probably fine. > > I've renamed CheckerNameWrapper to CheckerName. As for the need in this class > in general, it serves as a guard against random StringRefs being passed to > BugType. This was suggested by Jordan (I'm not sure, that the implementation > is exactly what he imagined, though), and I think it's a reasonable > precaution. Yes, it sounds good to me. ‘CheckerName’ is a big improvement over ‘CheckerNameWrapper’, and we can always refine the terminology later. > > ... > There’s a single loop that goes and creates all the checker objects, which (I > believe) includes both builtin checkers and ones loaded from plugins. The > CheckerInfo object contains the full string of the checker. What if we > changed the loop body to something like this: > > { > checkerManager.setCheckerName((*I)->FullName; > (*i)->Initialize(checkerMgr); > } > > Then in CheckerManager, we can modify CheckerManager::registerChecker<T>() to > consult that StringRef, and if it is there, use it to splat the name into the > CheckerBase object. You then don’t have to touch the individual > registerXXXChecker() functions at all. It’s less boilerplate for checker > authors, and also less error prone. It is also extensible if we ever want to > plumb in more data. I know it’s a bit gross since it’s meddling with some > shared state in CheckerManager, but this initialization really only happens > up front during startup. > > I was thinking about this alternative before, but chose the other one to > avoid dealing with a mutable shared state. If you prefer this option, I'm > happy to change it. It’s tradeoffs, but I don’t see it as a big deal. CheckerManager isn’t thread-safe when it is being mutated in any way, and all this work happens during initialization. I’d rather do this then punish checker writers with a bunch of redundant boilerplate. Jordan: what are your opinions on this approach? > > Aside from that, is there a reason we want to traffic at all with > CheckerNameWrapper (or rather, CheckerName) for calls to things like > EmitBasicReport? > > I've added an overload of the EmitBasicReport, which receives a pointer to > CheckerBase instead of the checker name. There's one corner-case though: > checkers, that implement several sets of checks (MallocChecker, > CStringChecker and a few others). These checkers need to pass the exact > checker name, which can't be replaced with a reference to the checker. Ah, yes. This concept of “checks” and “checkers” aren’t totally formalized (with the latter implementing the former). What “CheckerName” corresponds to is the specific “Check” or “Checker Diagnostic”. I think this is fine for now, but I think this concept will need to be refined over time. > > For example: > > > void BugReporter::EmitBasicReport(const Decl *DeclWithIssue, > > - StringRef name, > > - StringRef category, > > + CheckerNameWrapper checkerName, > > + StringRef name, StringRef category, > > StringRef str, PathDiagnosticLocation > > Loc, > > ArrayRef<SourceRange> Ranges) { > > > > > Why not always just traffic with Checker&, and avoid introducing one more > concept? This seems a bit more general. If we ever want to retrieve more > information from the Checker, we have a reference to the actual Checker > object, and not just a string. The following line could then be written from… > > > llvm::raw_svector_ostream(fullDesc) << checkerName.getName() << ":" << name > > > to > > > llvm::raw_svector_ostream(fullDesc) << checker.getName() << ":" << > > category; > > > Not only does that seem cleaner, it’s one less concept for the BugReporter > system to think about. I’m even wondering if PathDiagnostic would benefit > from just getting a reference to the entire Checker, for example: > > > PathDiagnostic::PathDiagnostic(StringRef checkerName, const Decl > > *declWithIssue, > > > would become: > > > PathDiagnostic::PathDiagnostic(Checker &checker, const Decl *declWithIssue, > > That also seems more general. As long as the PathDiagnostic object has a > shorter lifetime than the Checker object than this seems perfectly safe to me. > > What do you think? > > It would be interesting , but, as I've said, we can't just replace the > checker name with a reference to the checker. Yes, I forgot about this obviously important point. > > > Cheers, > Ted > > On Jan 22, 2014, at 5:32 AM, Alexander Kornienko <[email protected]> wrote: > > > Avoid copying strings when creating BugTypes or reporting errors. > > Introduced a wrapper type for checker names to ensure we only get > > StringRefs > > from the CheckerRegistry. > > > >> Much better! :-) I wish we could cut down a bit on the copying of the > >> name, but > >> to do that we'd have to rely on the CheckerRegistry always living longer > >> than > >> the diagnostics. We could probably assume that for BugType, though. (If we > >> did > >> that, we'd want to use a wrapper data type so that people didn't > >> accidentally > >> pass random strings.) > > > > Done. > > > >> At the very least, though, even the combined checkers can use StringRef > >> instead > >> of std::string to track the names of the individual checks. > > > > Done. > > > >> It might be nice to have a BugType constructor overload that takes a > >> Checker and > >> just immediately calls getCheckerName(), just so 90% the clients could pass > >> "this", but that's getting fairly nitpicky. > > > > Done for BugType and all its descendants. > > > >> I'd also like Ted to at least give a thumbs-up before this goes in. > > > > Yes, would be nice to hear from Ted on this. > > > > Hi jordan_rose, krememek, > > > > http://llvm-reviews.chandlerc.com/D2557 > > > > CHANGE SINCE LAST DIFF > > http://llvm-reviews.chandlerc.com/D2557?vs=6560&id=6568#toc > > > > Files: > > examples/analyzer-plugin/MainCallChecker.cpp > > include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h > > include/clang/StaticAnalyzer/Core/BugReporter/BugType.h > > include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h > > include/clang/StaticAnalyzer/Core/Checker.h > > include/clang/StaticAnalyzer/Core/CheckerManager.h > > include/clang/StaticAnalyzer/Core/CheckerRegistry.h > > lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp > > lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp > > lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp > > lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp > > lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp > > lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp > > lib/StaticAnalyzer/Checkers/CStringChecker.cpp > > lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp > > lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp > > lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp > > lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp > > lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp > > lib/StaticAnalyzer/Checkers/CheckObjCInstMethSignature.cpp > > lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp > > lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp > > lib/StaticAnalyzer/Checkers/ChrootChecker.cpp > > lib/StaticAnalyzer/Checkers/ClangSACheckers.h > > lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp > > lib/StaticAnalyzer/Checkers/DebugCheckers.cpp > > lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp > > lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp > > lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp > > lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp > > lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp > > lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp > > lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp > > lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp > > lib/StaticAnalyzer/Checkers/InterCheckerAPI.h > > lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp > > lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp > > lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp > > lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp > > lib/StaticAnalyzer/Checkers/MallocChecker.cpp > > lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp > > lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp > > lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp > > lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp > > lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp > > lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp > > lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp > > lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp > > lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp > > lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp > > lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp > > lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp > > lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp > > lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp > > lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp > > lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp > > lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp > > lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp > > lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp > > lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp > > lib/StaticAnalyzer/Checkers/StreamChecker.cpp > > lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp > > lib/StaticAnalyzer/Checkers/TraversalChecker.cpp > > lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp > > lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp > > lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp > > lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp > > lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp > > lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp > > lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp > > lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp > > lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp > > lib/StaticAnalyzer/Core/BugReporter.cpp > > lib/StaticAnalyzer/Core/Checker.cpp > > lib/StaticAnalyzer/Core/CheckerRegistry.cpp > > lib/StaticAnalyzer/Core/PathDiagnostic.cpp > > <D2557.4.patch> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
