Aqsis Renderer

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.

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.

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();
> > +}
> >
>

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.