[PATCH] D131499: Change prototype merging error into a warning for builtins

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Agree with backporting to Clang15, that'll break the fewest users.



================
Comment at: clang/test/Sema/prototype-redecls.c:32
 // clear that the previous declaration was a builtin.
-float rintf() { // expected-error {{conflicting types for 'rintf'}} \
+float rintf() { // expected-warning {{incompatible redeclaration of library function 'rintf'}} \
                    expected-note {{'rintf' is a builtin with type 'float (float)'}}
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > Hmm... this is a definition of a builtin with a completely incompatible prototype.  Do we REALLY want this to not be an error?  
> > 
> > I guess I could see it being OK with declarations, but it is odd with a definition that is incompatible.  
> I think we do want it to not be an error. The user has no choice in whether these are predeclared or not, which steals identifiers from users. By giving the user a warning, they're alerted to the fact they're doing something that may be highly confusing, so I agree we definitely want a diagnostic. If we give the user an error, they're stuck. Also, it's even weirder that:
> ```
> float rintf(void) {} // Always a warning, is fine
> float rintf() {} // Error pre C2x, but fine in C2x
> ```
Aren't all those identifiers already stolen by the standard?  They are reserved already, right?  

But that behavior you post there is unfortunate... I guess leaving these as warnings is important so they get suppressed in headers (for cases where a C-library 'implements' it anyway, despite it being a builtin).

I think I can live with this, and I think backporting it is the least breaking way to do this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131499