The code looks reasonable to me, but others on cfe-commits know Sema better
than I.
clang generally has a high bar for adding warnings. To evaluate a warning, we
usually run it on some large code base and measure true and false positives and
then compare the severity of the true positives with the noise from the false
positives. Do you have any data on what the warning warns on, how serious the
bugs it finds are, and if there are any common patterns this warns on but
shouldn't?
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:435
@@ -432,1 +434,3 @@
+ "%select{destination|source}2; expected %3">,
+ InGroup<SizeofPointerMemaccess>;
def warn_strlcpycat_wrong_size : Warning<
----------------
I wonder if this should have its own flag instead of going into the existing
group – else this might be annoying for people who have the current version of
this warning enabled. I'm not sure though. (Do others have opinions?)
================
Comment at: lib/Sema/SemaChecking.cpp:4741
@@ +4740,3 @@
+
+static bool typesAreCompatibleIgnoreQualifiers(ASTContext &Context,
+ QualType &LHSType,
----------------
It feels like we should have a function for this already…I couldn't find one
either though.
================
Comment at: lib/Sema/SemaChecking.cpp:4758
@@ +4757,3 @@
+ QualType &EltType) {
+ QualType LHSType =
+ Context.getCanonicalType(SizeOfArgFactorTy).getUnqualifiedType();
----------------
clang uses a 2 space indent. Please run clang-format on your patch.
================
Comment at: lib/Sema/SemaChecking.cpp:4916
@@ +4915,3 @@
+ << EltType << Dest->getSourceRange()
+ << LenExpr->getSourceRange());
+ }
----------------
Should this also emit a "cast to (void*) to suppress warning" fixit like other
warnings here do?
If the dest is an array with a known size, and the literal is the size of the
array, should we suggest sizeof(variablename) instead?
================
Comment at: test/SemaCXX/warn-memset-bad-sizeof.cpp:4
@@ +3,3 @@
+
+#include <stddef.h>
+
----------------
Tests should generally be standalone and not include any headers. What do you
need this for? size_t? If so, do `typedef __SIZE_TYPE__ size_t;` instead.
http://reviews.llvm.org/D8881
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
