[clang] False line number in a function definition with "void" parameter

[Jac1494]

Summary

This patch fixes false line number in a function definition with "void" parameter.
For more details PR46417.

[riccibruno]

clang/test/Sema/void-argument.cpp

1 ↗

(On Diff #280963)

These kinds of tests are typically done with a -verify test. See the other tests in Sema/ for examples.

[Jac1494]

clang/test/Sema/void-argument.cpp

1 ↗

(On Diff #280963)

we are testing here line number rather than diagnostic.

[riccibruno]

clang/test/Sema/void-argument.cpp

1 ↗

(On Diff #280963)

-verify implicitly checks line numbers. Even better: you don't have to hard-code the specific line numbers into the test.

[Jac1494]

Addressed @riccibruno comments.Thanks

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

5112

Are you sure that all parameters will have a valid IdentLoc? I am worried about the common case where the void parameter is unnamed.

clang/test/Sema/void-argument.cpp

9 ↗

(On Diff #281202)

I'd like to see some additional testing:

void foo(
  int a,
  void,
  int b
);

void bar(
  void,
  ...
);

struct S {
  S(
    void,
    void
  );
};

[Jac1494]

Address @aaron.ballman review comments.
I have added your test case that is passing as well.
This patch covers this scenario also.
Thanks @aaron.ballman

[aaron.ballman]

LGTM with a small nit about the tests, though I'm still surprised IdentLoc is valid even when there's no identifier present. :-)

clang/test/Sema/void-unnamed.cpp

1 ↗

(On Diff #281644)

I think these test should be combined with the other ones (no real need for separate files to test the same functionality, as that causes tests to be a bit slower to run for no real gain).

[Jac1494]

Hi @aaron.ballman ,
Address your review comments.
Thank you for accepting this. I don't have commit access please commit this.
Thanks.

[riccibruno]

In D84678#2184687, @Jac1494 wrote:

Hi @aaron.ballman ,
Address your review comments.
Thank you for accepting this. I don't have commit access please commit this.
Thanks.

As discussed with Aaron on IRC I can commit it for you. To do that I need a name and an email for the attribution.
But first move the test to the already existing test/Sema/void_arg.c which already test this diagnostic.

[riccibruno]

In D84678#2187747, @riccibruno wrote:

In D84678#2184687, @Jac1494 wrote:

Hi @aaron.ballman ,
Address your review comments.
Thank you for accepting this. I don't have commit access please commit this.
Thanks.

As discussed with Aaron on IRC I can commit it for you. To do that I need a name and an email for the attribution.
But first move the test to the already existing test/Sema/void_arg.c which already test this diagnostic.

Ah, it's a C++ test. Then put it into test/SemaCXX/

[Jac1494]

Hi @riccibruno,
Address your review comment.Add please use
Name :- Jaydeep Chauhan
Mail id:- [e-mail]
Thanks

clang/lib/Sema/SemaType.cpp

case DeclaratorChunk::Function: {
         for (unsigned i = 0, e = FTI.NumParams; i != e; ++i) {
           ParmVarDecl *Param = cast<ParmVarDecl>(FTI.Params[i].Param);
           QualType ParamTy = Param->getType();
           assert(!ParamTy.isNull() && "Couldn't parse type?");
 
           // Look for 'void'.  void is allowed only as a single parameter to a
           // function with no other parameters (C99 6.7.5.3p10).  We record
           // int(void) as a FunctionProtoType with an empty parameter list.
           if (ParamTy->isVoidType()) {
             // If this is something like 'float(int, void)', reject it.  'void'
             // is an incomplete type (C99 6.2.5p19) and function decls cannot
             // have parameters of incomplete type.
             if (FTI.NumParams != 1 || FTI.isVariadic) {
-              S.Diag(DeclType.Loc, diag::err_void_only_param);
+              S.Diag(FTI.Params[i].IdentLoc, diag::err_void_only_param);
               ParamTy = Context.IntTy;
               Param->setType(ParamTy);
             } else if (FTI.Params[i].Ident) {
               // Reject, but continue to parse 'int(void abc)'.
               S.Diag(FTI.Params[i].IdentLoc, diag::err_param_with_void_type);
               ParamTy = Context.IntTy;
               Param->setType(ParamTy);
             } else {

clang/test/SemaCXX/void-argument.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void fun(
+    void a, // expected-error{{'void' must be the first and only parameter if specified}}
+    double b,
+    int c,
+    void d, // expected-error{{'void' must be the first and only parameter if specified}}
+    int e,
+    void f) // expected-error{{'void' must be the first and only parameter if specified}}
+{}
+
+void foo(
+    int a,
+    void, // expected-error{{'void' must be the first and only parameter if specified}}
+    int b);
+
+void bar(
+    void, // expected-error{{'void' must be the first and only parameter if specified}}
+    ...);
+
+struct S {
+  S(
+      void,  // expected-error{{'void' must be the first and only parameter if specified}}
+      void); // expected-error{{'void' must be the first and only parameter if specified}}
+};