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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.


================
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.
----------------
erichkeane wrote:
> 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.
On the one hand, yes. On the other hand, this is only for people who specify `-std=c89` explicitly (which is not the default language mode for C), and I think we've been taking the viewpoint that these folks are mostly trying to keep old, mostly unmaintained code working.


================
Comment at: clang/include/clang/Basic/LangOptions.h:534
+  /// Returns true if implicit int is supported at all.
+  bool implicitIntEnabled() const { return !CPlusPlus && !C2x; }
+
----------------
erichkeane wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > 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`.
> > @erichkeane `requiresImplicitInt` is only used twice. Does it needs a name?
> > 
> *shrug*, I tend to be of the feeling that if you have to say something this often, and functions are basically free, mind as well make one.
The idea here is that `requiresImplicitInt()` tells you when the support is mandatory per spec, and `implicitIntEnabled()` tells you when the support is either mandatory or an extension. I'm not strongly tied to the names, how do these sound?

`isImplicitIntRequired()` and `isImplicitIntAllowed()`?

(I could also add the word `Support` in there as in `isImplicitIntSupportRequired()` but then the function names start to get a bit longer than I think is reasonable.)


================
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
----------------
erichkeane wrote:
> C90?
I'll fix the comment up. This confusion exists all over the place. :-D


================
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;
----------------
erichkeane wrote:
> cor3ntin wrote:
> > erichkeane wrote:
> > > IMO there should be a warning here for C89.  Warning for non-future-proof code is pretty typical.
> > Isn't the counter argument that was made on similar changes that people specifically asking for C89 have peculiar expectations?
> > Otherwise i agree
> Yep, folks asking for C89 ARE peculiar :D  However, warnings-not-as-error are, IMO, simply 'helpful'. 
Same here as above -- we *could* warn, but I suspect it wouldn't be particularly helpful. The fact that we were warning with an extension warning was non-conforming though (we shouldn't fail `-pedantic-errors` in C89 over that).


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14342-14343
         DeclSpec DS(attrs);
-        const char* PrevSpec; // unused
-        unsigned DiagID; // unused
+        const char *PrevSpec; // unused
+        unsigned DiagID;      // unused
         DS.SetTypeSpecType(DeclSpec::TST_int, FTI.Params[i].IdentLoc, PrevSpec,
----------------
erichkeane wrote:
> cor3ntin wrote:
> > Nitpick: whitespace only change
> This might be a clang-format thing based on the previous line.
Yeah, clang-format got a bit aggressive there, I'll be the formatting changes out, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124258