danielmarjamaki added a comment.
In http://reviews.llvm.org/D12359#287522, @rsmith wrote:
> Why does this construct justify the compiler emitting a warning? It seems to
> be reporting a fact about the code rather than a bug, and as there are many
> coding styles where variables are not routinely marked as const whenever
> possible, this appears to be checking that the code conforms to a particular
> coding style. As such, this seems like a better fit as a clang-tidy check
> than as a compiler warning.
>
> The choice to only apply this check to pointer parameters to functions seems
> arbitrary. What is the motivation for that?
I don't want to complain about all variables because as you say there are many
coding styles where variables are not routinely marked as const whenever
possible.
I do complain about pointer parameters because:
- it is common that people want that these are made const
- Imho, these are particularly important; It has a global effect in the program.
I do not complain about normal value parameters because coding style differs
too much. it also makes no difference to the function interface if such
parameter is const or not. It only has local effect.
I do not complain about local function variables because coding style differs
much. As you know it would generate lots of noise in Clangs own code. I assume
that it would generate false positives where variables are nonconst by
intention in most projects if we implement this pedantically.
================
Comment at: lib/Parse/ParseExpr.cpp:176
@@ +175,3 @@
+ if (auto *B = dyn_cast<BinaryOperator>(ER.get())) {
+ if (B->isAssignmentOp() || B->isAdditiveOp()) {
+ MarkNonConstUse(B->getLHS());
----------------
aaron.ballman wrote:
> > basic idea is that p can't be const here:
> ```
> void f(int *p) {
> int *q = p + 1;
> // ...
> }
> ```
> But it could be const here:
> ```
> void f(int *p) {
> const *q = p + 1;
> }
> ```
> I am not certain that addition, by itself, is sufficient to say the use is
> non-const. At the least, this could have some comments explaining the
> rationale with a FIXME.
that is not by intention. There is no nonconst use if lhs is a const pointer. I
will investigate.
================
Comment at: lib/Parse/ParseStmt.cpp:376
@@ -375,3 +375,3 @@
/// \brief Parse an expression statement.
StmtResult Parser::ParseExprStatement() {
// If a case keyword is missing, this is where it should be inserted.
----------------
aaron.ballman wrote:
> > I don't know what this "serialized AST" is that you are talking about. All
> > I know is the -ast-dump and that flag is only intended as a debugging aid
> > as far as I know. If I just run "clang -cc1 -Wnonconst-parameter
> > somefile.c" then it does not serialize does it? So what flags do I use to
> > serialize etc?
>
> Anything using PCH or modules will use a serialized AST.
> http://clang.llvm.org/docs/PCHInternals.html
>
> The basic idea is that these serialize the AST to a file to be later read
> back in with an ASTReader to represent the same semantic state (entirely
> skipping the parser).
Thanks for that info. As far as I see warnings are written when the pch is
generated. but not when the pch is included.
danielm@debian:~$ ~/llvm/build/Debug+Asserts/bin/clang -cc1 test.h
-emit-pch -o test.h.pch
test.h:2:14: warning: parameter 'p' can be const
void ab(int *p) {
^
1 warning generated.
danielm@debian:~$ ~/llvm/build/Debug+Asserts/bin/clang -cc1
-Wnonconst-parameter -include-pch test.h.pch test.c
danielm@debian:~$
Imho, that behaviour is fine.
================
Comment at: lib/Parse/ParseStmt.cpp:960
@@ -956,1 +959,3 @@
R = ParseStatementOrDeclaration(Stmts, false);
+ if (!R.isInvalid() && R.get()) {
+ if (ReturnStmt *RS = dyn_cast<ReturnStmt>(R.get())) {
----------------
aaron.ballman wrote:
> I think you want isUsable() here instead. Or remove the R.get() and use
> dyn_cast_or_null below.
Thanks!
http://reviews.llvm.org/D12359
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
