csyonghe / Spire

Other
174 stars 22 forks source link

Initial step toward HLSL code generation. #41

Closed tangent-vector closed 7 years ago

tangent-vector commented 7 years ago

Some notes/caveats:

And the big one:

hofstee commented 7 years ago

If you prepend/append a string onto user defined variables you have a guarantee that anything not starting/ending with the string is fresh.

Alternatively you could rename all variables.

Both cases you mangle the readability a bit.

tangent-vector commented 7 years ago

Yes, adding a unique prefix/suffix to all user-defined symbols would be one option, but it would conflict with our goal of making the output code be as readable as possible. It would be especially bad if we renamed user functions or types, if we ever want users to be able to generate GLSL/HLSL from Spire that they can use with hand-written GLSL/HLSL.

A good long-term proposal might be to reserve some specific prefix like S<digits>_* and then ensure that all auto-generated symbols use that prefix (with unique digits generated from a counter), and then for any user-defined symbol that starts with the prefix (not expected to be common), or which conflicts with a keyword or other reserved name for the target, have a way of escaping it. A simple approach would take a symbol like S123_foo and make it S1_US123_foo, so that the S<digits>_U* prefix is reserved for escaped user-defined names.

I'd argue that any such policy is orthogonal to this pull request, since it doesn't look like the existing code for GLSL code generation was 100% robust against name conflicts (unless I missed something).

csyonghe commented 7 years ago

My solution to this is just to maintain a set holding currently used identifiers, and if a new identifier is already in the set, I will append a postfix. The naming will then be processed by EscapeDoubleUnderscore() function defined in Naming.h to make sure there aren't two consecutive underscores in the name.

tangent-vector commented 7 years ago
  1. Can you point me to examples of how you use such a set in the existing GLSL codegen? I assume the data structure is already there, and I just need to use it.
  2. Are you asking that I resolve this issue before you'll accept the PR, or are we just having an unrelated conversation. :)

Please let me know if there is anything you want me to fix before this PR is okay to merge. Otherwise I'll probably start working on the next steps in a distinct branch while I wait for this to get merged.

csyonghe commented 7 years ago

No, I am not asking you to resolve this issue now. I am testing my refined API and integrating your pull request now.

From: Tim Foley [mailto:notifications@github.com] Sent: Thursday, November 10, 2016 3:30 PM To: csyonghe/Spire Spire@noreply.github.com Cc: Yong He yonghe@outlook.com; Comment comment@noreply.github.com Subject: Re: [csyonghe/Spire] Initial step toward HLSL code generation. (#41)

  1. Can you point me to examples of how you use such a set in the existing GLSL codegen? I assume the data structure is already there, and I just need to use it.
  2. Are you asking that I resolve this issue before you'll accept the PR, or are we just having an unrelated conversation. :)

Please let me know if there is anything you want me to fix before this PR is okay to merge. Otherwise I'll probably start working on the next steps in a distinct branch while I wait for this to get merged.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/csyonghe/Spire/pull/41#issuecomment-259798990, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ACh4haLVL5FzB2W1bl6o_65Peb95m1HBks5q837jgaJpZM4Ku6_0.