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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 9 08:25:00 PDT 2022


aaron.ballman added inline comments.


================
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)'}}
----------------
erichkeane wrote:
> 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. 
> Aren't all those identifiers already stolen by the standard? They are reserved already, right?

Yes, they are. However, the situation we should have sympathy for is a user who is compiling in C17 mode and using an identifier like `ckd_add` suddenly having that identifier stolen out from under them because C2x added that as a library function and we decided we wanted it to be a builtin for some reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131499



More information about the cfe-commits mailing list