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

Handle escaped string literals (like "\tfoo\n") more correctly. #1073

Merged
merged 2 commits into from Nov 7, 2019

Conversation

@lgritz
Copy link
Member

lgritz commented Nov 6, 2019

"Unescape" all string literals in .osl and .oso as they are parsed,
then "escape" them properly for output. We weren't being careful and
consistent before. This led to at least two subtle behavioral errors:

  1. oslinfo would appear to add an extra set of escape characters to
    metadata values that contained escape codes.

  2. If s = "\n", then strlen(s) was 1 if s was a local variable, but was
    incorrectly 2 (wowza!) if s was a parameter!

@fpsunflower

This comment has been minimized.

Copy link
Collaborator

fpsunflower commented Nov 6, 2019

LGTM!

@lgritz

This comment has been minimized.

Copy link
Member Author

lgritz commented Nov 6, 2019

Hold on, I discovered one more edge case I don't get quite right. Will fix today and post an update...

"Unescape" all string literals in .osl and .oso as they are parsed,
then "escape" them properly for output.  We weren't being careful and
consistent before. This led to at least two subtle behavioral errors:

1. oslinfo would appear to add an extra set of escape characters to
   metadata values that contained escape codes.

2. If s = "\n", then strlen(s) was 1 if s was a local variable, but was
   incorrectly 2 (wowza!) if s was a parameter!
@lgritz lgritz force-pushed the lgritz:lg-escape branch from 1de4c5a to 6e068f0 Nov 7, 2019
@lgritz

This comment has been minimized.

Copy link
Member Author

lgritz commented Nov 7, 2019

Slight update pushed.

Also note that a semi-related bug (at least inasmuch as it also concerned certain escaped strings) was found on the OIIO Strutil side: OpenImageIO/oiio#2386

@fpsunflower

This comment has been minimized.

Copy link
Collaborator

fpsunflower commented Nov 7, 2019

LGTM

@lgritz lgritz merged commit 3a06e88 into imageworks:master Nov 7, 2019
5 of 7 checks passed
5 of 7 checks passed
Linux VFX 2018-ish: gcc6, C++11, llvm7, OIIO 2.0, sse2, exr2.2
Details
Linux VFX 2019-ish: gcc6, C++14, llvm8, OIIO master, sse4, exr2.3
Details
Linux gcc8, C++14, llvm9, oiio master, avx2, exr2.4
Details
Linux bleeding edge: gcc8, C++17, llvm9, oiio master, avx2, exr2.4
Details
Mac py37
Details
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@lgritz lgritz deleted the lgritz:lg-escape branch Nov 7, 2019
lgritz added a commit to lgritz/OpenShadingLanguage that referenced this pull request Nov 7, 2019
…eworks#1073)

"Unescape" all string literals in .osl and .oso as they are parsed,
then "escape" them properly for output.  We weren't being careful and
consistent before. This led to at least two subtle behavioral errors:

1. oslinfo would appear to add an extra set of escape characters to
   metadata values that contained escape codes.

2. If s = "\n", then strlen(s) was 1 if s was a local variable, but was
   incorrectly 2 (wowza!) if s was a parameter!

* Add fixed version of parse_string (OpenImageIO/oiio#2386)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.