Page MenuHomePhabricator

Fix highlighting issue with _complex and initialization list with more than 2 items
ClosedPublic

Authored by chaitanyav on Mar 21 2023, 1:18 AM.

Diff Detail

Event Timeline

chaitanyav created this revision.Mar 21 2023, 1:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2023, 1:18 AM
chaitanyav requested review of this revision.Mar 21 2023, 1:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2023, 1:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
test.cpp:2:34: error: excess elements in scalar initializer
_Complex double dd = {1.0, 2.0 , 3.0};
                                 ^~~
test.cpp:3:32: error: excess elements in scalar initializer
_Complex float fd = {1.0, 2.0, 3.0, 4.0, 5.0};
                               ^~~
test.cpp:4:42: error: excess elements in scalar initializer
constexpr _Complex double d = {1.0, 2.0, 3.0, 45};
                                         ^~~
test.cpp:11:23: error: no viable conversion from 'foo' to '_Complex double'
_Complex double ds = {f, 1.0, b};
                      ^
test.cpp:13:28: error: no viable conversion from 'foo' to 'double'
_Complex double fg = {1.0, f};
                           ^
test.cpp:15:33: error: excess elements in scalar initializer
_Complex double gg = {1.0, 2.0, f};
                                ^
6 errors generated.

Tested various scenarios to ensure the behavior is as expected. Please review

I think the problem is that in https://github.com/llvm/llvm-project/blob/25ca26e0da2e1f80d62f71807828762691a049ac/clang/lib/Sema/SemaInit.cpp#L1532-L1541, the code checks for Index != 2 and then handles it as a single scalar initializer.

It will fall through to scalar type to do field wise initialization

_Complex double gx = {1.0};

_Complex double dx = {};

I think i get what you are saying, we can directly say that it has excess elements without having to do the field wise initialization. Let me look into that

Only proceed with scalar initialization if number of elements is less than 2

test outputs for C, C++

_Complex float invalid2 = { 1, 2, 3 };
                                  ^
test.cpp:2:34: error: excess elements in scalar initializer
_Complex double dd = {1.0, 2.0 , 3.0};
                                 ^~~
test.cpp:3:32: error: excess elements in scalar initializer
_Complex float fd = {1.0, 2.0, 3.0, 4.0, 5.0};
                               ^~~
test.cpp:4:42: error: excess elements in scalar initializer
constexpr _Complex double d = {1.0, 2.0, 3.0, 45};
                                         ^~~
test.cpp:11:23: error: no viable conversion from 'foo' to 'double'
_Complex double ds = {f, 1.0, b};
                      ^
test.cpp:13:28: error: no viable conversion from 'foo' to 'double'
_Complex double fg = {1.0, f};
                           ^
test.cpp:15:33: error: excess elements in scalar initializer
_Complex double gg = {1.0, 2.0, f};
                                ^
7 errors generated.
test.c:1:35: warning: excess elements in scalar initializer [-Wexcess-initializers]
_Complex float invalid2 = { 1, 2, 3 };
                                  ^
test.c:2:34: warning: excess elements in scalar initializer [-Wexcess-initializers]
_Complex double dd = {1.0, 2.0 , 3.0};
                                 ^~~
test.c:3:32: warning: excess elements in scalar initializer [-Wexcess-initializers]
_Complex float fd = {1.0, 2.0, 3.0, 4.0, 5.0};
                               ^~~
test.c:4:38: warning: excess elements in scalar initializer [-Wexcess-initializers]
const _Complex double d = {1.0, 2.0, 3.0, 45};
                                     ^~~
test.c:11:23: error: initializing 'double' with an expression of incompatible type 'struct foo'
_Complex double ds = {f, 1.0, b};
                      ^
test.c:13:28: error: initializing 'double' with an expression of incompatible type 'struct foo'
_Complex double fg = {1.0, f};
                           ^
test.c:15:33: warning: excess elements in scalar initializer [-Wexcess-initializers]
_Complex double gg = {1.0, 2.0, f};
                                ^
test.c:19:22: error: scalar initializer cannot be empty
_Complex double dx = {};

Please review this.

LGTM but I'll wait for @aaron.ballman to maybe comment on the diagnostic differences in C.

Thanks for this!

One thing that's missing is test coverage for the highlight itself, as that was what the original issue was about. You should add a test that uses -fcaret-diagnostics-max-lines and FileCheck so that you can test that the caret shows up where expected (along the lines of https://github.com/llvm/llvm-project/blob/main/clang/test/Misc/caret-diags-multiline.cpp).

You should also add a release note about the fix to clang/docs/ReleaseNotes.rst

clang/test/Sema/complex-init-list.c
38–39

This makes it a bit easier to see that there are two diagnostics being fired (even though we're not consistently doing that in this test file).

55–56

Added tests to check for the right diagnostic messages and highlighting at correct place
Update the release docs about the fix

chaitanyav marked an inline comment as done.Mar 22 2023, 3:17 PM

@aaron.ballman Please review.

clang/test/Sema/complex-init-list.c
38–39

Yeah, one because its an extension and other because it has excess elements. Checked this file added that on lines its applicable.

chaitanyav marked an inline comment as done.Mar 22 2023, 9:09 PM

I reported the flang failures https://github.com/llvm/llvm-project/issues/61634 and it was reverted. How can i trigger a new build with TOT?

The precommit CI failures are unrelated to your changes (the Debian one is related to flang failures and the libcxx one seems to be related to infrastructure), so no need to worry about them.

clang/test/Sema/complex-init-list.c
38–39

I'd still like to see these two changes made (where we use a line continuation character to put the diagnostic messages on their own lines instead of mashed together on one long line).

chaitanyav marked an inline comment as not done.

one diagnostic per line when there are multiple diagnostics expected

aaron.ballman accepted this revision.Mar 23 2023, 12:36 PM

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

This revision is now accepted and ready to land.Mar 23 2023, 12:36 PM
chaitanyav marked 3 inline comments as done.Mar 23 2023, 12:48 PM

Thank you, I will commit myself. Should i wait for the build to finish?. this will be my first direct commit to repo.

This revision was landed with ongoing or failed builds.Mar 23 2023, 2:22 PM
This revision was automatically updated to reflect the committed changes.

Thank you, I will commit myself. Should i wait for the build to finish?. this will be my first direct commit to repo.

Sorry for not getting back to you sooner. In general it's good to wait for precommit CI to finish just to be sure. Congrats on your first direct commit! :-)