subject:"\[PATCH\] D122895\: \[C89\/C2x\] Improve diagnostics around strict prototypes in C"

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-17 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added a comment.

In D122895#3511855 , @aaron.ballman 
wrote:

>> (It also seems unfortunate to regress the false positive rate of this 
>> diagnostic before `-fstrict-prototypes` is available.)
>
> `-fno-knr-functions` is available already today in trunk, so I'm not certain 
> I understand your concern there (or were you unaware we changed the flag name 
> perhaps?).

That's right, I missed it. Thanks for pointing it out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-17 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3512314 , @aaron.ballman 
wrote:

> In D122895#3511682 , @aaron.ballman 
> wrote:
>
>> In D122895#3511632 , @jyknight 
>> wrote:
>>
>>> The warnings for this case aren't great:
>>>
>>>   int foo();
>>>   
>>>   int
>>>   foo(int arg) {
>>> return 5;
>>>   }
>>
>> Yeah, that's not ideal, I'm looking into it to see if I can improve that 
>> scenario or not.
>
> There's not much to be done about the strict prototype warning for the 
> declaration; the issue is that we need to give the strict prototypes warning 
> when forming the function *type* for the declaration, which is long before we 
> have any idea there's a subsequent definition (also, because this is for 
> forming the type, we don't know what declarations, if any, may be related, so 
> there's no real way to improve the fix-it behavior). However, I think the 
> second warning is just bad wording -- we know we have a function definition 
> with a prototype for this particular diagnostic. However, there's the 
> redeclaration case to consider at the same time. So here's the full test and 
> the best behavior that I can come up with so far:
>
>   void foo();
>   void foo(int arg);
>   
>   void bar();
>   void bar(int arg) {}
>
> With `-Wstrict-prototypes` enabled:
>
>   F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only 
> -Wstrict-prototypes "C:\Users\aballman\OneDrive - Intel 
> Corporation\Desktop\test.c"
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:9: warning: 
> a function declaration without a prototype
> is deprecated in all versions of C [-Wstrict-prototypes]
>   void foo();
>   ^
>void
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: 
> this function declaration with a prototype
> will change behavior in C2x because of a previous declaration with no 
> prototype [-Wdeprecated-non-prototype]
>   void foo(int arg);
>^
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:9: warning: 
> a function declaration without a prototype
> is deprecated in all versions of C [-Wstrict-prototypes]
>   void bar();
>   ^
>void
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: 
> this function definition with a prototype
> will change behavior in C2x because of a previous declaration with no 
> prototype [-Wdeprecated-non-prototype]
>   void bar(int arg) {}
>^
>   4 warnings generated.
>
> With `-Wstrict-prototypes` disabled:
>
>   F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only 
> "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c"
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:6: warning: 
> a function declaration without a prototype
> is deprecated in all versions of C and is not supported in C2x 
> [-Wdeprecated-non-prototype]
>   void foo();
>^
>void
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: 
> this function declaration with a prototype
> will change behavior in C2x because of a previous declaration with no 
> prototype [-Wdeprecated-non-prototype]
>   void foo(int arg);
>^
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:6: warning: 
> a function declaration without a prototype
> is deprecated in all versions of C and is not supported in C2x 
> [-Wdeprecated-non-prototype]
>   void bar();
>^
>void
>   C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: 
> this function definition with a prototype
> will change behavior in C2x because of a previous declaration with no 
> prototype [-Wdeprecated-non-prototype]
>   void bar(int arg) {}
>^
>   4 warnings generated.
>
> It's not ideal, but it's about the most workable solution I can come up with 
> that doesn't regress far more important cases. It'd be nice if we had a note 
> instead of leaning on the previous warning like we're doing, but I still 
> claim this is defensible given how rare the situation is that you'd declare 
> without a prototype and later redeclare (perhaps through a definition) with a 
> non-`void` prototype. Note (it's easy to miss with the wall of text), the 
> difference between the two runs is that when strict prototypes are enabled, 
> we issue the strict prototype warning on the first declaration and when 
> strict prototypes are disabled, we issue the "is not supported in C2x" 
> diagnostic on the first declaration -- but in either case the intention is to 
> alert the user to which previous declaration has no prototype for the 
> subsequent declaration/definition that caused the issue.
>
> `this function declaration|definition with a prototype will change behavior 
> in C2x

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3512341 , @aaron.ballman 
wrote:

> I think the current behavior today makes sense but we should see if we can 
> improve it to make *more* sense. With `-Wstrict-prototypes`, we should 
> complain about the block without a prototype, but the use at the call site is 
> not declaring a conflicting declaration nor is it a call to a function 
> without a prototype but passes arguments (both of those are 
> `-Wdeprecated-non-prototype` warnings). Instead, it's a new kind of situation 
> that we may want to consider adding additional coverage for under 
> `-Wdeprecated-non-prototype` based on the same reasoning we used for 
> diagnosing calls to a function without a prototype but pass arguments. This 
> is not specific to blocks, consider:
>
>   void func(void (*fp)());
>   void do_it(int i);
>   
>   int main(void) {
> func(do_it); // It'd be nice to diagnose this too, but we don't today
>   }

Mostly as a note to myself (but others can definitely chime in if they think 
I'm wrong here), I *think* the correct way to handle both blocks and function 
pointers is to add new `AssignConvertType` enumerators for 
`IncompatibleC2xFunctionPointer` and `IncompatibleC2xBlockPointer` to represent 
an assignment conversion that is compatible today but will be incompatible in 
C2x. Then functions like `checkPointerTypesForAssignment()` and 
`checkBlockPointerTypesForAssignment()` can return the new value, which 
`DiagnoseAssignmentResult()` can use to determine whether to diagnose a 
potential behavior change or not.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3511855 , @aaron.ballman 
wrote:

> In D122895#3511646 , @dexonsmith 
> wrote:
>
>>   void f1(void (^block)());
>>   
>>   void f2(void) {
>> f1(^(int x) { /* do something with x */ });
>>   }
>
> Your code example is interesting though as I would expect that to trigger 
> `-Wdeprecated-non-prototype` as well as `-Wstrict-prototypes`. I'd expect the 
> pedantic warning to complain about the block declaration because it specifies 
> no prototype, and I'd expect the changes behavior warning because that code 
> will break in C2x.

I think the current behavior today makes sense but we should see if we can 
improve it to make *more* sense. With `-Wstrict-prototypes`, we should complain 
about the block without a prototype, but the use at the call site is not 
declaring a conflicting declaration nor is it a call to a function without a 
prototype but passes arguments (both of those are `-Wdeprecated-non-prototype` 
warnings). Instead, it's a new kind of situation that we may want to consider 
adding additional coverage for under `-Wdeprecated-non-prototype` based on the 
same reasoning we used for diagnosing calls to a function without a prototype 
but pass arguments. This is not specific to blocks, consider:

  void func(void (*fp)());
  void do_it(int i);

  int main(void) {
func(do_it); // It'd be nice to diagnose this too, but we don't today
  }

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3511682 , @aaron.ballman 
wrote:

> In D122895#3511632 , @jyknight 
> wrote:
>
>> The warnings for this case aren't great:
>>
>>   int foo();
>>   
>>   int
>>   foo(int arg) {
>> return 5;
>>   }
>
> Yeah, that's not ideal, I'm looking into it to see if I can improve that 
> scenario or not.

There's not much to be done about the strict prototype warning for the 
declaration; the issue is that we need to give the strict prototypes warning 
when forming the function *type* for the declaration, which is long before we 
have any idea there's a subsequent definition (also, because this is for 
forming the type, we don't know what declarations, if any, may be related, so 
there's no real way to improve the fix-it behavior). However, I think the 
second warning is just bad wording -- we know we have a function definition 
with a prototype for this particular diagnostic. However, there's the 
redeclaration case to consider at the same time. So here's the full test and 
the best behavior that I can come up with so far:

  void foo();
  void foo(int arg);

  void bar();
  void bar(int arg) {}

With `-Wstrict-prototypes` enabled:

  F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only 
-Wstrict-prototypes "C:\Users\aballman\OneDrive - Intel 
Corporation\Desktop\test.c"
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:9: warning: a 
function declaration without a prototype
is deprecated in all versions of C [-Wstrict-prototypes]
  void foo();
  ^
   void
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: 
this function declaration with a prototype
will change behavior in C2x because of a previous declaration with no 
prototype [-Wdeprecated-non-prototype]
  void foo(int arg);
   ^
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:9: warning: a 
function declaration without a prototype
is deprecated in all versions of C [-Wstrict-prototypes]
  void bar();
  ^
   void
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: 
this function definition with a prototype
will change behavior in C2x because of a previous declaration with no 
prototype [-Wdeprecated-non-prototype]
  void bar(int arg) {}
   ^
  4 warnings generated.

With `-Wstrict-prototypes` disabled:

  F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only 
"C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c"
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:1:6: warning: a 
function declaration without a prototype
is deprecated in all versions of C and is not supported in C2x 
[-Wdeprecated-non-prototype]
  void foo();
   ^
   void
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:6: warning: 
this function declaration with a prototype
will change behavior in C2x because of a previous declaration with no 
prototype [-Wdeprecated-non-prototype]
  void foo(int arg);
   ^
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:4:6: warning: a 
function declaration without a prototype
is deprecated in all versions of C and is not supported in C2x 
[-Wdeprecated-non-prototype]
  void bar();
   ^
   void
  C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:5:6: warning: 
this function definition with a prototype
will change behavior in C2x because of a previous declaration with no 
prototype [-Wdeprecated-non-prototype]
  void bar(int arg) {}
   ^
  4 warnings generated.

It's not ideal, but it's about the most workable solution I can come up with 
that doesn't regress far more important cases. It'd be nice if we had a note 
instead of leaning on the previous warning like we're doing, but I still claim 
this is defensible given how rare the situation is that you'd declare without a 
prototype and later redeclare (perhaps through a definition) with a non-`void` 
prototype. Note (it's easy to miss with the wall of text), the difference 
between the two runs is that when strict prototypes are enabled, we issue the 
strict prototype warning on the first declaration and when strict prototypes 
are disabled, we issue the "is not supported in C2x" diagnostic on the first 
declaration -- but in either case the intention is to alert the user to which 
previous declaration has no prototype for the subsequent declaration/definition 
that caused the issue.

`this function declaration|definition with a prototype will change behavior in 
C2x because of a previous declaration with no prototype` is the new diagnostic 
wording I've got so far, and I'm not strongly tied to it, if you have 
suggestions to improve it. I would also be happy with `this function 
declaration|definition with a prototype is not supported in C2x because of

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3511646 , @dexonsmith 
wrote:

> In D122895#3511611 , @dexonsmith 
> wrote:
>
>> Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in 
>> code.
>
> For example, it's important to have a warning that catches code like this:
>
>   void f1(void (^block)());
>   
>   void f2(void) {
> f1(^(int x) { /* do something with x */ });
>   }
>
> without triggering in cases that are pedantic.
>
> (It also seems unfortunate to regress the false positive rate of this 
> diagnostic before `-fstrict-prototypes` is available.)

`-fno-knr-functions` is available already today in trunk, so I'm not certain I 
understand your concern there (or were you unaware we changed the flag name 
perhaps?).

Your code example is interesting though as I would expect that to trigger 
`-Wdeprecated-non-prototype` as well as `-Wstrict-prototypes`. I'd expect the 
pedantic warning to complain about the block declaration because it specifies 
no prototype, and I'd expect the changes behavior warning because that code 
will break in C2x.

> Is it necessary to make -Wstrict-prototypes weaker in order to move the newer 
> more pedantic cases to a different name?

This diagnostic is *extremely* difficult for a whole host of reasons. 1) we 
need to diagnose function *types* without a prototype, but you form a function 
type when declaring or defining a function in addition to just using the type 
in a typedef or a variable declaration. 2) we need to handle function 
definitions differently; that's the only point at which we know whether the 
user is defining a function with an identifier list which will change behavior 
in C2x, 3) we need to handle function declaration merging (which happens for 
definitions as well as redeclarations). So there's this very convoluted dance 
where some cases are handled in `Sema::GetFullTypeForDeclarator()` (this is the 
only place we trigger the pedantic warning from), some are handled in 
`Sema::ActOnFinishFunctionBody()`, and some are handled in 
`Sema::MergeFunctionDecl()`. Trying to weaken `-Wstrict-prototypes` 
specifically means having to make the decision at the time when the only 
information we have is what we can glean from the declarator.

> Oh, I thought that would catch bugs like this:
>
>   void longfunctionname(){}
>   void longfunctionnames(int x){ /* do something with x */ }
>   
>   void g(void) {
> longfunctionname(7); // oops, meant to call longfunctionnames().
>   }
>
> but if it's entirely pedantic (if the call to longfunctionname(7) would fail 
> to compile) then I agree it'd be better to silence it unless someone asks for 
> -pedantic.

The call to `longfunctionname(7)` would be rejected in C2x and accepted in 
earlier modes, which is why it gets flagged `warning: passing arguments to 
'longfunctionname' without a prototype is deprecated in all versions of C and 
is not supported in C2x`. It's worth noting that `-Wstrict-prototypes` is now 
in `-pedantic` explicitly (it shifted from being a `Warning` to an `Extension` 
because it is now a pedantic warning).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added a comment.

In D122895#3511649 , @aaron.ballman 
wrote:

> In D122895#3511611 , @dexonsmith 
> wrote:
>
>> Sure, I'm all for adding a new warning for users that want a pedantic 
>> warning. Can it be put behind a separate flag, such as 
>> `-Wstrict-prototypes-pedantic`, which isn't triggered by 
>> `-Wstrict-prototypes`?
>>
>> Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in 
>> code.
>
> Doing that would then make `-Wstrict-prototypes` effectively a no-op (it 
> would still control `-Wdeprecated-non-prototype` I suppose?),

Is it necessary to make `-Wstrict-prototypes` weaker in order to move the newer 
more pedantic cases to a different name?

> but there were also people who enabled `-Wstrict-prototypes` because they 
> wanted the pedantic aspects of the warning in cases where it was firing, and 
> those folks would then be losing warning coverage without knowing it. For 
> example:
>
>   void f(){}
>
> In prior versions of Clang with `-Wstrict-prototypes` this would issue a 
> `this old-style function definition is not preceded by a prototype` 
> diagnostic, but would now be silenced entirely unless the user knew to turn 
> on a different flag.

Oh, I thought that would catch bugs like this:

  void longfunctionname(){}
  void longfunctionnames(int x){ /* do something with x */ }
  
  void g(void) {
longfunctionname(7); // oops, meant to call longfunctionnames().
  }

but if it's entirely pedantic (if the call to `longfunctionname(7)` would fail 
to compile) then I agree it'd be better to silence it unless someone asks for 
`-pedantic`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3511632 , @jyknight wrote:

> The warnings for this case aren't great:
>
>   int foo();
>   
>   int
>   foo(int arg) {
> return 5;
>   }

Yeah, that's not ideal, I'm looking into it to see if I can improve that 
scenario or not. Thankfully, IMO, such code is likely to be extremely rare. 
Most people writing that declaration either expect an arbitrary number of args 
(they really don't want the prototype) and so the definition is extremely 
suspect that it only accepts one, or they wrote the declaration assuming it 
would accept *no* args (the C++ behavior) and then the definition becomes 
flat-out a bug. Do you have evidence this code pattern appears with some 
frequency?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3511611 , @dexonsmith 
wrote:

> Sure, I'm all for adding a new warning for users that want a pedantic 
> warning. Can it be put behind a separate flag, such as 
> `-Wstrict-prototypes-pedantic`, which isn't triggered by 
> `-Wstrict-prototypes`?
>
> Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in 
> code.

Doing that would then make `-Wstrict-prototypes` effectively a no-op (it would 
still control `-Wdeprecated-non-prototype` I suppose?), but there were also 
people who enabled `-Wstrict-prototypes` because they wanted the pedantic 
aspects of the warning in cases where it was firing, and those folks would then 
be losing warning coverage without knowing it. For example:

  void f(){}

In prior versions of Clang with `-Wstrict-prototypes` this would issue a `this 
old-style function definition is not preceded by a prototype` diagnostic, but 
would now be silenced entirely unless the user knew to turn on a different flag.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added a comment.

In D122895#3511611 , @dexonsmith 
wrote:

> Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in 
> code.

For example, it's important to have a warning that catches code like this:

  void f1(void (^block)());
  
  void f2(void) {
f1(^(int x) { /* do something with x */ });
  }

without triggering in cases that are pedantic.

(It also seems unfortunate to regress the false positive rate of this 
diagnostic before `-fstrict-prototypes` is available.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread James Y Knight via Phabricator via cfe-commits

jyknight added a comment.

The warnings for this case aren't great:

  int foo();
  
  int
  foo(int arg) {
return 5;
  }

results in:

  /tmp/test.c:1:5: warning: a function declaration without a prototype is 
deprecated in all versions of C and is not supported in C2x 
[-Wdeprecated-non-prototype]
  int foo();
  ^
  void
  /tmp/test.c:4:1: warning: a function declaration without a prototype is 
deprecated in all versions of C and is not supported in C2x 
[-Wdeprecated-non-prototype]
  foo(int arg) {
  ^

Two warnings for the problem, instead of a warning and a note, plus, the fix-it 
hint is incorrect, which the compiler should know, since we see that there are 
arguments expected.

I'd expect more like:

  /tmp/test.c:1:5: warning: a function declaration without a prototype is 
deprecated in all versions of C and is not supported in C2x 
[-Wdeprecated-non-prototype]
  int foo();
  /tmp/test.c:4:1: note: subsequent declaration specifies arguments.
  foo(int arg) {
  ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added a subscriber: arphaman.
dexonsmith added a comment.

In D122895#3511376 , @aaron.ballman 
wrote:

> In D122895#3511312 , @aaron.ballman 
> wrote:
>
>> However, I think the blocks behavior is a bug based on this: 
>> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728
>>  and I'd be more than happy to fix that, as I'm not checking that condition 
>> here: 
>> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579
>>  which seems like a pretty obvious thing to be checking for before warning 
>> about not having a strict prototype.
>
> I fixed this false positive bug in 4be105c98a9c7e083cd878ee1751e11160b97b4a 
> , so 
> blocks (and OpenCL) behavior should now be improved.

Thanks! I think that's the bigger issue of the two. @steven_wu or @arphaman 
will have more context though.

In D122895#3511312 , @aaron.ballman 
wrote:

> In D122895#3510165 , @dexonsmith 
> wrote:
>
>> For additional context to my questions above, even though open source clang 
>> hasn't been using `-Wstrict-prototypes`, Xcode has had it on-by-default in 
>> new projects since sometime in 2017, with project modernizations to turn it 
>> on for old projects.
>
> Thanks, that's very good to know! And also, thank you for raising the 
> questions here, I appreciate the discussion.
>
>> Warning on block and function definitions such as `^(){}` and `void f1() 
>> {}`, which are pedantically lacking a prototype but the compiler knows there 
>> are exactly zero parameters, would be super noisy for users.
>>
>> Unless I read the RFC too quickly, it doesn't look like it's adding any 
>> value, since these aren't going to change meaning. Is it possible to revert 
>> this part of the change?
>
> `-Wstrict-prototypes` is now a pedantic deprecation warning that fires any 
> time you form a function type which has no prototype, which was discussed in 
> the RFC 
> (https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/38?u=aaronballman):
>
>   Change -Wstrict-prototypes to diagnose functions without a prototype that 
> don’t change behavior in C2x, it remains off-by-default but is automatically 
> enabled by -pedantic as it’s still warning the user about a deprecation.
>
> However, I think the blocks behavior is a bug based on this: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728
>  and I'd be more than happy to fix that, as I'm not checking that condition 
> here: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579
>  which seems like a pretty obvious thing to be checking for before warning 
> about not having a strict prototype.
>
> But I'm pretty insistent on warning about the other case as it does use 
> functions without a prototype and we need *some* blanket warning for use of a 
> deprecated feature for folks who want to be strictly conforming. It sounds 
> like Apple may want to no longer enable `-Wstrict-prototypes` by default as 
> it's shifted to be a more pedantic warning than it was before -- would that 
> be a viable option for you (can you use project modernizations to turn it 
> back off for old projects)?

Sure, I'm all for adding a new warning for users that want a pedantic warning. 
Can it be put behind a separate flag, such as `-Wstrict-prototypes-pedantic`, 
which isn't triggered by `-Wstrict-prototypes`?

Previously, `-Wstrict-prototypes` was useful for preventing actual bugs in code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3511312 , @aaron.ballman 
wrote:

> However, I think the blocks behavior is a bug based on this: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728
>  and I'd be more than happy to fix that, as I'm not checking that condition 
> here: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579
>  which seems like a pretty obvious thing to be checking for before warning 
> about not having a strict prototype.

I fixed this false positive bug in 4be105c98a9c7e083cd878ee1751e11160b97b4a 
, so 
blocks (and OpenCL) behavior should now be improved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-13 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3510165 , @dexonsmith 
wrote:

> For additional context to my questions above, even though open source clang 
> hasn't been using `-Wstrict-prototypes`, Xcode has had it on-by-default in 
> new projects since sometime in 2017, with project modernizations to turn it 
> on for old projects.

Thanks, that's very good to know! And also, thank you for raising the questions 
here, I appreciate the discussion.

> Warning on block and function definitions such as `^(){}` and `void f1() {}`, 
> which are pedantically lacking a prototype but the compiler knows there are 
> exactly zero parameters, would be super noisy for users.
>
> Unless I read the RFC too quickly, it doesn't look like it's adding any 
> value, since these aren't going to change meaning. Is it possible to revert 
> this part of the change?

`-Wstrict-prototypes` is now a pedantic deprecation warning that fires any time 
you form a function type which has no prototype, which was discussed in the RFC 
(https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/38?u=aaronballman):

  Change -Wstrict-prototypes to diagnose functions without a prototype that 
don’t change behavior in C2x, it remains off-by-default but is automatically 
enabled by -pedantic as it’s still warning the user about a deprecation.

However, I think the blocks behavior is a bug based on this: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728 
and I'd be more than happy to fix that, as I'm not checking that condition 
here: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579
 which seems like a pretty obvious thing to be checking for before warning 
about not having a strict prototype.

But I'm pretty insistent on warning about the other case as it does use 
functions without a prototype and we need *some* blanket warning for use of a 
deprecated feature for folks who want to be strictly conforming. It sounds like 
Apple may want to no longer enable `-Wstrict-prototypes` by default as it's 
shifted to be a more pedantic warning than it was before -- would that be a 
viable option for you (can you use project modernizations to turn it back off 
for old projects)?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-12 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added inline comments.



Comment at: clang/test/Sema/warn-strict-prototypes.m:20
+  // know it's a block when diagnosing.
+  void (^block2)(void) = ^void() { // expected-warning {{a function 
declaration without a prototype is deprecated in all versions of C}}
   };

efriedma wrote:
> dexonsmith wrote:
> > dexonsmith wrote:
> > > This is a definition, so the compiler knows that there are no parameters. 
> > > Why would we warn here? Reading 
> > > https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18
> > >  it looks to me like an example of what @rnk was referring to, about 
> > > churning code to add `(void)` and then return back to `()` later.
> > > 
> > > (cc: @steven_wu and @rjmccall as well)
> > Specifically, `^void() { /* anything */}` is the definition of a block with 
> > zero parameters. Maybe pedantically it's lacking a prototype, but the 
> > compiler knows (since this is the definition) how many parameters there are.
> > 
> > (Same goes for `void() { /* anything */ }` at global scope; is that 
> > triggering `-Wstrict-prototypes` now too?)
> To be clear, you're asking specifically about the following two cases where 
> the behavior with -Wstrict-prototypes changed, right?  I don't think this was 
> really discussed in the RFC.
> 
> ```
> // Block
> void (^f1)(void) = ^(){};
> 
> // Function definition
> void f(void);
> void f(){}
> ```
> 
> Note that the behavior of the following did not change:
> 
> ```
> void (^f1)(void) = ^{}; // no warning
> void f2(){} // warning since clang 11.
> ```
Right, specifically those two cases that changed. Thanks for clarifying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-12 Thread Eli Friedman via Phabricator via cfe-commits

efriedma added inline comments.



Comment at: clang/test/Sema/warn-strict-prototypes.m:20
+  // know it's a block when diagnosing.
+  void (^block2)(void) = ^void() { // expected-warning {{a function 
declaration without a prototype is deprecated in all versions of C}}
   };

dexonsmith wrote:
> dexonsmith wrote:
> > This is a definition, so the compiler knows that there are no parameters. 
> > Why would we warn here? Reading 
> > https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18
> >  it looks to me like an example of what @rnk was referring to, about 
> > churning code to add `(void)` and then return back to `()` later.
> > 
> > (cc: @steven_wu and @rjmccall as well)
> Specifically, `^void() { /* anything */}` is the definition of a block with 
> zero parameters. Maybe pedantically it's lacking a prototype, but the 
> compiler knows (since this is the definition) how many parameters there are.
> 
> (Same goes for `void() { /* anything */ }` at global scope; is that 
> triggering `-Wstrict-prototypes` now too?)
To be clear, you're asking specifically about the following two cases where the 
behavior with -Wstrict-prototypes changed, right?  I don't think this was 
really discussed in the RFC.

```
// Block
void (^f1)(void) = ^(){};

// Function definition
void f(void);
void f(){}
```

Note that the behavior of the following did not change:

```
void (^f1)(void) = ^{}; // no warning
void f2(){} // warning since clang 11.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-12 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added a comment.

For additional context to my questions above, even though open source clang 
hasn't been using `-Wstrict-prototypes`, Xcode has had it on-by-default in new 
projects since sometime in 2017, with project modernizations to turn it on for 
old projects.

Warning on block and function definitions such as `^(){}` and `void f1() {}`, 
which are pedantically lacking a prototype but the compiler knows there are 
exactly zero parameters, would be super noisy for users.

Unless I read the RFC too quickly, it doesn't look like it's adding any value, 
since these aren't going to change meaning. Is it possible to revert this part 
of the change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-12 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added inline comments.



Comment at: clang/test/Sema/warn-strict-prototypes.m:20
+  // know it's a block when diagnosing.
+  void (^block2)(void) = ^void() { // expected-warning {{a function 
declaration without a prototype is deprecated in all versions of C}}
   };

dexonsmith wrote:
> This is a definition, so the compiler knows that there are no parameters. Why 
> would we warn here? Reading 
> https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18
>  it looks to me like an example of what @rnk was referring to, about churning 
> code to add `(void)` and then return back to `()` later.
> 
> (cc: @steven_wu and @rjmccall as well)
Specifically, `^void() { /* anything */}` is the definition of a block with 
zero parameters. Maybe pedantically it's lacking a prototype, but the compiler 
knows (since this is the definition) how many parameters there are.

(Same goes for `void() { /* anything */ }` at global scope; is that triggering 
`-Wstrict-prototypes` now too?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-05-12 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added subscribers: steven_wu, rjmccall, rnk, dexonsmith.
dexonsmith added inline comments.



Comment at: clang/test/Sema/warn-strict-prototypes.m:20
+  // know it's a block when diagnosing.
+  void (^block2)(void) = ^void() { // expected-warning {{a function 
declaration without a prototype is deprecated in all versions of C}}
   };

This is a definition, so the compiler knows that there are no parameters. Why 
would we warn here? Reading 
https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c/60521/18
 it looks to me like an example of what @rnk was referring to, about churning 
code to add `(void)` and then return back to `()` later.

(cc: @steven_wu and @rjmccall as well)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-30 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3484097 , @aaron.ballman 
wrote:

> In D122895#3484076 , @manojgupta 
> wrote:
>
>> Tried locally but I still see the warning with -fno-knr-functions. It also 
>> says that the argument is unused.
>>
>> bin/clang --version
>> clang version 15.0.0 (https://github.com/llvm/llvm-project.git 
>> a9d68a5524dea113cace5983697786599cbdce9a 
>> )
>> Target: x86_64-unknown-linux-gnu
>>
>> $ cat pr.c
>> void foo(void);
>>
>> void foo() 
>> {
>> }
>> $ bin/clang -c pr.c -Wstrict-prototypes -fno-knr-functions
>> clang-14: warning: argument unused during compilation: '-fno-knr-functions' 
>> [-Wunused-command-line-argument]
>> pr.c:3:9: warning: a function declaration without a prototype is deprecated 
>> in all versions of C [-Wstrict-prototypes]
>> void foo()
>>
>>   ^
>>void
>>
>> 1 warning generated.
>>
>> It works if -fno-knr-functions is passed with Xclang .  Is it intentional 
>> that -fno-knr-functions is only a cc1 option? That makes it very hard for us 
>> to enable it.
>>
>> $ bin/clang -c pr.c -Wstrict-prototypes -Xclang -fno-knr-functions (no 
>> warnings)
>
> No, that's not at all intentional -- it should be exposed as a driver flag. I 
> can reproduce the issue locally and will fix this today (it's very strange 
> because the option is listed as a CoreOption should it should be exposed 
> through the driver). I'm very sorry for the trouble, but thank you for 
> catching this!

Sheesh, I added the driver tests to make sure we don't accept a negated version 
of the flag, but I didn't add the test to validate that the driver accepted the 
flag, which is why I didn't catch this before. I've corrected the problem and 
added test coverage in 786954db06ab253dbd62d059036e06f6bbd9223c 
. Thanks 
for letting me know about the issue!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-30 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3484076 , @manojgupta 
wrote:

> Tried locally but I still see the warning with -fno-knr-functions. It also 
> says that the argument is unused.
>
> bin/clang --version
> clang version 15.0.0 (https://github.com/llvm/llvm-project.git 
> a9d68a5524dea113cace5983697786599cbdce9a 
> )
> Target: x86_64-unknown-linux-gnu
>
> $ cat pr.c
> void foo(void);
>
> void foo() 
> {
> }
> $ bin/clang -c pr.c -Wstrict-prototypes -fno-knr-functions
> clang-14: warning: argument unused during compilation: '-fno-knr-functions' 
> [-Wunused-command-line-argument]
> pr.c:3:9: warning: a function declaration without a prototype is deprecated 
> in all versions of C [-Wstrict-prototypes]
> void foo()
>
>   ^
>void
>
> 1 warning generated.
>
> It works if -fno-knr-functions is passed with Xclang .  Is it intentional 
> that -fno-knr-functions is only a cc1 option? That makes it very hard for us 
> to enable it.
>
> $ bin/clang -c pr.c -Wstrict-prototypes -Xclang -fno-knr-functions (no 
> warnings)

No, that's not at all intentional -- it should be exposed as a driver flag. I 
can reproduce the issue locally and will fix this today (it's very strange 
because the option is listed as a CoreOption should it should be exposed 
through the driver). I'm very sorry for the trouble, but thank you for catching 
this!

In D122895#3484077 , @manojgupta 
wrote:

> Following behavior is also surprising:
>
> ` -Werror -Wimplicit-function-declaration` does not rep-promote it to an 
> error either if I suppress it globally with 
> -Wno-error=implicit-function-declaration.

On its face, I agree that it's surprising, but that's the general behavior of 
`-Werror` when warning flags default to an error, and is not specific to the 
changes here: https://godbolt.org/z/ronq687cj


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-30 Thread Manoj Gupta via Phabricator via cfe-commits

manojgupta added a comment.

Following behavior is also surprising:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-30 Thread Manoj Gupta via Phabricator via cfe-commits

manojgupta added a comment.

Tried locally but I still see the warning with -fno-knr-functions. It also says 
that the argument is unused.

bin/clang --version
clang version 15.0.0 (https://github.com/llvm/llvm-project.git 
a9d68a5524dea113cace5983697786599cbdce9a 
)
Target: x86_64-unknown-linux-gnu

$ cat pr.c
void foo(void);

void foo() 
{
}
$ bin/clang -c pr.c -Wstrict-prototypes -fno-knr-functions
clang-14: warning: argument unused during compilation: '-fno-knr-functions' 
[-Wunused-command-line-argument]
pr.c:3:9: warning: a function declaration without a prototype is deprecated in 
all versions of C [-Wstrict-prototypes]
void foo()

  ^
   void

1 warning generated.

It works if -fno-knr-functions is passed with Xclang .  Is it intentional that 
-fno-knr-functions is only a cc1 option? That makes it very hard for us to 
enable it.

$ bin/clang -c pr.c -Wstrict-prototypes -Xclang -fno-knr-functions (no warnings)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3483175 , @manojgupta 
wrote:

> Unless I  probably mis-interpreted something, -fno-knr-functions does not 
> suppress the warning:  https://godbolt.org/z/rbEfbbb33

Godbolt hasn't had the chance to catch up to 
https://github.com/llvm/llvm-project/commit/ef87865b98fa25af1d2c045bab1268b2a1503374
 yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3483163 , @manojgupta 
wrote:

>   Basically, I'm wondering if you'd be able to enable -fno-knr-function?
>
> Thanks. this looks promising. Any ideas when  -fno-knr-function support was 
> added?

Oops, I had a slight typo, it's `-fno-knr-functions` plural. But it was added 9 
days ago in 
https://github.com/llvm/llvm-project/commit/9955f14aaf9995f6f0f7bf058ac740463003c470,
 and as of today, using that flag will disable the `-Wstrict-prototypes` 
warnings entirely (as will specifying `-std=c2x`).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Manoj Gupta via Phabricator via cfe-commits

manojgupta added a comment.

Unless I  probably mis-interpreted something, -fno-knr-functions does not 
suppress the warning:  https://godbolt.org/z/rbEfbbb33


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Manoj Gupta via Phabricator via cfe-commits

manojgupta added a comment.

  Basically, I'm wondering if you'd be able to enable -fno-knr-function?

Thanks. this looks promising. Any ideas when  -fno-knr-function support was 
added?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3482575 , @aaron.ballman 
wrote:

> In D122895#3482555 , @tahonermann 
> wrote:
>
>>> I think it's debatable whether this is a bug or not
>>
>> For C99 through C17, I kind of agree, but for C2x (where the warning is 
>> still issued with `-Wstrict-prototypes`), my understanding is that `void 
>> foo(void)` and `void foo()` are equivalent; there is no unprototyped 
>> declaration. I think the diagnostic should at least be suppressed for C2x 
>> since we don't want to encourage programmers to explicitly add `void` when 
>> targeting that standard.
>
> Good catch... `-Wstrict-prototypes` should be a noop in C2x mode! I'll work 
> on fixing that.

I fixed that issue in ef87865b98fa25af1d2c045bab1268b2a1503374 
, thanks 
for catching it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3482707 , @manojgupta 
wrote:

>   Is disabling the pedantic warning an option for your users?
>
> Disabling it wholesale is not an option since they actually want this warning 
>  (the older version). But we agreed to disable it specifically for the code 
> where the warning was getting fired.
> One instance is https://review.coreboot.org/c/coreboot/+/63936 .
>
> I have been fixing our codebase to clean this and clean the instances. But it 
> takes a lot of time and effort. Plus it takes a long time to clean one 
> failure before I can find others  (public CLs) 
> https://chromium-review.googlesource.com/q/Wstrict-prototypes+owner:manojgupta

Out of curiosity, are you using K C functions in your code base (definitions 
with an identifier list or a declaration with empty parens)? Basically, I'm 
wondering if you'd be able to enable `-fno-knr-function`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Manoj Gupta via Phabricator via cfe-commits

manojgupta added a comment.

  Is disabling the pedantic warning an option for your users?

Disabling it wholesale is not an option since they actually want this warning  
(the older version). But we agreed to disable it specifically for the code 
where the warning was getting fired.
One instance is https://review.coreboot.org/c/coreboot/+/63936 .

I have been fixing our codebase to clean this and clean the instances. But it 
takes a lot of time and effort. Plus it takes a long time to clean one failure 
before I can find others  (public CLs) 
https://chromium-review.googlesource.com/q/Wstrict-prototypes+owner:manojgupta


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3482555 , @tahonermann 
wrote:

>> I think it's debatable whether this is a bug or not
>
> For C99 through C17, I kind of agree, but for C2x (where the warning is still 
> issued with `-Wstrict-prototypes`), my understanding is that `void foo(void)` 
> and `void foo()` are equivalent; there is no unprototyped declaration. I 
> think the diagnostic should at least be suppressed for C2x since we don't 
> want to encourage programmers to explicitly add `void` when targeting that 
> standard.

Good catch... `-Wstrict-prototypes` should be a noop in C2x mode! I'll work on 
fixing that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Tom Honermann via Phabricator via cfe-commits

tahonermann added a comment.

> I think it's debatable whether this is a bug or not

For C99 through C17, I kind of agree, but for C2x (where the warning is still 
issued with `-Wstrict-prototypes`), my understanding is that `void foo(void)` 
and `void foo()` are equivalent; there is no unprototyped declaration. I think 
the diagnostic should at least be suppressed for C2x since we don't want to 
encourage programmers to explicitly add `void` when targeting that standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-29 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D122895#3481564 , @manojgupta 
wrote:

> Some of our users are not very happy with the churn probably caused by this 
> change where the declaration has the "void" argument but the later definition 
> does not have explicit "void".
>
>   void foo(void);
>   
>   void foo() 
>   {
>   }
>
> GCC  does not warn about this usage: https://godbolt.org/z/zPP8qjc98
>
> Any opinions?

Thank you for the feedback! I think it's debatable whether this is a bug or 
not, but it's an interesting case that I may think about a bit further before 
making any final decisions. My current thinking is...

We turned `-Wstrict-prototypes` into a pedantic warning specifically telling 
the user about K C function usage everywhere it occurs, and from that 
perspective `void f() {}` is correct to diagnose pedantically as the signature 
is specifically a K C one. However, the composite type between `void f(void)` 
(with a parameter list) and `void f() {}` (with an empty identifier list) is a 
function with a prototype of `void(void)` (per C99 6.2.7p3). Thus, a call to 
the function through the declaration can pass no arguments, and a call to the 
function through the definition can pass no arguments, and so it also seems 
reasonable to not diagnose.

(We added `-Wdeprecated-non-prototype` to cover the case where there's a 
behavioral change to the code in C2x, which diagnoses `void f(int);` and `void 
f(i) int i; {}` (this code will break in C2x), so despite the similarity with 
your case, it's a different diagnostic entirely.)

FWIW, GCC's behavior isn't particularly interesting because we're knowingly 
diverging from their strategy with the warning, so it's not too surprising that 
our behavior is different.

I think what sells it for me not being a bug is that we issue that warning when 
determining the function *type* when forming the declaration which happens 
before doing function merging. At the point where we issue this warning, we 
don't have enough information to know whether the function will eventually have 
a composite type with a prototype or not (we don't even have enough information 
to know whether it's for a function declaration or not; it could be for a 
typedef, etc), so suppressing the diagnostic would be rather difficult. And 
because it's a pedantic warning specifically, I think it's defensible that 
we're issuing the diagnostic.

Is disabling the pedantic warning an option for your users?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-28 Thread Manoj Gupta via Phabricator via cfe-commits

manojgupta added a comment.

Some of our users are not very happy with the churn probably caused by this 
change where the declaration has the "void" argument but the later definition 
does not have explicit "void".

  void foo(void);
  
  void foo() 
  {
  }

GCC  does not warn about this usage: https://godbolt.org/z/zPP8qjc98

Any opinions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8
 
-static int func(f)
+static int func(f) // expected-warning {{this function declaration without a 
prototype is deprecated in all versions of C and changes behavior in C2x}}
   void *f;

aaron.ballman wrote:
> jyknight wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > jyknight wrote:
> > > > > This message seems vaguer than necessary, since we know _for sure_ 
> > > > > this is invalid.
> > > > > 
> > > > > Can we say something like: "K function definitions are 
> > > > > deprecated in all versions of C and not supported in C2x"?
> > > > I'd like to avoid using K in the diagnostic text (we're inconsistent 
> > > > about K vs prototype, etc already, but I'd like to see us excise 
> > > > `K` from diagnostics because it's not particularly descriptive to 
> > > > anyone younger than about 40. :-D
> > > > 
> > > > How about: `function declarations without a prototype are deprecated in 
> > > > all versions of C and not supported in C2x` ?
> > > I went with `a function declaration without a prototype is deprecated in 
> > > all versions of C and is not supported in C2x`, let me know if you think 
> > > it needs further adjusting. I also reworded the other diagnostics to 
> > > sound similar.
> > I do agree with you that "K" is perhaps not the best name either...
> > 
> > But I think this warning message may still be confusing to users. A typical 
> > C user I think will be surprised to see a warning about a "declaration" 
> > pointing to a definition -- even though that is technically accurate. 
> > Especially when we're complaining concretely about the weird old definition 
> > syntax, it would be better to say "definition".
> > 
> > It looks like the most common name for this is "old-style function 
> > definition" -- both GCC and MSVC use that name, and clang also did, before. 
> > (Hmmm. And GCC actually seems to place this particular warning under the 
> > off-by-default -Wold-style-definition flag -- which clang doesn't 
> > implement.) I don't know that "old-style" is a great description either, 
> > but "old-style function definition" definitely seems more understandable 
> > than "function declaration without a prototype" when talking about a 
> > K separated-arguments-types definition.
> > But I think this warning message may still be confusing to users. A typical 
> > C user I think will be surprised to see a warning about a "declaration" 
> > pointing to a definition -- even though that is technically accurate. 
> > Especially when we're complaining concretely about the weird old definition 
> > syntax, it would be better to say "definition".
> 
> I can look into making it say "definition" when written on a definition, but 
> this feels like it's starting to split awfully fine hairs to me, so if it 
> turns out to add more code than I think it's worth, I may stick with 
> "declaration" because it's still correct and I don't think the confusion 
> should impede a user's ability to fix the bug *too much*.
Yup, this turned out to be hard enough to not be worth it IMO. The issue is 
that when merging function declarations, we don't know whether the function we 
want to diagnose is a definition or not (we've not yet started parsing its 
body, we've just finished with its declarator).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/test/Sema/knr-def-call.c:15
+void f2(float); // expected-note{{previous declaration is here}} \
+   expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is followed by a function without 
a prototype}}
+void f2(x) float x; { } // expected-warning{{promoted type 'double' of K 
function parameter is not compatible with the parameter type 'float' declared 
in a previous prototype}} \

jyknight wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > jyknight wrote:
> > > > > > I think we don't want to emit a warning here. If a declaration with 
> > > > > > params doesn't match a K definition, we already emit a 
> > > > > > conflicting-types error.
> > > > > > 
> > > > > > [[Well...except for one case, I've noticed -- we don't current emit 
> > > > > > any warning when a definition with no parameters fails to match a 
> > > > > > preceding declaration with params, e.g. `void f(int); void f() { 
> > > > > > }`. I'm quite surprised -- I'm willing to say that such code is 
> > > > > > 100% just a bug, not intentional. I note that GCC unconditionally 
> > > > > > rejects it. I think we should also be emitting an unconditional 
> > > > > > error in that case.]]]
> > > > > > 
> > > > > > Anyhow, when you have a K style definition with parameters, that 
> > > > > > -- all by itself -- is definitely invalid in C2x, so we don't need 
> > > > > > to emit any warning on the declaration.
> > > > > > I'm quite surprised -- I'm willing to say that such code is 100% 
> > > > > > just a bug, not intentional. I note that GCC unconditionally 
> > > > > > rejects it. I think we should also be emitting an unconditional 
> > > > > > error in that case.
> > > > > 
> > > > > I'd rather we be consistent here -- either every mixture of 
> > > > > prototype/unprototyped is an error, or they're all a warning. I've 
> > > > > added your example as a test case and we warn on it as being a change 
> > > > > of behavior in C2x, which I think is defensible.
> > > > > 
> > > > > > Anyhow, when you have a K style definition with parameters, that 
> > > > > > -- all by itself -- is definitely invalid in C2x, so we don't need 
> > > > > > to emit any warning on the declaration.
> > > > > 
> > > > > I tend to agree, let me see what I can do.
> > > > I addressed this so we no longer diagnose the function with a prototype 
> > > > in the case where it precedes the function without a prototype.
> > > > I'd rather we be consistent here -- either every mixture of 
> > > > prototype/unprototyped is an error, or they're all a warning. I've 
> > > > added your example as a test case and we warn on it as being a change 
> > > > of behavior in C2x, which I think is defensible.
> > > 
> > > Even before, we are NOT consistent. We emit an error on `void f(int); 
> > > void f(x) float x; {}`, but not for `void f(int); void f() {}`. In both 
> > > cases, we have a prototyped declaration, followed by an old-style 
> > > "prototypeless" definition. I think it would be sensible to diagnose with 
> > > an unconditional error in both cases, not only the former.
> > > Even before, we are NOT consistent. We emit an error on void f(int); void 
> > > f(x) float x; {}, but not for void f(int); void f() {}. In both cases, we 
> > > have a prototyped declaration, followed by an old-style "prototypeless" 
> > > definition. I think it would be sensible to diagnose with an 
> > > unconditional error in both cases, not only the former.
> > 
> > I don't think `void f(int); void f() {}` should be diagnosed solely as an 
> > issue with strict prototypes; I think it should be diagnosed the same as 
> > `void f(int); void f(x) float x; {}`, which is *not* about strict 
> > prototypes but *is* about the fact that the function types cannot merge 
> > because they conflict. Once there's a declaration with a prototype, the 
> > function always has a prototype (see C17 6.2.7p3 ) and redeclarations 
> > (including for definition) need to have a compatible signature (see C17 
> > 6.7.6.3p15). So I'd expect a `conflicting types` error diagnostic. I think 
> > it's defensible to *additionally* issue the strict prototypes warning 
> > (though if we can silence the warning because we're already issuing an 
> > error without introducing significant extra complexity, that would be 
> > ideal). I'll look into adding the error diagnostic.
> Sorry about being unclear -- what you propose is precisely what I meant to 
> say.
Thank you for confirming (and poking me about this). As always, I really 
appreciate your input!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-12 Thread James Y Knight via Phabricator via cfe-commits

jyknight added inline comments.



Comment at: clang/test/Sema/knr-def-call.c:15
+void f2(float); // expected-note{{previous declaration is here}} \
+   expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is followed by a function without 
a prototype}}
+void f2(x) float x; { } // expected-warning{{promoted type 'double' of K 
function parameter is not compatible with the parameter type 'float' declared 
in a previous prototype}} \

aaron.ballman wrote:
> jyknight wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > jyknight wrote:
> > > > > I think we don't want to emit a warning here. If a declaration with 
> > > > > params doesn't match a K definition, we already emit a 
> > > > > conflicting-types error.
> > > > > 
> > > > > [[Well...except for one case, I've noticed -- we don't current emit 
> > > > > any warning when a definition with no parameters fails to match a 
> > > > > preceding declaration with params, e.g. `void f(int); void f() { }`. 
> > > > > I'm quite surprised -- I'm willing to say that such code is 100% just 
> > > > > a bug, not intentional. I note that GCC unconditionally rejects it. I 
> > > > > think we should also be emitting an unconditional error in that 
> > > > > case.]]]
> > > > > 
> > > > > Anyhow, when you have a K style definition with parameters, that -- 
> > > > > all by itself -- is definitely invalid in C2x, so we don't need to 
> > > > > emit any warning on the declaration.
> > > > > I'm quite surprised -- I'm willing to say that such code is 100% just 
> > > > > a bug, not intentional. I note that GCC unconditionally rejects it. I 
> > > > > think we should also be emitting an unconditional error in that case.
> > > > 
> > > > I'd rather we be consistent here -- either every mixture of 
> > > > prototype/unprototyped is an error, or they're all a warning. I've 
> > > > added your example as a test case and we warn on it as being a change 
> > > > of behavior in C2x, which I think is defensible.
> > > > 
> > > > > Anyhow, when you have a K style definition with parameters, that -- 
> > > > > all by itself -- is definitely invalid in C2x, so we don't need to 
> > > > > emit any warning on the declaration.
> > > > 
> > > > I tend to agree, let me see what I can do.
> > > I addressed this so we no longer diagnose the function with a prototype 
> > > in the case where it precedes the function without a prototype.
> > > I'd rather we be consistent here -- either every mixture of 
> > > prototype/unprototyped is an error, or they're all a warning. I've added 
> > > your example as a test case and we warn on it as being a change of 
> > > behavior in C2x, which I think is defensible.
> > 
> > Even before, we are NOT consistent. We emit an error on `void f(int); void 
> > f(x) float x; {}`, but not for `void f(int); void f() {}`. In both cases, 
> > we have a prototyped declaration, followed by an old-style "prototypeless" 
> > definition. I think it would be sensible to diagnose with an unconditional 
> > error in both cases, not only the former.
> > Even before, we are NOT consistent. We emit an error on void f(int); void 
> > f(x) float x; {}, but not for void f(int); void f() {}. In both cases, we 
> > have a prototyped declaration, followed by an old-style "prototypeless" 
> > definition. I think it would be sensible to diagnose with an unconditional 
> > error in both cases, not only the former.
> 
> I don't think `void f(int); void f() {}` should be diagnosed solely as an 
> issue with strict prototypes; I think it should be diagnosed the same as 
> `void f(int); void f(x) float x; {}`, which is *not* about strict prototypes 
> but *is* about the fact that the function types cannot merge because they 
> conflict. Once there's a declaration with a prototype, the function always 
> has a prototype (see C17 6.2.7p3 ) and redeclarations (including for 
> definition) need to have a compatible signature (see C17 6.7.6.3p15). So I'd 
> expect a `conflicting types` error diagnostic. I think it's defensible to 
> *additionally* issue the strict prototypes warning (though if we can silence 
> the warning because we're already issuing an error without introducing 
> significant extra complexity, that would be ideal). I'll look into adding the 
> error diagnostic.
Sorry about being unclear -- what you propose is precisely what I meant to say.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8
 
-static int func(f)
+static int func(f) // expected-warning {{this function declaration without a 
prototype is deprecated in all versions of C and changes behavior in C2x}}
   void *f;

jyknight wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > jyknight wrote:
> > > > This message seems vaguer than necessary, since we know _for sure_ this 
> > > > is invalid.
> > > > 
> > > > Can we say something like: "K function definitions are 
> > > > deprecated in all versions of C and not supported in C2x"?
> > > I'd like to avoid using K in the diagnostic text (we're inconsistent 
> > > about K vs prototype, etc already, but I'd like to see us excise `K` 
> > > from diagnostics because it's not particularly descriptive to anyone 
> > > younger than about 40. :-D
> > > 
> > > How about: `function declarations without a prototype are deprecated in 
> > > all versions of C and not supported in C2x` ?
> > I went with `a function declaration without a prototype is deprecated in 
> > all versions of C and is not supported in C2x`, let me know if you think it 
> > needs further adjusting. I also reworded the other diagnostics to sound 
> > similar.
> I do agree with you that "K" is perhaps not the best name either...
> 
> But I think this warning message may still be confusing to users. A typical C 
> user I think will be surprised to see a warning about a "declaration" 
> pointing to a definition -- even though that is technically accurate. 
> Especially when we're complaining concretely about the weird old definition 
> syntax, it would be better to say "definition".
> 
> It looks like the most common name for this is "old-style function 
> definition" -- both GCC and MSVC use that name, and clang also did, before. 
> (Hmmm. And GCC actually seems to place this particular warning under the 
> off-by-default -Wold-style-definition flag -- which clang doesn't implement.) 
> I don't know that "old-style" is a great description either, but "old-style 
> function definition" definitely seems more understandable than "function 
> declaration without a prototype" when talking about a K 
> separated-arguments-types definition.
> But I think this warning message may still be confusing to users. A typical C 
> user I think will be surprised to see a warning about a "declaration" 
> pointing to a definition -- even though that is technically accurate. 
> Especially when we're complaining concretely about the weird old definition 
> syntax, it would be better to say "definition".

I can look into making it say "definition" when written on a definition, but 
this feels like it's starting to split awfully fine hairs to me, so if it turns 
out to add more code than I think it's worth, I may stick with "declaration" 
because it's still correct and I don't think the confusion should impede a 
user's ability to fix the bug *too much*.



Comment at: clang/test/Parser/declarators.c:5
 
-void f0();
+void f0(); /* expected-warning {{a function declaration without a prototype is 
deprecated in all versions of C}} */
 void f1(int [*]);

jyknight wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > jyknight wrote:
> > > > Perhaps we should add an explanation to the message like
> > > >  `(specify '(void)' if you intended to accept no arguments)`
> > > > to make it clearer to folks who aren't familiar with this weirdness yet?
> > > They're already offered a fixit for this situation, so I don't know that 
> > > we need the extra explanation? The user currently gets something like 
> > > this:
> > > ```
> > > C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:16: 
> > > warning: declaration of a function without a prototype is deprecated in 
> > > all versions of C [-Wstrict-prototypes]
> > > void other_func();
> > >^
> > > void
> > > ```
> > > Do you still think the diagnostic needs to be made longer?
> > I elected to leave this alone unless you feel strongly.
> I guess I sort of wondered if users might consider "without a prototype" to 
> generally indicate "implicit declaration". So, the wording "declaration 
> without a prototype" might be like..."wait you're complaining that my 
> declaration doesn't have a declaration? Huh? It's right there!"
> 
> But, with the fixit hint suggesting the addition of "void", that's probably 
> sufficiently explanatory.
> 
> I guess I sort of wondered if users might consider "without a prototype" to 
> generally indicate "implicit declaration". So, the wording "declaration 
> without a prototype" might be like..."wait you're complaining that my 
> declaration doesn't have a declaration? Huh? It's right there!"

That's a good point, we should watch to see if that sort of confusion does 
happen in practice (I'll keep my eyes peeled), but I also think the fix-it 
should be sufficiently

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-12 Thread James Y Knight via Phabricator via cfe-commits

jyknight added inline comments.



Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8
 
-static int func(f)
+static int func(f) // expected-warning {{this function declaration without a 
prototype is deprecated in all versions of C and changes behavior in C2x}}
   void *f;

aaron.ballman wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > This message seems vaguer than necessary, since we know _for sure_ this 
> > > is invalid.
> > > 
> > > Can we say something like: "K function definitions are deprecated 
> > > in all versions of C and not supported in C2x"?
> > I'd like to avoid using K in the diagnostic text (we're inconsistent 
> > about K vs prototype, etc already, but I'd like to see us excise `K` 
> > from diagnostics because it's not particularly descriptive to anyone 
> > younger than about 40. :-D
> > 
> > How about: `function declarations without a prototype are deprecated in all 
> > versions of C and not supported in C2x` ?
> I went with `a function declaration without a prototype is deprecated in all 
> versions of C and is not supported in C2x`, let me know if you think it needs 
> further adjusting. I also reworded the other diagnostics to sound similar.
I do agree with you that "K" is perhaps not the best name either...

But I think this warning message may still be confusing to users. A typical C 
user I think will be surprised to see a warning about a "declaration" pointing 
to a definition -- even though that is technically accurate. Especially when 
we're complaining concretely about the weird old definition syntax, it would be 
better to say "definition".

It looks like the most common name for this is "old-style function definition" 
-- both GCC and MSVC use that name, and clang also did, before. (Hmmm. And GCC 
actually seems to place this particular warning under the off-by-default 
-Wold-style-definition flag -- which clang doesn't implement.) I don't know 
that "old-style" is a great description either, but "old-style function 
definition" definitely seems more understandable than "function declaration 
without a prototype" when talking about a K separated-arguments-types 
definition.



Comment at: clang/test/Parser/declarators.c:5
 
-void f0();
+void f0(); /* expected-warning {{a function declaration without a prototype is 
deprecated in all versions of C}} */
 void f1(int [*]);

aaron.ballman wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > Perhaps we should add an explanation to the message like
> > >  `(specify '(void)' if you intended to accept no arguments)`
> > > to make it clearer to folks who aren't familiar with this weirdness yet?
> > They're already offered a fixit for this situation, so I don't know that we 
> > need the extra explanation? The user currently gets something like this:
> > ```
> > C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:16: 
> > warning: declaration of a function without a prototype is deprecated in all 
> > versions of C [-Wstrict-prototypes]
> > void other_func();
> >^
> > void
> > ```
> > Do you still think the diagnostic needs to be made longer?
> I elected to leave this alone unless you feel strongly.
I guess I sort of wondered if users might consider "without a prototype" to 
generally indicate "implicit declaration". So, the wording "declaration without 
a prototype" might be like..."wait you're complaining that my declaration 
doesn't have a declaration? Huh? It's right there!"

But, with the fixit hint suggesting the addition of "void", that's probably 
sufficiently explanatory.




Comment at: clang/test/Sema/knr-def-call.c:15
+void f2(float); // expected-note{{previous declaration is here}} \
+   expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is followed by a function without 
a prototype}}
+void f2(x) float x; { } // expected-warning{{promoted type 'double' of K 
function parameter is not compatible with the parameter type 'float' declared 
in a previous prototype}} \

aaron.ballman wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > I think we don't want to emit a warning here. If a declaration with 
> > > params doesn't match a K definition, we already emit a 
> > > conflicting-types error.
> > > 
> > > [[Well...except for one case, I've noticed -- we don't current emit any 
> > > warning when a definition with no parameters fails to match a preceding 
> > > declaration with params, e.g. `void f(int); void f() { }`. I'm quite 
> > > surprised -- I'm willing to say that such code is 100% just a bug, not 
> > > intentional. I note that GCC unconditionally rejects it. I think we 
> > > should also be emitting an unconditional error in that case.]]]
> > > 
> > > Anyhow, when you have a K style definition with parameters, that -- all 
> > > by itself -- is definitely invalid in C2x, so we don't need to emit

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-08 Thread Aaron Ballman via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG11da1b53d8cd: [C89/C2x] Improve diagnostics around strict 
prototypes in C (authored by aaron.ballman).

Changed prior to commit:
  https://reviews.llvm.org/D122895?vs=419853=421627#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122895

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/2009-06-01-addrofknr.c
  clang/test/FixIt/fixit.c
  clang/test/Parser/declarators.c
  clang/test/Parser/knr_parameter_attributes.c
  clang/test/Parser/opencl-kernel.cl
  clang/test/Parser/traditional_arg_scope.c
  clang/test/Sema/arg-duplicate.c
  clang/test/Sema/block-return.c
  clang/test/Sema/knr-def-call.c
  clang/test/Sema/knr-variadic-def.c
  clang/test/Sema/vfprintf-valid-redecl.c
  clang/test/Sema/warn-deprecated-non-prototype.c
  clang/test/Sema/warn-missing-prototypes.c
  clang/test/Sema/warn-strict-prototypes.c
  clang/test/Sema/warn-strict-prototypes.m
  clang/test/SemaObjC/nonnull.m
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
  clang/test/SemaOpenCL/func.cl

Index: clang/test/SemaOpenCL/func.cl
===
--- clang/test/SemaOpenCL/func.cl
+++ clang/test/SemaOpenCL/func.cl
@@ -43,9 +43,9 @@
 #endif
 
 // Expect no diagnostics for an empty parameter list.
-void bar();
+void bar(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 
-void bar()
+void bar() // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 {
   // declaring a function pointer is an error
   void (*fptr)(int);
Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -11,7 +11,7 @@
 
 typedef struct {int a;} ndrange_t;
 // Diagnostic tests for different overloads of enqueue_kernel from Table 6.13.17.1 of OpenCL 2.0 Spec.
-kernel void enqueue_kernel_tests() {
+kernel void enqueue_kernel_tests(void) {
   queue_t default_queue;
   unsigned flags = 0;
   QUALS ndrange_t ndrange;
@@ -169,7 +169,7 @@
 }
 
 // Diagnostic tests for get_kernel_work_group_size and allowed block parameter types in dynamic parallelism.
-kernel void work_group_size_tests() {
+kernel void work_group_size_tests(void) {
   void (^const block_A)(void) = ^{
 return;
   };
@@ -223,16 +223,22 @@
 kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
-  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
+// expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected block argument type}}
 }
 
 kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
-  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected 'ndrange_t' argument type}}
+ // expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected block argument type}}
 }
 
@@ -241,12 +247,18 @@
 kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
   buf[0] =

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Thanks @erichkeane!

@jyknight, @hubert.reinterpretcast, @eli.friedman -- I'll likely land this 
sometime tomorrow unless I hear from you. But if I land it and you still have 
comments, I'll be happy to address them post commit.


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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-06 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Ping


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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman updated this revision to Diff 419853.
aaron.ballman added a comment.

Fix failing test case caught by precommit CI.


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

https://reviews.llvm.org/D122895

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/2009-06-01-addrofknr.c
  clang/test/FixIt/fixit.c
  clang/test/Parser/declarators.c
  clang/test/Parser/knr_parameter_attributes.c
  clang/test/Parser/opencl-kernel.cl
  clang/test/Parser/traditional_arg_scope.c
  clang/test/Sema/arg-duplicate.c
  clang/test/Sema/block-return.c
  clang/test/Sema/knr-def-call.c
  clang/test/Sema/knr-variadic-def.c
  clang/test/Sema/vfprintf-valid-redecl.c
  clang/test/Sema/warn-deprecated-non-prototype.c
  clang/test/Sema/warn-missing-prototypes.c
  clang/test/Sema/warn-strict-prototypes.c
  clang/test/Sema/warn-strict-prototypes.m
  clang/test/SemaObjC/nonnull.m
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
  clang/test/SemaOpenCL/func.cl

Index: clang/test/SemaOpenCL/func.cl
===
--- clang/test/SemaOpenCL/func.cl
+++ clang/test/SemaOpenCL/func.cl
@@ -43,9 +43,9 @@
 #endif
 
 // Expect no diagnostics for an empty parameter list.
-void bar();
+void bar(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 
-void bar()
+void bar() // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 {
   // declaring a function pointer is an error
   void (*fptr)(int);
Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -11,7 +11,7 @@
 
 typedef struct {int a;} ndrange_t;
 // Diagnostic tests for different overloads of enqueue_kernel from Table 6.13.17.1 of OpenCL 2.0 Spec.
-kernel void enqueue_kernel_tests() {
+kernel void enqueue_kernel_tests(void) {
   queue_t default_queue;
   unsigned flags = 0;
   QUALS ndrange_t ndrange;
@@ -169,7 +169,7 @@
 }
 
 // Diagnostic tests for get_kernel_work_group_size and allowed block parameter types in dynamic parallelism.
-kernel void work_group_size_tests() {
+kernel void work_group_size_tests(void) {
   void (^const block_A)(void) = ^{
 return;
   };
@@ -223,16 +223,22 @@
 kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
-  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
+// expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected block argument type}}
 }
 
 kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
-  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected 'ndrange_t' argument type}}
+ // expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected block argument type}}
 }
 
@@ -241,12 +247,18 @@
 kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' requires cl_khr_subgroups or __opencl_c_subgroups support}}
+

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8
 
-static int func(f)
+static int func(f) // expected-warning {{this function declaration without a 
prototype is deprecated in all versions of C and changes behavior in C2x}}
   void *f;

aaron.ballman wrote:
> jyknight wrote:
> > This message seems vaguer than necessary, since we know _for sure_ this is 
> > invalid.
> > 
> > Can we say something like: "K function definitions are deprecated 
> > in all versions of C and not supported in C2x"?
> I'd like to avoid using K in the diagnostic text (we're inconsistent about 
> K vs prototype, etc already, but I'd like to see us excise `K` from 
> diagnostics because it's not particularly descriptive to anyone younger than 
> about 40. :-D
> 
> How about: `function declarations without a prototype are deprecated in all 
> versions of C and not supported in C2x` ?
I went with `a function declaration without a prototype is deprecated in all 
versions of C and is not supported in C2x`, let me know if you think it needs 
further adjusting. I also reworded the other diagnostics to sound similar.



Comment at: clang/test/Parser/declarators.c:5
 
-void f0();
+void f0(); /* expected-warning {{a function declaration without a prototype is 
deprecated in all versions of C}} */
 void f1(int [*]);

aaron.ballman wrote:
> jyknight wrote:
> > Perhaps we should add an explanation to the message like
> >  `(specify '(void)' if you intended to accept no arguments)`
> > to make it clearer to folks who aren't familiar with this weirdness yet?
> They're already offered a fixit for this situation, so I don't know that we 
> need the extra explanation? The user currently gets something like this:
> ```
> C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:16: warning: 
> declaration of a function without a prototype is deprecated in all versions 
> of C [-Wstrict-prototypes]
> void other_func();
>^
> void
> ```
> Do you still think the diagnostic needs to be made longer?
I elected to leave this alone unless you feel strongly.



Comment at: clang/test/Sema/knr-def-call.c:15
+void f2(float); // expected-note{{previous declaration is here}} \
+   expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is followed by a function without 
a prototype}}
+void f2(x) float x; { } // expected-warning{{promoted type 'double' of K 
function parameter is not compatible with the parameter type 'float' declared 
in a previous prototype}} \

aaron.ballman wrote:
> jyknight wrote:
> > I think we don't want to emit a warning here. If a declaration with params 
> > doesn't match a K definition, we already emit a conflicting-types error.
> > 
> > [[Well...except for one case, I've noticed -- we don't current emit any 
> > warning when a definition with no parameters fails to match a preceding 
> > declaration with params, e.g. `void f(int); void f() { }`. I'm quite 
> > surprised -- I'm willing to say that such code is 100% just a bug, not 
> > intentional. I note that GCC unconditionally rejects it. I think we should 
> > also be emitting an unconditional error in that case.]]]
> > 
> > Anyhow, when you have a K style definition with parameters, that -- all 
> > by itself -- is definitely invalid in C2x, so we don't need to emit any 
> > warning on the declaration.
> > I'm quite surprised -- I'm willing to say that such code is 100% just a 
> > bug, not intentional. I note that GCC unconditionally rejects it. I think 
> > we should also be emitting an unconditional error in that case.
> 
> I'd rather we be consistent here -- either every mixture of 
> prototype/unprototyped is an error, or they're all a warning. I've added your 
> example as a test case and we warn on it as being a change of behavior in 
> C2x, which I think is defensible.
> 
> > Anyhow, when you have a K style definition with parameters, that -- all 
> > by itself -- is definitely invalid in C2x, so we don't need to emit any 
> > warning on the declaration.
> 
> I tend to agree, let me see what I can do.
I addressed this so we no longer diagnose the function with a prototype in the 
case where it precedes the function without a prototype.



Comment at: clang/test/Sema/warn-deprecated-non-prototype.c:46
+ strict-note {{this function declaration without a 
prototype changes behavior in C2x}}
+void order1(int i);   // both-warning {{this function declaration with a 
prototype changes behavior in C2x because it is preceded by a function without 
a prototype}}
+

aaron.ballman wrote:
> jyknight wrote:
> > Maybe something like "this function declaration will conflict with the 
> > preceding declaration in C2x, because the preceding declaration does not 
> > specify arguments."
> I can see the

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman updated this revision to Diff 419844.
aaron.ballman marked 5 inline comments as done.
aaron.ballman added a comment.

Updating based on review feedback.


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

https://reviews.llvm.org/D122895

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/2009-06-01-addrofknr.c
  clang/test/FixIt/fixit.c
  clang/test/Parser/declarators.c
  clang/test/Parser/knr_parameter_attributes.c
  clang/test/Parser/opencl-kernel.cl
  clang/test/Parser/traditional_arg_scope.c
  clang/test/Sema/arg-duplicate.c
  clang/test/Sema/block-return.c
  clang/test/Sema/implicit-decl.c
  clang/test/Sema/knr-def-call.c
  clang/test/Sema/knr-variadic-def.c
  clang/test/Sema/vfprintf-valid-redecl.c
  clang/test/Sema/warn-deprecated-non-prototype.c
  clang/test/Sema/warn-missing-prototypes.c
  clang/test/Sema/warn-strict-prototypes.c
  clang/test/Sema/warn-strict-prototypes.m
  clang/test/SemaObjC/nonnull.m
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
  clang/test/SemaOpenCL/func.cl

Index: clang/test/SemaOpenCL/func.cl
===
--- clang/test/SemaOpenCL/func.cl
+++ clang/test/SemaOpenCL/func.cl
@@ -43,9 +43,9 @@
 #endif
 
 // Expect no diagnostics for an empty parameter list.
-void bar();
+void bar(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 
-void bar()
+void bar() // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 {
   // declaring a function pointer is an error
   void (*fptr)(int);
Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -11,7 +11,7 @@
 
 typedef struct {int a;} ndrange_t;
 // Diagnostic tests for different overloads of enqueue_kernel from Table 6.13.17.1 of OpenCL 2.0 Spec.
-kernel void enqueue_kernel_tests() {
+kernel void enqueue_kernel_tests(void) {
   queue_t default_queue;
   unsigned flags = 0;
   QUALS ndrange_t ndrange;
@@ -169,7 +169,7 @@
 }
 
 // Diagnostic tests for get_kernel_work_group_size and allowed block parameter types in dynamic parallelism.
-kernel void work_group_size_tests() {
+kernel void work_group_size_tests(void) {
   void (^const block_A)(void) = ^{
 return;
   };
@@ -223,16 +223,22 @@
 kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
-  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
+// expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected block argument type}}
 }
 
 kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
-  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected 'ndrange_t' argument type}}
+ // expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected block argument type}}
 }
 
@@ -241,12 +247,18 @@
 kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' requires

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman marked 3 inline comments as done.
aaron.ballman added inline comments.



Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8
 
-static int func(f)
+static int func(f) // expected-warning {{this function declaration without a 
prototype is deprecated in all versions of C and changes behavior in C2x}}
   void *f;

jyknight wrote:
> This message seems vaguer than necessary, since we know _for sure_ this is 
> invalid.
> 
> Can we say something like: "K function definitions are deprecated in 
> all versions of C and not supported in C2x"?
I'd like to avoid using K in the diagnostic text (we're inconsistent about 
K vs prototype, etc already, but I'd like to see us excise `K` from 
diagnostics because it's not particularly descriptive to anyone younger than 
about 40. :-D

How about: `function declarations without a prototype are deprecated in all 
versions of C and not supported in C2x` ?



Comment at: clang/test/Parser/declarators.c:5
 
-void f0();
+void f0(); /* expected-warning {{a function declaration without a prototype is 
deprecated in all versions of C}} */
 void f1(int [*]);

jyknight wrote:
> Perhaps we should add an explanation to the message like
>  `(specify '(void)' if you intended to accept no arguments)`
> to make it clearer to folks who aren't familiar with this weirdness yet?
They're already offered a fixit for this situation, so I don't know that we 
need the extra explanation? The user currently gets something like this:
```
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.c:2:16: warning: 
declaration of a function without a prototype is deprecated in all versions of 
C [-Wstrict-prototypes]
void other_func();
   ^
void
```
Do you still think the diagnostic needs to be made longer?



Comment at: clang/test/Sema/implicit-decl.c:25
+Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t 
**vector, int32_t count) { // expected-error {{conflicting types}} \
+   
 // expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is preceded by a function without 
a prototype}}
  return 0;

jyknight wrote:
> It's not "preceded by a function without a prototype", though, it's preceeded 
> by a implicit declaration due to a call. Which we would've already diagnosed 
> with the default-on -Wimplicit-function-declaration diagnostic.
> 
> Although...what _is_ the desired behavior of an implicit function declaration 
> in C2x mode? If we permit it _at all_, it must still create a non-prototype 
> function declaration, I expect. In that case this decl with types would still 
> be valid, so the warning is just wrong.
> 
> It's not "preceded by a function without a prototype", though, it's preceeded 
> by a implicit declaration due to a call. Which we would've already diagnosed 
> with the default-on -Wimplicit-function-declaration diagnostic.

In order for that to work, Clang gins up an implicit declaration of `extern int 
func();` which is the signature C89 requires, so I think the diagnostic here is 
fine (it's also a very weird edge case I don't expect people to hit all that 
often, since the only way to trigger this diagnostic is to define with a 
prototype the implicitly-declared function).

There are no implicit function declarations after C89 (warned by default, but 
should be an error). There's also no implicit int after C89 (not even warned on 
by default, but should also be an error). Personally, I would like to see us 
enable both diagnostics as warning-defaults-to-error in C99 through C17 (if 
possible) and error only in C2x and forward, but I've not proposed it or worked 
on it yet (fixing one horror show at a time, basically).



Comment at: clang/test/Sema/knr-def-call.c:15
+void f2(float); // expected-note{{previous declaration is here}} \
+   expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is followed by a function without 
a prototype}}
+void f2(x) float x; { } // expected-warning{{promoted type 'double' of K 
function parameter is not compatible with the parameter type 'float' declared 
in a previous prototype}} \

jyknight wrote:
> I think we don't want to emit a warning here. If a declaration with params 
> doesn't match a K definition, we already emit a conflicting-types error.
> 
> [[Well...except for one case, I've noticed -- we don't current emit any 
> warning when a definition with no parameters fails to match a preceding 
> declaration with params, e.g. `void f(int); void f() { }`. I'm quite 
> surprised -- I'm willing to say that such code is 100% just a bug, not 
> intentional. I note that GCC unconditionally rejects it. I think we should 
> also be emitting an

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread James Y Knight via Phabricator via cfe-commits

jyknight added a comment.

Starting by looking at the test cases, I've got some suggestions on making the 
diagnostics a bit less confusing.




Comment at: clang/test/CodeGen/2009-06-01-addrofknr.c:8
 
-static int func(f)
+static int func(f) // expected-warning {{this function declaration without a 
prototype is deprecated in all versions of C and changes behavior in C2x}}
   void *f;

This message seems vaguer than necessary, since we know _for sure_ this is 
invalid.

Can we say something like: "K function definitions are deprecated in 
all versions of C and not supported in C2x"?



Comment at: clang/test/Parser/declarators.c:5
 
-void f0();
+void f0(); /* expected-warning {{a function declaration without a prototype is 
deprecated in all versions of C}} */
 void f1(int [*]);

Perhaps we should add an explanation to the message like
 `(specify '(void)' if you intended to accept no arguments)`
to make it clearer to folks who aren't familiar with this weirdness yet?



Comment at: clang/test/Sema/implicit-decl.c:25
+Boolean _CFCalendarDecomposeAbsoluteTimeV(const char *componentDesc, int32_t 
**vector, int32_t count) { // expected-error {{conflicting types}} \
+   
 // expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is preceded by a function without 
a prototype}}
  return 0;

It's not "preceded by a function without a prototype", though, it's preceeded 
by a implicit declaration due to a call. Which we would've already diagnosed 
with the default-on -Wimplicit-function-declaration diagnostic.

Although...what _is_ the desired behavior of an implicit function declaration 
in C2x mode? If we permit it _at all_, it must still create a non-prototype 
function declaration, I expect. In that case this decl with types would still 
be valid, so the warning is just wrong.




Comment at: clang/test/Sema/knr-def-call.c:15
+void f2(float); // expected-note{{previous declaration is here}} \
+   expected-warning {{this function declaration with a 
prototype changes behavior in C2x because it is followed by a function without 
a prototype}}
+void f2(x) float x; { } // expected-warning{{promoted type 'double' of K 
function parameter is not compatible with the parameter type 'float' declared 
in a previous prototype}} \

I think we don't want to emit a warning here. If a declaration with params 
doesn't match a K definition, we already emit a conflicting-types error.

[[Well...except for one case, I've noticed -- we don't current emit any warning 
when a definition with no parameters fails to match a preceding declaration 
with params, e.g. `void f(int); void f() { }`. I'm quite surprised -- I'm 
willing to say that such code is 100% just a bug, not intentional. I note that 
GCC unconditionally rejects it. I think we should also be emitting an 
unconditional error in that case.]]]

Anyhow, when you have a K style definition with parameters, that -- all by 
itself -- is definitely invalid in C2x, so we don't need to emit any warning on 
the declaration.



Comment at: clang/test/Sema/warn-deprecated-non-prototype.c:46
+ strict-note {{this function declaration without a 
prototype changes behavior in C2x}}
+void order1(int i);   // both-warning {{this function declaration with a 
prototype changes behavior in C2x because it is preceded by a function without 
a prototype}}
+

Maybe something like "this function declaration will conflict with the 
preceding declaration in C2x, because the preceding declaration does not 
specify arguments."


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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5566-5567
+def warn_non_prototype_changes_behavior : Warning<
+  "this function declaration without a prototype is deprecated in all versions 
"
+  "of C and changes behavior in C2x">, InGroup;
+def warn_prototype_changes_behavior : Warning<

cor3ntin wrote:
> "declaration of a function without a prototype is deprecated", may be 
> slightly better?
Sure, I can reword the rest so they're consistent.



Comment at: clang/lib/Sema/SemaDecl.cpp:3881
+  if (WithProto->getNumParams() != 0) {
+// The function definition has parameters, so this will change
+// behavior in C2x.

cor3ntin wrote:
> I wonder if
> 
>   - This shouldn't be placed in a separate function for clarity
>   - We  could bail out early for built ins ?
> 
> 
The change looks more complicated than it is because of the indentation changes 
-- I'm not certain putting this in its own function gains a whole lot (it's not 
reusable below; I already looked into doing that and wasn't happy with the 
results).

We can't bail out early for builtins because we still issue diagnostics in 
cases where only one side is a builtin. e.g.,
```
void exit(); // We still want to diagnose this as changing behavior specifically
```
See test/Sema/knr-variadic-def.c for this test case.




Comment at: clang/lib/Sema/SemaType.cpp:5560-5562
   if (!LangOpts.CPlusPlus &&
-  D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration) {
+  (D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration ||
+   D.getFunctionDefinitionKind() == FunctionDefinitionKind::Definition)) {

cor3ntin wrote:
> `if (!LangOpts.CPlusPlus)` is probably enough here
Hmm, yeah, you're right. Good catch!


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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Corentin Jabot via Phabricator via cfe-commits

cor3ntin added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5566-5567
+def warn_non_prototype_changes_behavior : Warning<
+  "this function declaration without a prototype is deprecated in all versions 
"
+  "of C and changes behavior in C2x">, InGroup;
+def warn_prototype_changes_behavior : Warning<

"declaration of a function without a prototype is deprecated", may be slightly 
better?



Comment at: clang/lib/Sema/SemaDecl.cpp:3881
+  if (WithProto->getNumParams() != 0) {
+// The function definition has parameters, so this will change
+// behavior in C2x.

I wonder if

  - This shouldn't be placed in a separate function for clarity
  - We  could bail out early for built ins ?





Comment at: clang/lib/Sema/SemaType.cpp:5560-5562
   if (!LangOpts.CPlusPlus &&
-  D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration) {
+  (D.getFunctionDefinitionKind() == FunctionDefinitionKind::Declaration ||
+   D.getFunctionDefinitionKind() == FunctionDefinitionKind::Definition)) {

`if (!LangOpts.CPlusPlus)` is probably enough here


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

https://reviews.llvm.org/D122895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman updated this revision to Diff 419734.
aaron.ballman added a comment.

Rebased, no changes.


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

https://reviews.llvm.org/D122895

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/2009-06-01-addrofknr.c
  clang/test/FixIt/fixit.c
  clang/test/Parser/declarators.c
  clang/test/Parser/knr_parameter_attributes.c
  clang/test/Parser/opencl-kernel.cl
  clang/test/Parser/traditional_arg_scope.c
  clang/test/Sema/arg-duplicate.c
  clang/test/Sema/block-return.c
  clang/test/Sema/implicit-decl.c
  clang/test/Sema/knr-def-call.c
  clang/test/Sema/knr-variadic-def.c
  clang/test/Sema/vfprintf-valid-redecl.c
  clang/test/Sema/warn-deprecated-non-prototype.c
  clang/test/Sema/warn-missing-prototypes.c
  clang/test/Sema/warn-strict-prototypes.c
  clang/test/Sema/warn-strict-prototypes.m
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
  clang/test/SemaOpenCL/func.cl

Index: clang/test/SemaOpenCL/func.cl
===
--- clang/test/SemaOpenCL/func.cl
+++ clang/test/SemaOpenCL/func.cl
@@ -43,9 +43,9 @@
 #endif
 
 // Expect no diagnostics for an empty parameter list.
-void bar();
+void bar(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 
-void bar()
+void bar() // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 {
   // declaring a function pointer is an error
   void (*fptr)(int);
Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -11,7 +11,7 @@
 
 typedef struct {int a;} ndrange_t;
 // Diagnostic tests for different overloads of enqueue_kernel from Table 6.13.17.1 of OpenCL 2.0 Spec.
-kernel void enqueue_kernel_tests() {
+kernel void enqueue_kernel_tests(void) {
   queue_t default_queue;
   unsigned flags = 0;
   QUALS ndrange_t ndrange;
@@ -169,7 +169,7 @@
 }
 
 // Diagnostic tests for get_kernel_work_group_size and allowed block parameter types in dynamic parallelism.
-kernel void work_group_size_tests() {
+kernel void work_group_size_tests(void) {
   void (^const block_A)(void) = ^{
 return;
   };
@@ -223,16 +223,22 @@
 kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
-  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
+// expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected block argument type}}
 }
 
 kernel void bar(global unsigned int *buf)
 {
   __private ndrange_t n;
-  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_sub_group_count_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected 'ndrange_t' argument type}}
+ // expected-warning@-1 {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_sub_group_count_for_ndrange(n, 1); // expected-error{{illegal call to 'get_kernel_sub_group_count_for_ndrange', expected block argument type}}
 }
 
@@ -241,12 +247,18 @@
 kernel void foo1(global unsigned int *buf)
 {
   ndrange_t n;
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-error {{use of declaration 'get_kernel_max_sub_group_size_for_ndrange' requires cl_khr_subgroups or __opencl_c_subgroups support}}
+

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

2022-04-01 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman created this revision.
aaron.ballman added reviewers: jyknight, eli.friedman, hubert.reinterpretcast, 
erichkeane, clang-language-wg.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added a project: clang.

Functions without prototypes in C (also known as K C functions) were 
introduced into C89 as a deprecated feature and C2x is now reclaiming that 
syntax space with different semantics. However, Clang's `-Wstrict-prototypes` 
diagnostic is off-by-default (even in pedantic mode) and does not suffice to 
warn users about issues in their code.

This patch changes the behavior of `-Wstrict-prototypes` to only diagnose 
declarations and definitions which are not going to change behavior in C2x 
mode, and enables the diagnostic in `-pedantic` mode. The diagnostic is now 
specifically about the fact that the feature is deprecated.

It also adds `-Wdeprecated-non-prototype`, which is grouped under 
`-Wstrict-prototypes` and diagnoses declarations or definitions which will 
change behavior in C2x mode. This diagnostic is enabled by default because the 
risk is higher for the user to continue to use the deprecated feature.

A few things to note:

- The RFC for this can be found at: 
https://discourse.llvm.org/t/rfc-enabling-wstrict-prototypes-by-default-in-c
- There is some awkward overlap between the diagnostics, but the best time to 
diagnose a function type without a prototype is when forming the function type, 
but we don't know whether the behavior will change in C2x at that point. So we 
alter the behavior sometimes depending on whether `-Wstrict-prototypes` is 
enabled in an effort to keep the diagnostics understandable and not too chatty.
- There will be another patch for handling call site behavior as a follow-up.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122895

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/CodeGen/2009-06-01-addrofknr.c
  clang/test/FixIt/fixit.c
  clang/test/Parser/declarators.c
  clang/test/Parser/knr_parameter_attributes.c
  clang/test/Parser/opencl-kernel.cl
  clang/test/Parser/traditional_arg_scope.c
  clang/test/Sema/arg-duplicate.c
  clang/test/Sema/block-return.c
  clang/test/Sema/implicit-decl.c
  clang/test/Sema/knr-def-call.c
  clang/test/Sema/knr-variadic-def.c
  clang/test/Sema/vfprintf-valid-redecl.c
  clang/test/Sema/warn-deprecated-non-prototype.c
  clang/test/Sema/warn-missing-prototypes.c
  clang/test/Sema/warn-strict-prototypes.c
  clang/test/Sema/warn-strict-prototypes.m
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
  clang/test/SemaOpenCL/func.cl

Index: clang/test/SemaOpenCL/func.cl
===
--- clang/test/SemaOpenCL/func.cl
+++ clang/test/SemaOpenCL/func.cl
@@ -43,9 +43,9 @@
 #endif
 
 // Expect no diagnostics for an empty parameter list.
-void bar();
+void bar(); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 
-void bar()
+void bar() // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
 {
   // declaring a function pointer is an error
   void (*fptr)(int);
Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -11,7 +11,7 @@
 
 typedef struct {int a;} ndrange_t;
 // Diagnostic tests for different overloads of enqueue_kernel from Table 6.13.17.1 of OpenCL 2.0 Spec.
-kernel void enqueue_kernel_tests() {
+kernel void enqueue_kernel_tests(void) {
   queue_t default_queue;
   unsigned flags = 0;
   QUALS ndrange_t ndrange;
@@ -169,7 +169,7 @@
 }
 
 // Diagnostic tests for get_kernel_work_group_size and allowed block parameter types in dynamic parallelism.
-kernel void work_group_size_tests() {
+kernel void work_group_size_tests(void) {
   void (^const block_A)(void) = ^{
 return;
   };
@@ -223,16 +223,22 @@
 kernel void foo(global unsigned int *buf)
 {
   ndrange_t n;
-  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){});
+  // FIXME: this should be diagnosed as a block instead of a function, but
+  // block literals don't track the ^ as part of their declarator.
+  buf[0] = get_kernel_max_sub_group_size_for_ndrange(n, ^(){}); // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}}
   buf[0] = get_kernel_max_sub_group_size_for_ndrange(0, ^(){}); // expected-error{{illegal call to 'get_kernel_max_sub_group_size_for_ndrange', expected 'ndrange_t' argument type}}
+// expected-warning@-1 {{a

51 matches

Mail list logo