Page MenuHomePhabricator

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

Authored by Jac1494 on Jul 27 2020, 10:16 AM.

Details

Summary

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

Diff Detail

Event Timeline

Jac1494 created this revision.Jul 27 2020, 10:16 AM
riccibruno added inline comments.Jul 27 2020, 3:01 PM
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 added inline comments.Jul 28 2020, 12:32 AM
clang/test/Sema/void-argument.cpp
1 ↗(On Diff #280963)

we are testing here line number rather than diagnostic.

riccibruno added inline comments.Jul 28 2020, 2:26 AM
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 updated this revision to Diff 281202.Jul 28 2020, 5:48 AM

Addressed @riccibruno comments.Thanks

aaron.ballman added inline comments.Jul 29 2020, 4:57 AM
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 updated this revision to Diff 281644.Jul 29 2020, 9:55 AM

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 accepted this revision.Jul 29 2020, 10:50 AM

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).

This revision is now accepted and ready to land.Jul 29 2020, 10:50 AM
Jac1494 updated this revision to Diff 281911.Jul 30 2020, 6:38 AM

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

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.

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 updated this revision to Diff 282263.Jul 31 2020, 10:52 AM

Hi @riccibruno,
Address your review comment.Add please use
Name :- Jaydeep Chauhan
Mail id:- jaydeepchauhan1494@gmail.com
Thanks