-
Notifications
You must be signed in to change notification settings - Fork 438
Deprecate operator & for getting addresses
#10206
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
base: master
Are you sure you want to change the base?
Changes from all commits
c2cf652
80fd64f
4eaf822
078f367
af9bc26
bf5c81b
2933212
8450b93
1c4b8f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3782,6 +3782,25 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr) | |||||||||||||||||
| if (funcDeclRefExpr) | ||||||||||||||||||
| funcDeclBase = as<FunctionDeclBase>(funcDeclRefExpr->declRef.getDecl()); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (funcDeclRefExpr) | ||||||||||||||||||
| { | ||||||||||||||||||
| auto knownBuiltinAttrDeclRef = getDeclRef(m_astBuilder, funcDeclRefExpr) | ||||||||||||||||||
| .getDecl() | ||||||||||||||||||
| ->findModifier<KnownBuiltinAttribute>(); | ||||||||||||||||||
| if (auto knownBuiltinAttr = as<KnownBuiltinAttribute>(knownBuiltinAttrDeclRef)) | ||||||||||||||||||
|
Comment on lines
+3787
to
+3790
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Redundant cast and misleading variable name Two issues with this pattern:
auto knownBuiltinAttr = funcDeclRef.getDecl()->findModifier<KnownBuiltinAttribute>();
if (knownBuiltinAttr) { ... }Suggested fix: Simplify to match the existing pattern:
Suggested change
Comment on lines
+3785
to
+3790
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Redundant cast, misleading variable name, and missing null guard Three issues in this block:
Suggested fix (aligning with the existing pattern at lines 3879–3900): auto funcDeclRef = getDeclRef(m_astBuilder, funcDeclRefExpr);
if (funcDeclRef)
{
if (auto knownBuiltinAttr =
funcDeclRef.getDecl()->findModifier<KnownBuiltinAttribute>())
{
if (auto constantIntVal = as<ConstantIntVal>(knownBuiltinAttr->name))
{
if (constantIntVal->getValue() ==
(int)KnownBuiltinDeclName::OperatorAddressOf)
{
getSink()->diagnose(
Diagnostics::AddressOfOperatorNotSupported{.expr = invoke});
return CreateErrorExpr(invoke);
}
}
}
} |
||||||||||||||||||
| { | ||||||||||||||||||
| if (auto constantIntVal = as<ConstantIntVal>(knownBuiltinAttr->name)) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (constantIntVal->getValue() == | ||||||||||||||||||
| (int)KnownBuiltinDeclName::OperatorAddressOf) | ||||||||||||||||||
| { | ||||||||||||||||||
| getSink()->diagnose( | ||||||||||||||||||
|
Comment on lines
+3796
to
+3797
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: No early return after diagnosis leads to cascading errors After emitting error 30087, execution falls through to the parameter validation loop (~line 3766+). For expressions like
The first error is sufficient — the operator is unconditionally rejected. The subsequent errors are about specific argument constraints of an operator that no longer exists, which is noise. Suggested fix: getSink()->diagnose(
Diagnostics::AddressOfOperatorNotSupported{.expr = invoke});
return CreateErrorExpr(invoke);This also makes the existing |
||||||||||||||||||
| Diagnostics::AddressOfOperatorNotSupported{.expr = invoke}); | ||||||||||||||||||
|
Comment on lines
+3797
to
+3798
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: No early return after E30087 — cascading errors and continued IR lowering After emitting E30087, execution falls through to the parameter validation loop (line 3804+), which produces additional confusing diagnostics for the same expression. The updated More significantly, the Example: Only the first error is the actual issue; the rest are noise. Suggested fix: getSink()->diagnose(
Diagnostics::AddressOfOperatorNotSupported{.expr = invoke});
return CreateErrorExpr(invoke);This follows the existing pattern in this file and suppresses both cascading diagnostics and IR lowering. |
||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+3785
to
+3799
|
||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+3785
to
+3800
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Bug: Multiple existing tests use This unconditional check fires for ALL uses of
Suggestion: Each of these tests needs to either migrate from |
||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| Index paramCount = funcType->getParamCount(); | ||||||||||||||||||
|
|
||||||||||||||||||
| for (Index pp = 0; pp < paramCount; ++pp) | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1389,6 +1389,13 @@ err( | |||||
| span { loc = "expr:Expr", message = "Not allowed to take the address of an immutable object" } | ||||||
| ) | ||||||
|
|
||||||
| err( | ||||||
| "address-of-operator-not-supported", | ||||||
| 30087, | ||||||
| "address-of operator not supported", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Error message does not suggest replacement The message "the '&' operator for taking addresses is no longer supported in Slang" tells users what's wrong but not what to do instead. Since this is a breaking change, the error message is the primary migration guide most users will see. The PR description states internal code should use Suggestion: Include the replacement in the message: span { loc = "expr:Expr", message = "the '&' operator for taking addresses is no longer supported in Slang; use '__getAddress()' instead" } |
||||||
| span { loc = "expr:Expr", message = "the '&' operator for taking addresses is no longer supported in Slang" } | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Error message lacks migration guidance and docs will become stale The error message tells users what's wrong but not what to do about it. Since Additionally, several documentation pages still describe
These docs should be updated in this PR or a follow-up to avoid sending users to a now-broken feature. Suggested fix for the error message:
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Error message lacks migration guidance; documentation still describes The error message tells users what's wrong but not what to do instead. Since Additionally, this is a breaking change but the documentation is not updated in this PR:
Suggestion: Update the message to include the replacement: span { loc = "expr:Expr", message = "the '&' operator for taking addresses is no longer supported in Slang; use '__getAddress' instead" }And update the documentation files listed above to use |
||||||
| ) | ||||||
|
|
||||||
| err( | ||||||
| "cannot-specialize-generic-with-existential", | ||||||
| 33180, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| //DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK): -target spirv -entry computeMain -stage compute | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Documentation not updated for this breaking change Two documentation files still describe
Both files present Suggestion: Update these docs in the same PR or a companion PR. At minimum, swap the guidance so |
||
|
|
||
| RWStructuredBuffer<float> buffer; | ||
|
|
||
| [shader("compute")] | ||
| [numthreads(1,1,1)] | ||
| void computeMain(uint3 threadId : SV_DispatchThreadID) | ||
| { | ||
| // CHECK: error[E30087]: address-of operator not supported | ||
| float* p = &buffer[threadId.x]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Multiple existing tests use Beyond this new test and the updated
Suggestion: Migrate these tests to use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Gap: Limited test coverage and existing tests not updated Two concerns: 1. Existing tests using
These tests will now emit E30087 in addition to their expected errors. They may still pass due to 2. Limited scenario coverage: The new test only covers buffer element access ( |
||
| } | ||
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.
🟡 Gap: Missing null guard and redundant
as<>castThe chain
getDeclRef(m_astBuilder, funcDeclRefExpr).getDecl()->findModifier<KnownBuiltinAttribute>()dereferencesgetDecl()without a null check. Compare with the existing pattern at line 3841-3848 (inside the parameter loop), which stores theDeclRefresult and checksif (funcDeclRef)before calling.getDecl()->findModifier<>(). WhilegetDecl()returning null is unlikely after successful overload resolution, the defensive pattern is used elsewhere in this same function.Additionally, the variable name
knownBuiltinAttrDeclRefis misleading — it holds aKnownBuiltinAttribute*(a modifier), not aDeclRef. The subsequentas<KnownBuiltinAttribute>(knownBuiltinAttrDeclRef)on line 3752 is redundant sincefindModifier<KnownBuiltinAttribute>()already returnsKnownBuiltinAttribute*.Suggestion: Follow the defensive pattern from line 3841+ and simplify: