Szelethus added a comment. This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended?
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100 +// The APIModeling package is for checkers that model APIs. These checkers are +// always turned on; this package is intended for API modeling that is not +// controlled by the target triple. ---------------- Charusso wrote: > Szelethus wrote: > > This isn't true: the user may decide to only enable non-pathsensitive > > checkers. > > > > I think the comment should rather state that these whether these checkers > > are enabled shouldn't be explicitly specified, unless in **extreme** > > circumstances (causes a crash in a release build?). > Well, I have removed it instead. Makes no sense, you are right. I don't think it's a good idea -- we definitely should eventually be able to list packages with descriptions just like checkers (there actually is a FIXME in CheckerRegistry.cpp for that), but this is the next best thing that we have. How about this: ``` // The APIModeling package is for checkers that model APIs and don't perform // any diagnostics. Checkers within this package are enabled by the core or // through checker dependencies, so one shouldn't enable/disable them by // hand (unless they cause a crash, but that will cause dependent checkers to be // implicitly disabled). ``` ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119 - const T *lookup(const CallEvent &Call) const { + Optional<T> lookup(const CallEvent &Call) const { // Slow path: linear lookup. ---------------- Charusso wrote: > NoQ wrote: > > I hope `T` never gets too expensive to copy. The ideal return value here is > > `Optional<const T &>` but i suspect that `llvm::Optional`s don't support > > this (while C++17 `std::optional`s do). Could you double-check my vague > > memories here? > Optional<const T *> is working and used widely, I like that. Why do we need the optional AND the pointer? How about we just return with a nullptr if we fail to find the call? ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:210 /// the default tag for the checker will be used. + /// @param IsPrunable Whether the note is prunable. ExplodedNode * ---------------- Charusso wrote: > It makes perfectly no sense here. Is it the mentioned "class doc"? You're right. I meant to add it to `NoteTag`s class doc, but whether a `NoteTag` is prunable isn't a property of `NoteTag` itself, but rather the infrastructure around it. Oops! I think above `const NoteTag *getNoteTag()` would be better. ================ Comment at: clang/test/Analysis/return-value-guaranteed.cpp:90 + + // no-warning: "The left operand of '==' is a garbage value" was here. + doSomething(); ---------------- Was it? I just tried it out and it doesn't seem to be the case. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63915/new/ https://reviews.llvm.org/D63915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits