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