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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
