Skip to content
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

Improve entry point lookup function documentation #6451

Merged
merged 6 commits into from
Feb 27, 2025

Conversation

aleino-nv
Copy link
Collaborator

@aleino-nv aleino-nv commented Feb 25, 2025

  • Add a unit test for the case where the code does not specify entry points, but they are instead specified via the API
  • Make the entry point lookup logic check the information in the options
  • Add user guide documentation that explains which lookup function to use when there is no [shader(...)] attribute on an entry point
  • Add a documentation comment in slang.h mentioning the restriction on findEntryPointByName

#6452

See also issue #4760.

@aleino-nv aleino-nv force-pushed the aleino/entry-point-lookup-bug branch from d25be96 to 2165c51 Compare February 25, 2025 08:59
@aleino-nv aleino-nv force-pushed the aleino/entry-point-lookup-bug branch from 2165c51 to 46ee809 Compare February 25, 2025 14:04
@aleino-nv aleino-nv added the pr: non-breaking PRs without breaking changes label Feb 25, 2025
@aleino-nv aleino-nv marked this pull request as ready for review February 25, 2025 14:09
@aleino-nv aleino-nv requested a review from a team as a code owner February 25, 2025 14:09
Copy link
Collaborator

@cheneym2 cheneym2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but someone else should approve

@csyonghe
Copy link
Collaborator

There is no bug to be fixed here. If the source code did not specify [shader()] attribute, the host side should call findAndCheckEntryPoint.

@aleino-nv
Copy link
Collaborator Author

aleino-nv commented Feb 26, 2025

There is no bug to be fixed here. If the source code did not specify [shader()] attribute, the host side should call findAndCheckEntryPoint.

@csyonghe IIUC the user guide just states that [shader(...)] should not be needed: #4760 (comment). I don't find any mentions of findAndCheckEntryPoint.
It seems pretty obscure to me that leaving off [shader(...)] would mean you need to call one particular API rather than another similar one.

I ran into this because several tests don't specify [shader(...)], and those fail to look up entry points when I migrate away from the deprecated build request API.

@aleino-nv aleino-nv changed the title Add unit test for entry point lookup bug Fix entry point lookup bug Feb 26, 2025
@csyonghe
Copy link
Collaborator

@aleino-nv
Copy link
Collaborator Author

At least I see this doc comment for findAndCheckEntryPoint:

    /// Find and validate an entry point by name, even if the function is
    /// not marked with the `[shader("...")]` attribute.
    virtual SLANG_NO_THROW SlangResult SLANG_MCALL findAndCheckEntryPoint(...)

The findEntryPointByName function should probably also have a doc comment that mentions this, and the user guide should probably direct the user to findAndCheckEntryPoint.

I will change the patch accordingly.

@aleino-nv aleino-nv force-pushed the aleino/entry-point-lookup-bug branch from 46ee809 to a76bfdf Compare February 26, 2025 09:24
@aleino-nv aleino-nv changed the title Fix entry point lookup bug Improve entry point lookup function documentation Feb 26, 2025
@aleino-nv aleino-nv requested a review from cheneym2 February 26, 2025 09:29
@aleino-nv
Copy link
Collaborator Author

@csyonghe I added a mention in the user guide and slang.h as well. I think it's worth mentioning there since it would have saved me some time...

csyonghe
csyonghe previously approved these changes Feb 27, 2025
@csyonghe
Copy link
Collaborator

looks good! needs a formatting fix.

@aleino-nv
Copy link
Collaborator Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@aleino-nv aleino-nv requested a review from csyonghe February 27, 2025 06:19
@aleino-nv aleino-nv enabled auto-merge (squash) February 27, 2025 06:19
@csyonghe csyonghe disabled auto-merge February 27, 2025 06:42
@csyonghe csyonghe enabled auto-merge (squash) February 27, 2025 06:42
@csyonghe csyonghe disabled auto-merge February 27, 2025 06:42
@csyonghe csyonghe merged commit 525c2ac into shader-slang:master Feb 27, 2025
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants