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

Fix output of dxbc assembly. #719

Merged
merged 1 commit into from Nov 13, 2018

Conversation

jsmall-zzz
Copy link
Contributor

  • Fix bug outputting dxbc assembly
  • Make the disassembly methods returns SlangResult and String as last output param so as to make error case clear.

* Make the disassembly methods returns SlangResult and String as last output param so as to make error case clear.
Copy link
Contributor

@tangent-vector tangent-vector left a comment

Choose a reason for hiding this comment

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

This seems fine but I am somehow missing what the bug is in the original code (admittedly I’m a bit under the weather).

{
stringOut = String();
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally like to name output or input/output parameters with an out or io prefix, respectively. I also don’t really like output reference parameters because they aren’t visible at the call site, but I’m okay with them when people find using pointer parameters less savory.

I’m also not sure about the convention here of initializing the output parameter at the start. I would usually expect a model where an output parameter is only written after all preconditions have been checked, so that in case of an error the output doesn’t get touched...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the reference parameter output is a bit problematic, can change to use pointer/s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug is that the dxbc disassembly is not written to the stringOut param. It's set on result and then thrown away. It originated when the SharedLibrary support was added.

@jsmall-zzz jsmall-zzz merged commit cfb1f61 into shader-slang:master Nov 13, 2018
@jsmall-zzz jsmall-zzz deleted the hotfix/dxbc-disassembly branch November 29, 2018 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants