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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

In D59402#1516482 <https://reviews.llvm.org/D59402#1516482>, @rsmith wrote:

> In D59402#1516479 <https://reviews.llvm.org/D59402#1516479>, @aaronpuchert wrote:
>
> > In D59402#1516432 <https://reviews.llvm.org/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 it were declared `static`.
> >
> >
> > I think the recovery rule only applies to errors, there is no need to recover from a warning.
>
>
> That is incorrect, because `-Werror` can be used to promote warnings to errors. Please see https://clang.llvm.org/docs/InternalsManual.html#fix-it-hints for the rule and rationale; if it's insufficiently clear on this, we should fix it.


Ok, but can't we generally recover from warnings with a no-op, because it's still valid C++? Neither parser nor the rest of Sema should be held up by this. Don't we in fact have to recover with a no-op to be compliant with the standard?

So my understanding of the rules is that the recovery rule only applies to "actual" errors, which would prevent continuing if we didn't recover. 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. Otherwise the rules are pretty clear, except that perhaps "when it’s very likely they match the user’s intent" is open to interpretation. But I think we agreed here that it is not "very likely", so it should be a note.



================
Comment at: lib/Sema/SemaDecl.cpp:13225-13230
           if (FunctionNoProtoTypeLoc FTL = TL.getAs<FunctionNoProtoTypeLoc>())
-            Diag(PossibleZeroParamPrototype->getLocation(),
+            Diag(PossiblePrototype->getLocation(),
                  diag::note_declaration_not_a_prototype)
-                << PossibleZeroParamPrototype
+                << PossiblePrototype
                 << FixItHint::CreateInsertion(FTL.getRParenLoc(), "void");
         }
----------------
rsmith wrote:
> If we produce this note, we should not also produce the "add static" suggestion.
They are exclusive because we suggest `static` only if `PossiblePrototype` is false, and this note only when it's true. But you're right that this isn't obvious, I'll make it an if/else.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59402/new/

https://reviews.llvm.org/D59402