-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[ConstraintSystem] [NFC] add tests for label matching diagnostics #31934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@swift-ci please smoke test |
test/Constraints/diagnostics.swift
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if since there are quite a few tests specifically about label matching, is it a good idea to separate them in their own label matching file? This is just an idea because I see this diagnostics file is general, and maybe separate label matching diagnostics can make it easier to organize things :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea.
Test cases about matching of call arguments are also in test/Constraints/keyword_arguments.swift.
I don't know how other authors distinguish them.
I choice diagnostics.swift for what has interest in unlabeled arguments because they has no keyword.
If we can make a big change,
I want to rename keyword_arguments.swift to argument_matching.swift and
move some cases in diagnostics.swift to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can, I want to split it more like:
argument_matching_labeled.swiftargument_matching_unlabeled.swiftargument_matching_variadic.swiftargument_matching_defaulted.swiftargument_matching_trailing_closure.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since all fall in the context of argument_matching I think only one file is already fine.
I don't know exactly what are the rules to this in the test suit, but to me, it seems reasonable to separate in, for example, an argument_matching.swift and have the subcases e.g. labeled, unlabeled, variadic.. divided inside the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think extracting all this into test/Constraints/argument_matching.swift would be just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucianoPAlmeida @xedin Thanks to comment.
OK I will make new patch to create argument_matching.swift.
test/Sema/object_literals_osx.swift
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it as is since this changes is unrelated to other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
I will move it to new another patch.
|
I will make two new other patches.
After these works will be finished, I will adjust this patch to make it only add new tests. |
|
Sounds like a plan! |
It bakes current behavior for reviewing changes in future.
|
@LucianoPAlmeida @xedin I moved test cases to @swift-ci please smoke test |
| f(aa: 0, 1, cc: 2, dd: 3, ee: 4, ff: 5) | ||
| // expected-error@-1 {{extraneous argument label 'aa:' in call}} {{5-9=}} {{none}} | ||
|
|
||
| // 1 ooo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question, what is 'ooo'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks :)
|
LGTM! @omochi :) |
This patch add test cases for label matching diagnostics.
This haven't changes for compiler.
This is a part of following my other project.
#30444
I am planning to make many changes of diagnostics there particular corner cases.
It is useful to bake current behavior to repository as test cases
before reviewing my that project.