<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Jan 25, 2014 at 7:50 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Alex,<br>
<br>
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.<br>
<br>
A few high bits:<br>
<br>
(1) This patch modifies every call to registerChecker<T>() just to pass through an extra string. This seems like excessive boilerplate. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
(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.<br>
</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
...<br>
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:<br>
<br>
{<br>
checkerManager.setCheckerName((*I)->FullName;<br>
(*i)->Initialize(checkerMgr);<br>
}<br>
<br>
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.<br>
</blockquote><div><br></div><div>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.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Aside from that, is there a reason we want to traffic at all with CheckerNameWrapper (or rather, CheckerName) for calls to things like EmitBasicReport? </blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> For example:<br>
<br>
> void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,<br>
> - StringRef name,<br>
> - StringRef category,<br>
> + CheckerNameWrapper checkerName,<br>
> + StringRef name, StringRef category,<br>
> StringRef str, PathDiagnosticLocation Loc,<br>
> ArrayRef<SourceRange> Ranges) {<br>
><br>
<br>
<br>
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…<br>
<br>
> llvm::raw_svector_ostream(fullDesc) << checkerName.getName() << ":" << name<br>
<br>
<br>
to<br>
<br>
> llvm::raw_svector_ostream(fullDesc) << checker.getName() << ":" << category;<br>
<br>
<br>
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:<br>
<br>
> PathDiagnostic::PathDiagnostic(StringRef checkerName, const Decl *declWithIssue,<br>
<br>
<br>
would become:<br>
<br>
> PathDiagnostic::PathDiagnostic(Checker &checker, const Decl *declWithIssue,<br>
<br>
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.<br>
<br>
What do you think?<br></blockquote><div><br></div><div>It would be interesting , but, as I've said, we can't just replace the checker name with a reference to the checker.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Cheers,<br>
<div class="im">Ted<br>
<br>
On Jan 22, 2014, at 5:32 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
<br>
</div><div><div class="h5">> Avoid copying strings when creating BugTypes or reporting errors.<br>
> Introduced a wrapper type for checker names to ensure we only get StringRefs<br>
> from the CheckerRegistry.<br>
><br>
>> Much better! :-) I wish we could cut down a bit on the copying of the name, but<br>
>> to do that we'd have to rely on the CheckerRegistry always living longer than<br>
>> the diagnostics. We could probably assume that for BugType, though. (If we did<br>
>> that, we'd want to use a wrapper data type so that people didn't accidentally<br>
>> pass random strings.)<br>
><br>
> Done.<br>
><br>
>> At the very least, though, even the combined checkers can use StringRef instead<br>
>> of std::string to track the names of the individual checks.<br>
><br>
> Done.<br>
><br>
>> It might be nice to have a BugType constructor overload that takes a Checker and<br>
>> just immediately calls getCheckerName(), just so 90% the clients could pass<br>
>> "this", but that's getting fairly nitpicky.<br>
><br>
> Done for BugType and all its descendants.<br>
><br>
>> I'd also like Ted to at least give a thumbs-up before this goes in.<br>
><br>
> Yes, would be nice to hear from Ted on this.<br>
><br>
> Hi jordan_rose, krememek,<br>
><br>
> <a href="http://llvm-reviews.chandlerc.com/D2557" target="_blank">http://llvm-reviews.chandlerc.com/D2557</a><br>
><br>
> CHANGE SINCE LAST DIFF<br>
> <a href="http://llvm-reviews.chandlerc.com/D2557?vs=6560&id=6568#toc" target="_blank">http://llvm-reviews.chandlerc.com/D2557?vs=6560&id=6568#toc</a><br>
><br>
> Files:<br>
> examples/analyzer-plugin/MainCallChecker.cpp<br>
> include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h<br>
> include/clang/StaticAnalyzer/Core/BugReporter/BugType.h<br>
> include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h<br>
> include/clang/StaticAnalyzer/Core/Checker.h<br>
> include/clang/StaticAnalyzer/Core/CheckerManager.h<br>
> include/clang/StaticAnalyzer/Core/CheckerRegistry.h<br>
> lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp<br>
> lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp<br>
> lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/CStringChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp<br>
> lib/StaticAnalyzer/Checkers/CheckObjCInstMethSignature.cpp<br>
> lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp<br>
> lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp<br>
> lib/StaticAnalyzer/Checkers/ChrootChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ClangSACheckers.h<br>
> lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/DebugCheckers.cpp<br>
> lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp<br>
> lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp<br>
> lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/InterCheckerAPI.h<br>
> lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/MallocChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/StreamChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/TraversalChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp<br>
> lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp<br>
> lib/StaticAnalyzer/Core/BugReporter.cpp<br>
> lib/StaticAnalyzer/Core/Checker.cpp<br>
> lib/StaticAnalyzer/Core/CheckerRegistry.cpp<br>
> lib/StaticAnalyzer/Core/PathDiagnostic.cpp<br>
</div></div>> <D2557.4.patch><br>
<br>
</blockquote></div><br>
</div></div>