[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert updated this revision to Diff 202483. aaronpuchert added a comment. Add missing CHECK-NOTs for -Wmissing-prototypes. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59402/new/ https://reviews.llvm.org/D59402 Files:

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert updated this revision to Diff 202480. aaronpuchert added a comment. Remove other change from this one. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59402/new/ https://reviews.llvm.org/D59402 Files: include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert updated this revision to Diff 202479. aaronpuchert added a comment. This revision is now accepted and ready to land. Show note suggesting to add `static`, move fix-it to note for functions, remove fix-it for variables. Show note even for `extern` function definitions when we can't

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert added a comment. I've factored out some changes into D62750 and will rebase this on top if it. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59402/new/ https://reviews.llvm.org/D59402

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert removed a reviewer: bkramer. aaronpuchert added a comment. I discovered an issue with variables, because there can be multiple definitions with the same `TypeLoc`: extern int a; int a, b; We emit the warning for `b`, but we can't just put static in front of the declaration,

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

rsmith added a comment. In D59402#1516589 , @aaronpuchert wrote: > Perhaps we could clarify in the docs that fix-it hints on warnings are > appropriate only if they don't change anything except suppressing the warning. That sounds like a great idea.

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert added a comment. In D59402#1516567 , @aaronpuchert wrote: > If that is wrong, then my reading is that fix-it hints for warnings must > always be applied to a note, because the recovery must be a no-op. Not true, because some warnings

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert marked an inline comment as done. aaronpuchert added a comment. In D59402#1516482 , @rsmith wrote: > In D59402#1516479 , @aaronpuchert > wrote: > > > In D59402#1516432

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

rsmith added a comment. In D59402#1516479 , @aaronpuchert wrote: > In D59402#1516432 , @aaron.ballman > wrote: > > > Also, we're not attempting to recover from the error, which is a good point > > that @thakis

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

rsmith added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11810 + << var + << ((var->getStorageClass() != SC_Extern) + ? FixItHint::CreateInsertion(var->getBeginLoc(), "static ") It would be more appropriate to suppress

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert added a comment. In D59402#1516432 , @aaron.ballman wrote: > Also, we're not attempting to recover from the error, which is a good point > that @thakis raised. aka, if you apply the fix-it, you should also treat the > declaration as though

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaron.ballman added a comment. In D59402#1516421 , @aaronpuchert wrote: > I guess you're referring to "[fix-it hints] should only be used when it’s > very likely they match the user’s intent". Also, we're not attempting to recover from the error,

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert planned changes to this revision. aaronpuchert added a comment. I guess you're referring to "[fix-it hints] should only be used when it’s very likely they match the user’s intent". When turning on the warning on an existing code base, I think that `static` is almost always right.

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

thakis added a comment. Please see https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints , especially the "need to obey these rules" bit. I'm not sure that's fulfilled here. Maybe the fixit should be on a note instead? Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert added a comment. In D59402#1515703 , @aaron.ballman wrote: > My only real concern about this is that sometimes the fix-it will be wrong > because the issue is a typo in the definition. You're right about that, there are multiple reasons

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaron.ballman accepted this revision. aaron.ballman added a comment. My only real concern about this is that sometimes the fix-it will be wrong because the issue is a typo in the definition. However, it seems like that could be handled by preferring typo correction when the edit distance is

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

ed accepted this revision. ed added a comment. This revision is now accepted and ready to land. The patch looks pretty good as far as I can see. That said, I've been out of the loop for quite a while, so do take my approval with a grain of salt. Thanks for working on this! \o/ Repository:

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert added a reviewer: ed. aaronpuchert added a comment. Another ping. I've tried to add the original authors as reviewers, but both warnings are several years old and you might not remember. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59402/new/

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert edited reviewers, added: riccibruno; removed: efriedma. aaronpuchert removed a subscriber: riccibruno. aaronpuchert added a comment. Ping? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59402/new/ https://reviews.llvm.org/D59402

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert updated this revision to Diff 190966. aaronpuchert added a comment. Don't suggest adding `static` if there is a non-prototype declaration. This required a minor refactoring: we let `ShouldWarnAboutMissingPrototype` return any kind of declaration it finds and check for the number of

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert added inline comments. Comment at: test/Sema/warn-missing-prototypes.c:7 int f(int x) { return x; } // expected-warning{{no previous prototype for function 'f'}} +// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static " Maybe there

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

aaronpuchert created this revision. aaronpuchert added reviewers: bkramer, efriedma, rsmith. Herald added subscribers: cfe-commits, jdoerfert. Herald added a project: clang. I've found that most often the proper way to fix this warning is to add `static`, because if the code otherwise compiles