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

[Erich Keane via Phabricator via cfe-commits]

erichkeane 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.
----------------
aaron.ballman wrote:
> 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.
I'm annoyed by how much I understand/agree with both sides of this argument.  


================
Comment at: clang/include/clang/Basic/LangOptions.h:534
+  /// Returns true if implicit int is supported at all.
+  bool implicitIntEnabled() const { return !CPlusPlus && !C2x; }
+
----------------
aaron.ballman wrote:
> 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.)
>> 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?

Well, as it is now, the latter tells you whether 'it is allowed as an extension'.


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

These seem pretty clear to me.  I don't see 'Support' as being valuable.



================
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;
----------------
aaron.ballman wrote:
> 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).
*grumble grumble*.


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

https://reviews.llvm.org/D124258