[PATCH] D124258: [C89/C2x] Change the behavior of implicit int diagnostics

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Some quick comments... I didn't even bother opening the tests besides checking on functionality, so hopefully those are ok too :D



================
Comment at: clang/docs/ReleaseNotes.rst:169
+  As of C2x, support for implicit int has been removed, and the warning options
+  will have no effect. Specifying ``-Wimplicit-int`` in C89 mode will now issue
+  warnings instead of being a noop.
----------------
I find myself thinking that despite it being 'valid code' that `-Wimplicit-int` should be on-by-default in C89 mode. We often warn about 'valid, but non-future-proof' code, by default, particularly when they are code-breaking like this is.


================
Comment at: clang/include/clang/Basic/LangOptions.h:534
+  /// Returns true if implicit int is supported at all.
+  bool implicitIntEnabled() const { return !CPlusPlus && !C2x; }
+
----------------
This name seems inverse of what you mean to me?  IDK if you're bike-shed-sniping me here, but:

`prohibitsImplicitInt` to be the reverse of above? It becomes SLIGHTLY ambiguous to me (in that we don't mean "the language standard prohibits", we mean "the compiler implementation prohibits"), but that can be fixed up with a comment.

If you want to keep the direction, perhaps `implicitIntPermitted`, at which point I might suggest the one above become `implicitIntRequired`.


================
Comment at: clang/lib/Parse/Parser.cpp:1198
 
   // If this is C90 and the declspecs were completely missing, fudge in an
   // implicit int.  We do this here because this is the only place where
----------------
C90?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14329
       if (FTI.Params[i].Param == nullptr) {
-        SmallString<256> Code;
-        llvm::raw_svector_ostream(Code)
-            << "  int " << FTI.Params[i].Ident->getName() << ";\n";
-        Diag(FTI.Params[i].IdentLoc, diag::ext_param_not_declared)
-            << FTI.Params[i].Ident
-            << FixItHint::CreateInsertion(LocAfterDecls, Code);
+        if (getLangOpts().C99) {
+          SmallString<256> Code;
----------------
IMO there should be a warning here for C89.  Warning for non-future-proof code is pretty typical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124258