patch for bug id 2813534 "Escaped strings in RSL not working"

View: New views
8 Messages — Rating Filter:   Alert me  

patch for bug id 2813534 "Escaped strings in RSL not working"

by Ricardo Mayor :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Hello,
Find attached a patch for the bug "Escaped strings in RSL not working".
According
to ri specs, the supported escape string are

\n       linefeed (newline)
\r       carriage return
\t       horizontal tab
\b       backspace
\f       form feed
\\       backslash
\”       double quote
\ddd     character code ddd (octal)
\newline no character – both are ignored

Only the last one was currently supported. The attached patch adds support for all the cases.

Best regards

[escape_char.patch]

diff --git a/libs/shadervm/shadervm.cpp b/libs/shadervm/shadervm.cpp
index 225670a..a8d693e 100644
--- a/libs/shadervm/shadervm.cpp
+++ b/libs/shadervm/shadervm.cpp
@@ -1279,11 +1279,89 @@ void CqShaderVM::LoadProgram( std::istream* pFile )
  ( *pFile ) >> std::ws;
  char c;
  CqString s( "" );
+ bool escapeChar = false, escapeNumeric = false;
+ CqString capturedNumber( "" );
+
  pFile->get
  ();
- while ( ( c = pFile->get
- () ) != '"' )
- s += c;
+ while ( (( c = pFile->get() ) != '"') || escapeChar  )
+ {
+
+ if (escapeNumeric)
+ {
+ if(c < '0' or c > '9')
+ {
+ char number = strtoul(capturedNumber.c_str(), NULL, 8);
+ s+= number;
+ escapeChar = false;
+ escapeNumeric =  false;
+ capturedNumber = "";
+ }
+ }
+ if(escapeChar)
+ {
+ //Treatment for escape char
+ switch(c){
+ case 'n':
+ s += '\n';
+ escapeChar = false;
+ break;
+ case 'r':
+ s += '\r';
+ escapeChar = false;
+ break;
+ case 't':
+ s += '\t';
+ escapeChar = false;
+ break;
+ case 'b':
+ s += '\b';
+ escapeChar = false;
+ break;
+ case 'f':
+ s += '\f';
+ escapeChar = false;
+ break;
+ case '"':
+ s += '"';
+ escapeChar = false;
+ break;
+ case '\\':
+ s += '\\';
+ escapeChar = false;
+ break;
+ case '0':
+ case '1':
+ case '2':
+ case '3':
+ case '4':
+ case '5':
+ case '6':
+ case '7':
+ case '8':
+ case '9':
+
+ capturedNumber += c;
+ if (capturedNumber.length() == 3)
+ {
+ escapeNumeric = false;
+ escapeChar = false;
+ s +=  (char)strtoul(capturedNumber.c_str(), NULL, 8);
+ }
+ else
+ escapeNumeric = true;
+ break;
+ default :
+ escapeChar = false;
+ break;
+ }
+ }
+ else
+ if (c ==  '\\')
+ escapeChar = true;
+ else
+ s += c;
+ }
  AddString( s.c_str(), pProgramArea );
  }
  break;


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

Re: patch for bug id 2813534 "Escaped strings in RSL not working"

by Chris Foster-5 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Hi Ricardo,

On Sun, Sep 27, 2009 at 04:12:57PM +0100, Ricardo Mayor wrote:

> Hello,
> Find attached a patch for the bug "*Escaped strings in RSL not working".
> According *to ri specs, the supported escape string are
>
> \n       linefeed (newline)
> \r       carriage return
> \t       horizontal tab
> \b       backspace
> \f       form feed
> \\       backslash
> \"       double quote
> \ddd     character code ddd (octal)
> \newline no character ? both are ignored

I think these may be the escape sequences specified in the part of the RISpec
which deals with the RIB format, not the renderman shading language?

Here's what I see in my copy of the RISpec (around page 117):

" Strings are used to name external objects (texture maps, for
example). String literals (or
" constants) are enclosed in double quotes, as in the C language, and
may contain any of the
" standard C "escape sequences" (such as \n for newline or \" for a quote).

So actually I guess we should support a few more things which are in the C
language but not part of the RIB syntax:

\a
\v
\'
\?
\ooo
\x hh
\x hhh
(According to http://msdn.microsoft.com/en-us/library/h21280bw(VS.80).aspx)

I've also got some suggestions to make the code a bit easier to read.  To begin
with, please consider putting this chunk of code into its own function
(eg, LoadString() or whatever).  CqShaderVM::LoadProgram() is a monster
function already, let's not make it any huger!

Other suggestions follow:

> diff --git a/libs/shadervm/shadervm.cpp b/libs/shadervm/shadervm.cpp
> index 225670a..a8d693e 100644
> --- a/libs/shadervm/shadervm.cpp
> +++ b/libs/shadervm/shadervm.cpp
> @@ -1279,11 +1279,89 @@ void CqShaderVM::LoadProgram( std::istream* pFile )
>   ( *pFile ) >> std::ws;
>   char c;
>   CqString s( "" );
> + bool escapeChar = false, escapeNumeric = false;
> + CqString capturedNumber( "" );
> +
>   pFile->get
>   ();
> - while ( ( c = pFile->get
> - () ) != '"' )
> - s += c;
> + while ( (( c = pFile->get() ) != '"') || escapeChar  )
> + {
> +
> + if (escapeNumeric)
> + {
> + if(c < '0' or c > '9')
> + {
> + char number = strtoul(capturedNumber.c_str(), NULL, 8);
> + s+= number;
> + escapeChar = false;
> + escapeNumeric =  false;
> + capturedNumber = "";
> + }
> + }
> + if(escapeChar)
> + {
> + //Treatment for escape char
> + switch(c){

Please use opening braces on their own line for consistency with the rest of
the codebase.

> + case 'n':
> + s += '\n';
> + escapeChar = false;
> + break;
> + case 'r':
> + s += '\r';
> + escapeChar = false;
> + break;
> + case 't':
> + s += '\t';
> + escapeChar = false;
> + break;
> + case 'b':
> + s += '\b';
> + escapeChar = false;
> + break;
> + case 'f':
> + s += '\f';
> + escapeChar = false;
> + break;
> + case '"':
> + s += '"';
> + escapeChar = false;
> + break;
> + case '\\':
> + s += '\\';
> + escapeChar = false;
> + break;
> + case '0':
> + case '1':
> + case '2':
> + case '3':
> + case '4':
> + case '5':
> + case '6':
> + case '7':
> + case '8':
> + case '9':
> +
> + capturedNumber += c;
> + if (capturedNumber.length() == 3)
> + {
> + escapeNumeric = false;
> + escapeChar = false;
> + s +=  (char)strtoul(capturedNumber.c_str(), NULL, 8);
> + }
> + else
> + escapeNumeric = true;
> + break;

When I was reading through this code, I got rather confused by the handling of
capturedNumber, since it appears in two different places and there's two
different places where it's converted to a char using strtoul().  I suggest
just using a nested loop inside this part of the case statement to confine
octal handling logic to a single place in the code.  When you encounter the
end of the octal number you can use the function pFile->unget() to put back
the terminating character into the input stream.  (For an example of something
very similar, see CqRibLexer::readString() in riblexer.cpp.)

> + default :
> + escapeChar = false;
> + break;
> + }
> + }
> + else
> + if (c ==  '\\')
> + escapeChar = true;
> + else
> + s += c;
> + }
>   AddString( s.c_str(), pProgramArea );
>   }
>   break;

Just attach any changes as a new patch, thanks :-)
~Chris.

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

Re: patch for bug id 2813534 "Escaped strings in RSL not working"

by Ricardo Mayor :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Hi Chris,
I'll implement all your suggestion and corrections in a new patch. Most of them are easy to add I think. Find some comments in the email


On Wed, Sep 30, 2009 at 10:58 PM, Chris Foster <chris42f@...> wrote:
Hi Ricardo,

On Sun, Sep 27, 2009 at 04:12:57PM +0100, Ricardo Mayor wrote:
> Hello,
> Find attached a patch for the bug "*Escaped strings in RSL not working".
> According *to ri specs, the supported escape string are
>
> \n       linefeed (newline)
> \r       carriage return
> \t       horizontal tab
> \b       backspace
> \f       form feed
> \\       backslash
> \"       double quote
> \ddd     character code ddd (octal)
> \newline no character ? both are ignored

I think these may be the escape sequences specified in the part of the RISpec
which deals with the RIB format, not the renderman shading language?

Here's what I see in my copy of the RISpec (around page 117):

" Strings are used to name external objects (texture maps, for
example). String literals (or
" constants) are enclosed in double quotes, as in the C language, and
may contain any of the
" standard C "escape sequences" (such as \n for newline or \" for a quote).

So actually I guess we should support a few more things which are in the C
language but not part of the RIB syntax:

\a
\v
\'
\?
\ooo
\x hh
\x hhh
(According to http://msdn.microsoft.com/en-us/library/h21280bw(VS.80).aspx)

I agree with. this. I misinterpreted the specs. Anyway most of this new escape characteres are easy to  add.

I've also got some suggestions to make the code a bit easier to read.  To begin
with, please consider putting this chunk of code into its own function
(eg, LoadString() or whatever).  CqShaderVM::LoadProgram() is a monster
function already, let's not make it any huger!

Yes, I'll do it.
Other suggestions follow:

> diff --git a/libs/shadervm/shadervm.cpp b/libs/shadervm/shadervm.cpp
> index 225670a..a8d693e 100644
> --- a/libs/shadervm/shadervm.cpp
> +++ b/libs/shadervm/shadervm.cpp
> @@ -1279,11 +1279,89 @@ void CqShaderVM::LoadProgram( std::istream* pFile )
>                       ( *pFile ) >> std::ws;
>                       char c;
>                       CqString s( "" );
> +                     bool escapeChar = false, escapeNumeric = false;
> +                     CqString capturedNumber( "" );
> +
>                       pFile->get
>                       ();
> -                     while ( ( c = pFile->get
> -                                             () ) != '"' )
> -                             s += c;
> +                     while ( (( c = pFile->get() ) != '"') || escapeChar  )
> +                     {
> +
> +                             if (escapeNumeric)
> +                             {
> +                                     if(c < '0' or c > '9')
> +                                     {
> +                                             char number = strtoul(capturedNumber.c_str(), NULL, 8);
> +                                             s+= number;
> +                                             escapeChar = false;
> +                                             escapeNumeric =  false;
> +                                             capturedNumber = "";
> +                                     }
> +                             }
> +                             if(escapeChar)
> +                             {
> +                                     //Treatment for escape char
> +                                     switch(c){

Please use opening braces on their own line for consistency with the rest of
the codebase.

I installed  Artistic style to check the code style but.. Where is the  style file?

> +                                             case 'n':
> +                                                     s += '\n';
> +                                                     escapeChar = false;
> +                                                     break;
> +                                             case 'r':
> +                                                     s += '\r';
> +                                                     escapeChar = false;
> +                                                     break;
> +                                             case 't':
> +                                                     s += '\t';
> +                                                     escapeChar = false;
> +                                                     break;
> +                                             case 'b':
> +                                                     s += '\b';
> +                                                     escapeChar = false;
> +                                                     break;
> +                                             case 'f':
> +                                                     s += '\f';
> +                                                     escapeChar = false;
> +                                                     break;
> +                                             case '"':
> +                                                     s += '"';
> +                                                     escapeChar = false;
> +                                                     break;
> +                                             case '\\':
> +                                                     s += '\\';
> +                                                     escapeChar = false;
> +                                                     break;
> +                                             case '0':
> +                                             case '1':
> +                                             case '2':
> +                                             case '3':
> +                                             case '4':
> +                                             case '5':
> +                                             case '6':
> +                                             case '7':
> +                                             case '8':
> +                                             case '9':
> +
> +                                                     capturedNumber += c;
> +                                                     if (capturedNumber.length() == 3)
> +                                                     {
> +                                                             escapeNumeric = false;
> +                                                             escapeChar = false;
> +                                                             s +=  (char)strtoul(capturedNumber.c_str(), NULL, 8);
> +                                                     }
> +                                                     else
> +                                                             escapeNumeric = true;
> +                                                     break;

When I was reading through this code, I got rather confused by the handling of
capturedNumber, since it appears in two different places and there's two
different places where it's converted to a char using strtoul().  I suggest
just using a nested loop inside this part of the case statement to confine
octal handling logic to a single place in the code.  When you encounter the
end of the octal number you can use the function pFile->unget() to put back
the terminating character into the input stream.  (For an example of something
very similar, see CqRibLexer::readString() in riblexer.cpp.)

I wrote the code in this way to avoid using file->unget() when the length of the number is less than 3. Anyway I´ll check it

> +                                             default :
> +                                                     escapeChar = false;
> +                                                     break;
> +                                     }
> +                             }
> +                             else
> +                                     if (c ==  '\\')
> +                                             escapeChar = true;
> +                                     else
> +                                             s += c;
> +                     }
>                       AddString( s.c_str(), pProgramArea );
>               }
>               break;

Just attach any changes as a new patch, thanks :-)
~Chris.

------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

Re: patch for bug id 2813534 "Escaped strings in RSL not working"

by Chris Foster-5 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Thu, Oct 1, 2009 at 6:10 PM, Ricardo Mayor <ricardo.mayor@...> wrote:
> Hi Chris,
> I'll implement all your suggestion and corrections in a new patch. Most of
> them are easy to add I think. Find some comments in the email

Cool, thanks.

>> Please use opening braces on their own line for consistency with the rest
>> of
>> the codebase.
>
> I installed  Artistic style to check the code style but.. Where is the
> style file?

I personally dislike automatic code formatting because it's possible to do a
better job by hand if you're careful.  The most important thing to keep in
mind is to follow the style of the code around that which you're fixing to
keep things consistent.

Unfortunately there is more than one styles used in the aqsis codebase (eg,
capitalization of class member functions is inconsistent).  The current
"standard" such as it is can be found in

doc/coding_standard/codestyle.*

You will also find the astylerc file in doc/coding_standard, thought as I
said, I prefer to do careful manual formatting.

>> When I was reading through this code, I got rather confused by the
>> handling of
>> capturedNumber, since it appears in two different places and there's two
>> different places where it's converted to a char using strtoul().  I
>> suggest
>> just using a nested loop inside this part of the case statement to confine
>> octal handling logic to a single place in the code.  When you encounter
>> the
>> end of the octal number you can use the function pFile->unget() to put
>> back
>> the terminating character into the input stream.  (For an example of
>> something
>> very similar, see CqRibLexer::readString() in riblexer.cpp.)
>
> I wrote the code in this way to avoid using file->unget() when the length of
> the number is less than 3. Anyway I´ll check it

Yes, you definitely need to use unget() to write the code how I'm suggesting,
but I see no reason to avoid using it.  I think it will make this code clearer
and shorter ;-)

~Chris.

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

Re: patch for bug id 2813534 "Escaped strings in RSL not working"

by Ricardo Mayor :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Please find attached another version of the patch. I hope this version fulfill all the discussed requirements

Best regards

On Thu, Oct 1, 2009 at 12:36 PM, Chris Foster <chris42f@...> wrote:
On Thu, Oct 1, 2009 at 6:10 PM, Ricardo Mayor <ricardo.mayor@...> wrote:
> Hi Chris,
> I'll implement all your suggestion and corrections in a new patch. Most of
> them are easy to add I think. Find some comments in the email

Cool, thanks.

>> Please use opening braces on their own line for consistency with the rest
>> of
>> the codebase.
>
> I installed  Artistic style to check the code style but.. Where is the
> style file?

I personally dislike automatic code formatting because it's possible to do a
better job by hand if you're careful.  The most important thing to keep in
mind is to follow the style of the code around that which you're fixing to
keep things consistent.

Unfortunately there is more than one styles used in the aqsis codebase (eg,
capitalization of class member functions is inconsistent).  The current
"standard" such as it is can be found in

doc/coding_standard/codestyle.*

You will also find the astylerc file in doc/coding_standard, thought as I
said, I prefer to do careful manual formatting.

>> When I was reading through this code, I got rather confused by the
>> handling of
>> capturedNumber, since it appears in two different places and there's two
>> different places where it's converted to a char using strtoul().  I
>> suggest
>> just using a nested loop inside this part of the case statement to confine
>> octal handling logic to a single place in the code.  When you encounter
>> the
>> end of the octal number you can use the function pFile->unget() to put
>> back
>> the terminating character into the input stream.  (For an example of
>> something
>> very similar, see CqRibLexer::readString() in riblexer.cpp.)
>
> I wrote the code in this way to avoid using file->unget() when the length of
> the number is less than 3. Anyway I´ll check it

Yes, you definitely need to use unget() to write the code how I'm suggesting,
but I see no reason to avoid using it.  I think it will make this code clearer
and shorter ;-)

~Chris.

------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development


[escape_char_v2.patch]

diff --git a/libs/shadervm/shadervm.cpp b/libs/shadervm/shadervm.cpp
index b498adf..5861646 100644
--- a/libs/shadervm/shadervm.cpp
+++ b/libs/shadervm/shadervm.cpp
@@ -1276,14 +1276,7 @@ void CqShaderVM::LoadProgram( std::istream* pFile )
  break;
  case type_string:
  {
- ( *pFile ) >> std::ws;
- char c;
- CqString s( "" );
- pFile->get
- ();
- while ( ( c = pFile->get
- () ) != '"' )
- s += c;
+ CqString s = GetString(pFile);
  AddString( s.c_str(), pProgramArea );
  }
  break;
@@ -1341,6 +1334,127 @@ void CqShaderVM::LoadProgram( std::istream* pFile )
  }
 }
 
+CqString CqShaderVM::GetString(std::istream* pFile)
+{
+ ( *pFile ) >> std::ws;
+ char c;
+ CqString s( "" );
+ bool escapeChar = false;
+
+ pFile->get();
+ while ( (( c = pFile->get() ) != '"') || escapeChar  )
+ {
+ if(escapeChar)
+ {
+ //Treatment for escape char
+ switch(c)
+ {
+ case 'a'://Bell (alert)
+ s += '\a';
+ escapeChar = false;
+ break;
+ case 'v'://Vertical tab
+ s += '\v';
+ escapeChar = false;
+ break;
+ case '\''://Single quotation mark
+ s += "'";
+ escapeChar = false;
+ break;
+ case '?'://Literal question mark
+ s += '\?';
+ escapeChar = false;
+ break;
+ case 'n'://New line
+ s += '\n';
+ escapeChar = false;
+ break;
+ case 'r'://Carriage return
+ s += '\r';
+ escapeChar = false;
+ break;
+ case 't'://Horizontal tab
+ s += '\t';
+ escapeChar = false;
+ break;
+ case 'b'://Backspace
+ s += '\b';
+ escapeChar = false;
+ break;
+ case 'f'://Formfeed
+ s += '\f';
+ escapeChar = false;
+ break;
+ case '"'://Double quotation mark
+ s += '"';
+ escapeChar = false;
+ break;
+ case '\\'://Backslash
+ s += '\\';
+ escapeChar = false;
+ break;
+ case 'x'://Hexadecimal
+ case '0'://octal
+ case '1':
+ case '2':
+ case '3':
+ case '4':
+ case '5':
+ case '6':
+ case '7':
+ case '8':
+ case '9':
+ GetNumericEscapeChar(pFile, s,  c);
+ escapeChar = false;
+ break;
+ default :
+ escapeChar = false;
+ break;
+ }
+ }
+ else
+ if (c ==  '\\')
+ escapeChar = true;
+ else
+ s += c;
+ }
+ return s;
+}
+
+void CqShaderVM::GetNumericEscapeChar(std::istream* pFile, CqString &s, char c)
+{
+ CqString a("");
+ bool isHexadecimal = (c =='x');
+ unsigned int maxLength;
+
+ if (isHexadecimal)
+ {
+ maxLength = 2;
+ }
+ else
+ {
+ a += c;
+ maxLength = 3;
+ }
+ c = pFile->get();
+
+ while ((( c >= '0' && c <= '9') || ((( c >= 'a' && c <= 'f') || ( c >= 'A' && c <= 'F')) && isHexadecimal)) && (a.length() <maxLength))
+ {
+ a += c;
+ c = pFile->get();
+ }
+
+ int numericalBase;
+
+ if(isHexadecimal)
+ numericalBase = 16;
+ else
+ numericalBase = 8;
+ char result = strtoul(a.c_str(), NULL, numericalBase);
+
+ if (result != 0) s += result;
+ pFile->unget();
+}
 
 //---------------------------------------------------------------------
 /** Ready the shader for execution.
diff --git a/libs/shadervm/shadervm.h b/libs/shadervm/shadervm.h
index 11eb227..9e55f90 100644
--- a/libs/shadervm/shadervm.h
+++ b/libs/shadervm/shadervm.h
@@ -246,6 +246,19 @@ class AQSIS_SHADERVM_SHARE CqShaderVM : public CqShaderStack, public IqShader, p
  bool m_outsideWorld; ///< Flag indicating this shader was declared outside the world.
  IqRenderer* m_pRenderContext;
 
+
+ /** Get the char defined in the Numeric Escape char
+ * \param pFile shader stream
+ * \return String
+ */
+ CqString GetString(std::istream* pFile);
+ /** Get the char defined in the Numeric Escape char
+ * \param pFile shader stream
+ * \param s result string
+ * \param c, current character
+ * \return char equivalence for the numeric escape sequence
+ */
+ void GetNumericEscapeChar(std::istream* pFile, CqString &s, char c) ;
  /** Determine whether the program execution has finished.
  */
  bool fDone()


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

Re: patch for bug id 2813534 "Escaped strings in RSL not working"

by Chris Foster-5 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Hi Ricardo,

On Sun, Oct 04, 2009 at 08:43:11PM +0100, Ricardo Mayor wrote:
> Please find attached another version of the patch.

Thanks!

There's still a few issues, but the patch certainly looks better now.
Since we're just about to release aqsis-1.6 I'm going to commit the
patch anyway, and clean up the few bits I think are most important
myself.

I'm quite picky with code quality, so bear with me ;-)  Here's a review
of the patch.  The main things which I think are fairly important and
I'll fix myself (only because of the tight shedule before the release)
are:

1) Some inaccurate function docs in shadervm.h
2) Declare GetString() and GetNumericEscapeChar() as static since they
   don't access any CqShaderVM member data

When checking the patch I also noticed that the function SO_sprintf()
(for printing strings from a shader program) actually translates escape
characters itself.  It uses the function CqString::TranslateEscapes()
which I didn't know about.  With the new code the translation has
already happened when reading the shader program, so I'll fix that.


> diff --git a/libs/shadervm/shadervm.cpp b/libs/shadervm/shadervm.cpp
> index b498adf..5861646 100644
> --- a/libs/shadervm/shadervm.cpp
> +++ b/libs/shadervm/shadervm.cpp
> @@ -1341,6 +1334,127 @@ void CqShaderVM::LoadProgram( std::istream* pFile )
>   }
>  }
>  
> +CqString CqShaderVM::GetString(std::istream* pFile)
> +{
> + ( *pFile ) >> std::ws;
> + char c;
> + CqString s( "" );
> + bool escapeChar = false;
> +
> + pFile->get();
> + while ( (( c = pFile->get() ) != '"') || escapeChar  )
> + {
> + if(escapeChar)

The presence of the escapeChar variable seems to complicate things
a bit unnecessarily.  It would be simpler if you allow yourself
more than one place to call pFile->get().  In particular, you can have

if(c == '\\')
{
    c = pFile->get();
    switch(c)
    {
        //...
    }
}
else
    s += c;

which avoids the escapeChar variable completely.  Other than that, this
function looks about right - you seem to have all the right escape
characters there.

> + {
> + //Treatment for escape char
> + switch(c)
> + {
> + case 'a'://Bell (alert)
> + s += '\a';
> + escapeChar = false;
> + break;
> + case 'v'://Vertical tab
> + s += '\v';
> + escapeChar = false;
> + break;
> + case '\''://Single quotation mark
> + s += "'";
> + escapeChar = false;
> + break;
> + case '?'://Literal question mark
> + s += '\?';
> + escapeChar = false;
> + break;
> + case 'n'://New line
> + s += '\n';
> + escapeChar = false;
> + break;
> + case 'r'://Carriage return
> + s += '\r';
> + escapeChar = false;
> + break;
> + case 't'://Horizontal tab
> + s += '\t';
> + escapeChar = false;
> + break;
> + case 'b'://Backspace
> + s += '\b';
> + escapeChar = false;
> + break;
> + case 'f'://Formfeed
> + s += '\f';
> + escapeChar = false;
> + break;
> + case '"'://Double quotation mark
> + s += '"';
> + escapeChar = false;
> + break;
> + case '\\'://Backslash
> + s += '\\';
> + escapeChar = false;
> + break;
> + case 'x'://Hexadecimal
> + case '0'://octal
> + case '1':
> + case '2':
> + case '3':
> + case '4':
> + case '5':
> + case '6':
> + case '7':
> + case '8':
> + case '9':
> + GetNumericEscapeChar(pFile, s,  c);
> + escapeChar = false;
> + break;
> + default :
> + escapeChar = false;
> + break;
> + }
> + }
> + else
> + if (c ==  '\\')
> + escapeChar = true;
> + else
> + s += c;
> + }
> + return s;
> +}
>
> +void CqShaderVM::GetNumericEscapeChar(std::istream* pFile, CqString &s, char c)

This function signature would be a lot cleaner if you avoid passing in c
(just unget() it) and if you return the interpreted character rather
than appending it to the string s.

> +{
> + CqString a("");
> + bool isHexadecimal = (c =='x');
> + unsigned int maxLength;
> +
> + if (isHexadecimal)
> + {
> + maxLength = 2;
> + }
> + else
> + {
> + a += c;
> + maxLength = 3;
> + }
> + c = pFile->get();
> +
> + while ((( c >= '0' && c <= '9') || ((( c >= 'a' && c <= 'f') || ( c >= 'A' && c <= 'F')) && isHexadecimal)) && (a.length() <maxLength))
> + {
> + a += c;
> + c = pFile->get();
> + }
> +
> + int numericalBase;
> +
> + if(isHexadecimal)
> + numericalBase = 16;
> + else
> + numericalBase = 8;
> + char result = strtoul(a.c_str(), NULL, numericalBase);
> +
> + if (result != 0) s += result;

I wonder whether this is the right thing to do?  Maybe the user should
be allowed to insert NULL into the string if they want?  Mind you, it's
probably going to cause problems if we use char* instead of std::string
anywhere since then the string will be prematurely terminated...

> + pFile->unget();
> +}
>
>  //---------------------------------------------------------------------
>  /** Ready the shader for execution.
> diff --git a/libs/shadervm/shadervm.h b/libs/shadervm/shadervm.h
> index 11eb227..9e55f90 100644
> --- a/libs/shadervm/shadervm.h
> +++ b/libs/shadervm/shadervm.h
> @@ -246,6 +246,19 @@ class AQSIS_SHADERVM_SHARE CqShaderVM : public CqShaderStack, public IqShader, p
>   bool m_outsideWorld; ///< Flag indicating this shader was declared outside the world.
>   IqRenderer* m_pRenderContext;
>  
> +
> + /** Get the char defined in the Numeric Escape char
> + * \param pFile shader stream
> + * \return String
> + */
> + CqString GetString(std::istream* pFile);

The documentation is confusing here.

Also, note that GetString doesn't modify or even reference the state of
CqShaderVM.  Therefore, it should be a static method:

  static CqString GetString(std::istream* pFile);

(At the very least it should be const method:
  CqString GetString(std::istream* pFile) const;
)

> + /** Get the char defined in the Numeric Escape char
> + * \param pFile shader stream
> + * \param s result string
> + * \param c, current character
> + * \return char equivalence for the numeric escape sequence
> + */
> + void GetNumericEscapeChar(std::istream* pFile, CqString &s, char c) ;

Similar thing here.  The docs seem out of date, and this function should
be static too.

That's it, thanks for the patch - it's committed now along with the
minor changes by me in a separate commit.
~Chris.

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

Re: patch for bug id 2813534 "Escaped strings in RSL not working"

by Ricardo Mayor :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

I agree with all the changes you suggested except with two.

On Mon, Oct 5, 2009 at 1:49 PM, Chris Foster <chris42f@...> wrote:

This function signature would be a lot cleaner if you avoid passing in c
(just unget() it) and if you return the interpreted character rather
than appending it to the string s.

My first option was to return the interpreted character but this assumes that you always have a char to return. This is not the case when the user writes incorrect numeric escapes sequences. For example:
"\xp" //Hex incorrect number
"\0AF"//Octal incorrect number

In both cases the numeric sequence is invalid and the actual code does not add any char to the string. If the function returns a char, it doesn't have a char to return. That's the reason why I pass the CqString, to encapsulate this functionality inside the function. In the current examples, the final string are:
"p"
"AF"
 

> +{
> +     CqString a("");
> +     bool isHexadecimal = (c =='x');
> +     unsigned int maxLength;
> +
> +     if (isHexadecimal)
> +     {
> +             maxLength = 2;
> +     }
> +     else
> +     {
> +             a += c;
> +             maxLength = 3;
> +     }
> +     c = pFile->get();
> +
> +     while ((( c >= '0' && c <= '9') || ((( c >= 'a' && c <= 'f') || ( c >= 'A' && c <= 'F')) && isHexadecimal)) && (a.length() <maxLength))
> +     {
> +             a += c;
> +             c = pFile->get();
> +     }
> +
> +     int numericalBase;
> +
> +     if(isHexadecimal)
> +             numericalBase = 16;
> +     else
> +             numericalBase = 8;
> +     char result = strtoul(a.c_str(), NULL, numericalBase);
> +
> +     if (result != 0) s += result;
 
I wonder whether this is the right thing to do?  Maybe the user should
be allowed to insert NULL into the string if they want?  Mind you, it's
probably going to cause problems if we use char* instead of std::string
anywhere since then the string will be prematurely terminated...

In this case the null is not allowed. If the user create a strint "\000", this will not be accepted. 
> +     pFile->unget();
> +}
>


------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

Re: patch for bug id 2813534 "Escaped strings in RSL not working"

by Chris Foster-5 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, Oct 6, 2009 at 12:05 AM, Ricardo Mayor <ricardo.mayor@...> wrote:
> My first option was to return the interpreted character but this assumes
> that you always have a char to return. This is not the case when the user
> writes incorrect numeric escapes sequences. For example:
> "\xp" //Hex incorrect number
> "\0AF"//Octal incorrect number

Fair point.  (Though I think the octal example is valid... it's \0 followed by
the letters AF which are unrelated.)

[...]

>> I wonder whether this is the right thing to do?  Maybe the user should
>> be allowed to insert NULL into the string if they want?  Mind you, it's
>> probably going to cause problems if we use char* instead of std::string
>> anywhere since then the string will be prematurely terminated...
>>
> In this case the null is not allowed.  If the user create a strint "\000",
> this will not be accepted.

Why can't it be accepted though?  It is legal in C, and we're trying to follow
the C rules because the RISpec says so.  In the following C++ code, it's
perfectly legal to use \0 in a string, and it gets interpreted as 0:

----------
#include <iostream>
#include <string>

int main()
{
    const char s[] = "asdf\0asdf";
    std::string str(s, s+sizeof(s));

    std::cout << str << "\n";

    std::cout << "str[4] = " << int(str[4]) << "\n";

    return 0;
}
----------


~Chris.

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development