danielmarjamaki marked 9 inline comments as done.
danielmarjamaki added a comment.
I will remove test/SemaCXX/warn-nonconst-parameter.cpp in the next patch since
this checker does not handle C++ yet.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+ InGroup<NonConstParameter>, DefaultIgnore;
def warn_unused_variable : Warning<"unused variable %0">,
----------------
aaron.ballman wrote:
> Off-by-default warnings aren't something we usually want to add to the
> frontend. They add to the cost of every compile, and are rarely enabled by
> users. From what I understand (and I could be wrong), we generally only add
> DefaultIgnore diagnostics if we are emulating GCC behavior.
ok. I personally prefer to only get serious errors by default. I believe people
should use -Weverything and then -Wno-.. to turn off noise. But I do want to
follow the Clang best practices here so I fix this.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:201
@@ -200,1 +200,3 @@
+def warn_nonconst_parameter : Warning<"parameter %0 can be const">,
+ InGroup<NonConstParameter>, DefaultIgnore;
def warn_unused_variable : Warning<"unused variable %0">,
----------------
danielmarjamaki wrote:
> aaron.ballman wrote:
> > Off-by-default warnings aren't something we usually want to add to the
> > frontend. They add to the cost of every compile, and are rarely enabled by
> > users. From what I understand (and I could be wrong), we generally only add
> > DefaultIgnore diagnostics if we are emulating GCC behavior.
> ok. I personally prefer to only get serious errors by default. I believe
> people should use -Weverything and then -Wno-.. to turn off noise. But I do
> want to follow the Clang best practices here so I fix this.
> and are rarely enabled by users
I disagree about this. Normal usage is to enable as much warnings as you can.
Is it possible for you to show a document, discussion or something that backs
your claim?
When I make it on by default lots of testcases fail. So before fixing all this
I'd prefer to know that I don't waste my time.
================
Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+ if (auto *B = dyn_cast<BinaryOperator>(ER.get())) {
+ if (B->isAssignmentOp() || B->isAdditiveOp()) {
+ MarkWritten(B->getLHS());
----------------
aaron.ballman wrote:
> I do not understand why addition is included here.
basic idea is that p can't be const here:
void f(int *p) {
int *q = p + 1;
// ...
}
================
Comment at: lib/Parse/ParseExpr.cpp:181
@@ +180,3 @@
+ } else if (isa<CallExpr>(ER.get()) ||
+ isa<ConditionalOperator>(ER.get()) ||
+ isa<UnaryOperator>(ER.get())) {
----------------
aaron.ballman wrote:
> Or why a conditional expression is included along with unary operators.
This "conditional expression" check ensures that dontwarn9 does not generate
FP:
char *dontwarn9(char *p) {
char *x;
return p ? p : "";
}
The "unary operator" check ensures that dontwarn11 does not generate FP:
char dontwarn11(int *p) {
return ++(*p);
}
================
Comment at: lib/Parse/ParseExprCXX.cpp:2831
@@ -2830,1 +2830,3 @@
+ MarkWritten(Operand.get());
+
----------------
aaron.ballman wrote:
> Why does this count as a write? Also, if you are not including support for
> C++ yet, perhaps this should be removed regardless.
ok I remove it. It will be needed later if we want to add support for C++. I've
tried to add support for C++ but had problems with templates I failed to fix.
================
Comment at: lib/Parse/ParseStmt.cpp:376
@@ +375,3 @@
+// Mark symbols in r-value expression as written.
+void Parser::MarkWritten(Expr *E) {
+ E = E->IgnoreParenCasts();
----------------
This is called from the Parser only.
================
Comment at: lib/Sema/SemaDecl.cpp:11028
@@ -10972,1 +11027,3 @@
DiagnoseUnusedParameters(FD->param_begin(), FD->param_end());
+ if (!getLangOpts().CPlusPlus)
+ DiagnoseNonConstPointerParameters(FD->param_begin(), FD->param_end());
----------------
no. but the c++ check means that is not a problem now.
================
Comment at: lib/Sema/SemaDecl.cpp:8934
@@ -8933,1 +8933,3 @@
+static void MarkWritten(Expr *E) {
+ E = E->IgnoreParenCasts();
----------------
yes good points.. figuring out good names are always hard.
================
Comment at: test/Sema/warn-nonconst-parameter.c:55
@@ +54,3 @@
+}
+
+void dontwarn7(int *p) {
----------------
yes I only warn about parameters currently.
http://reviews.llvm.org/D12359
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
