-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[C99] Claim conformance to WG14 N717 #87228
Conversation
This was the paper that added Universal Character Names to C.
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesThis was the paper that added Universal Character Names to C. Full diff: https://github.com/llvm/llvm-project/pull/87228.diff 3 Files Affected:
|
✅ With the latest revision this PR passed the Python code formatter. |
clang/test/C/C99/n717.c
Outdated
// | ||
// We use a python script to generate the test contents for the large ranges | ||
// without edge cases. | ||
// RUN: %python %S/n717.py >%t.inc | ||
// RUN: %clang_cc1 -verify -std=c99 -Wno-unicode-whitespace -Wno-unicode-homoglyph -Wno-unicode-zero-width -Wno-mathematical-notation-identifier-extension %t.inc | ||
|
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.
This seem excessive, can we just test at the boundaries at the range?
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.
Yeah, I was worried about this being a bit excessive (the test takes about 2s to run on my machine with a debug build, so it's a heavier test than usual but not the worst in terms of performance). But it also seems like this is something we can test exhaustively to ensure we don't have special handling for some particular UCN that causes issues (like assertions) because some of these diagnostics are generated by values in the middle of the range rather than at the edges.
That said, if you have a suggestion for a better approach, I'm game!
/* WG14 N717: Clang 17 | ||
* Extended identifiers | ||
*/ |
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.
Do you know what was not supported before?
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.
UCNs weren't supported in C89 but were added in C99, so this is testing UCNs as specified in https://www.open-std.org/jtc1/sc22/wg14/www/docs/n717.htm but based on whatever was in the final text of the C99 standard (so it incorporates changes from NB comments, etc).
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 mean, before clang 17. I don't remember we changed anything there (for C99) in a while.
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! We were missing some diagnostics: https://godbolt.org/z/hEKsEKqzT
This removes the Python script as overkill and adds an additional test case.
Corentin pointed out that UCNs are converted in Phase 5 when they're not part of an identifier, so I can't use my macro trick to test them. Also added some tests for edge cases. Still claiming full support as of Clang 17 for this despite there being some rough edges.
This removes the FIXME, which is better handled via filing an issue.
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.
Thanks Aaron for putting up with all my offline requests. LGTM
Thank you for all the excellent suggestions, this was a lot harder to test correctly than I realized at first. |
This was the paper that added Universal Character Names to C.