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


================
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;
----------------
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'. 


================
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,
----------------
cor3ntin wrote:
> Nitpick: whitespace only change
This might be a clang-format thing based on the previous line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124258