-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Don't emit double underscores in SkSL backend. #36131
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
This isn't a reservation in SKSL. Section 3.6 of the GLSL spec has this restriction. "In SKSL is leaving the identifier as-is when transpiling to GLSL. This may lead to a situation where a valid SKSL shader doesn't generate valid GLSL. Perhaps this is also a Skia bug. But yeah, fixing it here sounds good as we'll side-step this issue in ImpellerC. EDIT: Oof, what a mess. The 4.60 spec clarifies: "Defining or undefining such a name in a shader does not itself result in an error, but may result in unintended behaviors that stem from having multiple definitions of the same name." I think the Android emulator is incorrect in treating this as an error. |
|
Ahh. Thanks for clarifying and bringing it up with the Skia team @chinmaygarde. |
|
This is an oversight in SkSL. We should reject this user code. I'll get in a fix. |
This comment was marked as outdated.
This comment was marked as outdated.
|
I have an SkSL fix on deck at http://review.skia.org/580587 SkSL will continue to allow identifiers with multiple underscores. The GLSL backend will perform mangling as needed to make sure that we emit valid GLSL identifiers even if the original identifier contained It should be safe for Impeller to ignore this issue going forward, as SkSL will paper over it for you. |
|
The SkSL mangling fix has been submitted. |
Fixes flutter/flutter#111379. Identifiers with double-underscore in them are reserved in SkSL, but it looks like this is only enforced when targeting an Android emulator.