a.sidorin added a comment.
Hello Gabor,
For me, this patch looks much better than previous. I have some questions
inline.
================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:40
public:
- BugType(class CheckName check, StringRef name, StringRef cat)
- : Check(check), Name(name), Category(cat), SuppressonSink(false) {}
- BugType(const CheckerBase *checker, StringRef name, StringRef cat)
- : Check(checker->getCheckName()), Name(name), Category(cat),
- SuppressonSink(false) {}
- virtual ~BugType() {}
-
- // FIXME: Should these be made strings as well?
- StringRef getName() const { return Name; }
+ BugType(class CheckName Check, StringRef Name, StringRef Cat)
+ : Check(Check), Name(Name), Category(Cat), Checker(nullptr),
----------------
Hm, do we really want to use elaborated name here: `class CheckName`?
================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:50
+ // FIXME: This is a workaround to ensure that Check is initialized
+ // correctly. The name of the checks are set after checker the constructor
+ // is run. In case the BugType object is initialized in the checker's ctor
----------------
Looks like a misspell. Should it be "The check names are set after the
constructors are run"?
================
Comment at: include/clang/StaticAnalyzer/Core/BugReporter/BugType.h:54
+ // initialization.
+ if (getCheckName().empty() && Checker) {
+ Check = Checker->getCheckName();
----------------
Possibly I am missing the context, but could you please explain, why do we
modify CheckName in getName() but not in getCheckName()? It seems a bit unclear
to me.
================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:277
+ BT_leakedvalist.reset(new BugType(
+ CheckNames[ReportUninit ? CK_Uninitialized : CK_Unterminated],
+ "Leaked va_list", categories::MemoryError));
----------------
If I understand correctly, if we report uninitialized and then unterminated,
the second report will have wrong CheckName because it is never reset.
================
Comment at: test/Analysis/malloc.c:1723
-char *dupstrWarn(const char *s) {
- const int len = strlen(s);
----------------
Should we enable the checker instead of removing test?
Repository:
rC Clang
https://reviews.llvm.org/D41538
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
