Fix compiler warnings on WebKit ports when updating ANGLE in WebKit

Fix compiler warnings on WebKit ports when updating ANGLE in WebKit

[mvujovic, 2012-06-25 16:26:19]

Hi Alok,

Could you take a look at this when you get a chance? I need these changes to
update ANGLE in WebKit.

Thank you,
Max

[kbr1, 2012-06-25 19:08:22]

These changes LGTM, but wait 24 hours for Alok's review; I'm not sure whether
he's on vacation today. Also CC'ing Daniel and Nicolas for their comments on the
particular version of Bison used to generate the parsers.

https://codereview.appspot.com/6310067/diff/1/src/compiler/glslang_tab.cpp
File src/compiler/glslang_tab.cpp (right):

https://codereview.appspot.com/6310067/diff/1/src/compiler/glslang_tab.cpp#ne...
src/compiler/glslang_tab.cpp:1: /* A Bison parser, made by GNU Bison 2.5.  */
Could you try to rebuild the parsers with the same version of Bison (2.4.2) used
previously? I think Bison 2.4.2 on Windows has been the canonical version used
up to this point.

[mvujovic, 2012-06-25 20:07:59]

On 2012/06/25 19:08:22, kbr1 wrote:
> These changes LGTM, but wait 24 hours for Alok's review; I'm not sure whether
> he's on vacation today. Also CC'ing Daniel and Nicolas for their comments on
the
> particular version of Bison used to generate the parsers.

Sounds good. Thanks, Ken.

> https://codereview.appspot.com/6310067/diff/1/src/compiler/glslang_tab.cpp
> File src/compiler/glslang_tab.cpp (right):
> 
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/glslang_tab.cpp#ne...
> src/compiler/glslang_tab.cpp:1: /* A Bison parser, made by GNU Bison 2.5.  */
> Could you try to rebuild the parsers with the same version of Bison (2.4.2)
used
> previously? I think Bison 2.4.2 on Windows has been the canonical version used
> up to this point.

Sure, I'll rebuild with Bison 2.4.2.

[Alok Priyadarshi, 2012-06-25 22:13:26]

I have some minor comments. Otherwise it looks reasonable.

https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp
File src/compiler/OutputHLSL.cpp (right):

https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp#new...
src/compiler/OutputHLSL.cpp:2253: break;
add UNREACHABLE() if it should not be reached.

https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
src/compiler/ParseHelper.cpp:196: void TParseContext::error(TSourceLoc loc,
const char *reason, const char* token)
hmm. Is this change really necessary?

https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
src/compiler/ParseHelper.cpp:217: void TParseContext::warning(TSourceLoc loc,
const char* reason, const char* token)
same here.

https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h
File src/compiler/ParseHelper.h (right):

https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h#newc...
src/compiler/ParseHelper.h:76: const char* extraInfoFormat, ...)
I do not think we need variable-arguments here. I think the following signature
should suffice:
error(TSourceLoc loc, const char *reason, const char* token, const char* extra);

Could you check?

[mvujovic, 2012-06-25 23:38:46]

On 2012/06/25 22:13:26, Alok Priyadarshi wrote:
> I have some minor comments. Otherwise it looks reasonable.

Thanks for the review, Alok!

> https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp
> File src/compiler/OutputHLSL.cpp (right):
>
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp#new...
> src/compiler/OutputHLSL.cpp:2253: break;
> add UNREACHABLE() if it should not be reached.

This code falls through to UNIMPLEMENTED() at the bottom of the function when
there is no type match. I think this is intended. This patch doesn't change that
behavior.

> https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp
> File src/compiler/ParseHelper.cpp (right):
>
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
> src/compiler/ParseHelper.cpp:196: void TParseContext::error(TSourceLoc loc,
> const char *reason, const char* token)
> hmm. Is this change really necessary?

I introduced this variant of the error method because most calls to error()
don't need extraInfoFormat and varargs. I wanted to avoid writing "%s", "" in
many places.

For example, I think (1) looks better than (2):
(1) error(line, "illegal vector field selection", compString.c_str());
(2) error(line, "illegal vector field selection", compString.c_str(), "%s", "");

If you prefer (2), I can remove this function. What do you think?

>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
> src/compiler/ParseHelper.cpp:217: void TParseContext::warning(TSourceLoc loc,
> const char* reason, const char* token)
> same here.
>
> https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h
> File src/compiler/ParseHelper.h (right):
>
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h#newc...
> src/compiler/ParseHelper.h:76: const char* extraInfoFormat, ...)
> I do not think we need variable-arguments here. I think the following
signature
> should suffice:
> error(TSourceLoc loc, const char *reason, const char* token, const char*
extra);
>
> Could you check?

Unfortunately, we use the varargs in error() in at least 8 places, for
situations like these:

ParseHelper.cpp: 
error(line, "", op, "cannot convert from '%s' to '%s'", right.c_str(),
left.c_str());

glslang_tab.cpp:
context->error((yyvsp[(2) - (4)].lex).line, "", "[", "field selection out of
range '%d'", (yyvsp[(3) -
(4)].interm.intermTypedNode)->getAsConstantUnion()->getUnionArrayPointer()->getIConst());

Are you okay with keeping it this way?

[Alok Priyadarshi, 2012-06-26 04:31:07]

For future, please reply to comments by browsing to individual files and
clicking on the comments. Replying inline loses context.

On 2012/06/25 23:38:46, mvujovic wrote:
> On 2012/06/25 22:13:26, Alok Priyadarshi wrote:
> > I have some minor comments. Otherwise it looks reasonable.
> 
> Thanks for the review, Alok!
> 
> > https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp
> > File src/compiler/OutputHLSL.cpp (right):
> >
> >
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp#new...
> > src/compiler/OutputHLSL.cpp:2253: break;
> > add UNREACHABLE() if it should not be reached.
> 
> This code falls through to UNIMPLEMENTED() at the bottom of the function when
> there is no type match. I think this is intended. This patch doesn't change
that
> behavior.

I see. Its fine this way then.

> > https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp
> > File src/compiler/ParseHelper.cpp (right):
> >
> >
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
> > src/compiler/ParseHelper.cpp:196: void TParseContext::error(TSourceLoc loc,
> > const char *reason, const char* token)
> > hmm. Is this change really necessary?
> 
> I introduced this variant of the error method because most calls to error()
> don't need extraInfoFormat and varargs. I wanted to avoid writing "%s", "" in
> many places.
> 
> For example, I think (1) looks better than (2):
> (1) error(line, "illegal vector field selection", compString.c_str());
> (2) error(line, "illegal vector field selection", compString.c_str(), "%s",
"");
> 
> If you prefer (2), I can remove this function. What do you think?
> 

I personally do not mind adding an extra "" everywhere, but could you instead
make extra as default parameter? Function overloading seems wrong here. Of
course this would mean changing the function signature to what I suggested
before.

>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
> > src/compiler/ParseHelper.cpp:217: void TParseContext::warning(TSourceLoc
loc,
> > const char* reason, const char* token)
> > same here.
> >
> > https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h
> > File src/compiler/ParseHelper.h (right):
> >
> >
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h#newc...
> > src/compiler/ParseHelper.h:76: const char* extraInfoFormat, ...)
> > I do not think we need variable-arguments here. I think the following
> signature
> > should suffice:
> > error(TSourceLoc loc, const char *reason, const char* token, const char*
> extra);
> >
> > Could you check?
> 
> Unfortunately, we use the varargs in error() in at least 8 places, for
> situations like these:
> 
> ParseHelper.cpp: 
> error(line, "", op, "cannot convert from '%s' to '%s'", right.c_str(),
> left.c_str());
> 
> glslang_tab.cpp:
> context->error((yyvsp[(2) - (4)].lex).line, "", "[", "field selection out of
> range '%d'", (yyvsp[(3) -
>
(4)].interm.intermTypedNode)->getAsConstantUnion()->getUnionArrayPointer()->getIConst());
> 
> Are you okay with keeping it this way?

I know this is getting outside the scope of what you are trying to do, but can
we fix these 8 places? That would make thing much cleaner.

[mvujovic, 2012-06-26 16:41:04]

On 2012/06/26 04:31:07, Alok Priyadarshi wrote:
> For future, please reply to comments by browsing to individual files and
> clicking on the comments. Replying inline loses context.

Will do. Sorry about that.

> On 2012/06/25 23:38:46, mvujovic wrote:
> > On 2012/06/25 22:13:26, Alok Priyadarshi wrote:
> > > I have some minor comments. Otherwise it looks reasonable.
> >
> > Thanks for the review, Alok!
> >
> > > https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp
> > > File src/compiler/OutputHLSL.cpp (right):
> > >
> > >
> >
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/OutputHLSL.cpp#new...
> > > src/compiler/OutputHLSL.cpp:2253: break;
> > > add UNREACHABLE() if it should not be reached.
> >
> > This code falls through to UNIMPLEMENTED() at the bottom of the function
when
> > there is no type match. I think this is intended. This patch doesn't change
> that
> > behavior.
>
> I see. Its fine this way then.
>
> > > https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp
> > > File src/compiler/ParseHelper.cpp (right):
> > >
> > >
> >
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
> > > src/compiler/ParseHelper.cpp:196: void TParseContext::error(TSourceLoc
loc,
> > > const char *reason, const char* token)
> > > hmm. Is this change really necessary?
> >
> > I introduced this variant of the error method because most calls to error()
> > don't need extraInfoFormat and varargs. I wanted to avoid writing "%s", ""
in
> > many places.
> >
> > For example, I think (1) looks better than (2):
> > (1) error(line, "illegal vector field selection", compString.c_str());
> > (2) error(line, "illegal vector field selection", compString.c_str(), "%s",
> "");
> >
> > If you prefer (2), I can remove this function. What do you think?
> >
>
> I personally do not mind adding an extra "" everywhere, but could you instead
> make extra as default parameter? Function overloading seems wrong here. Of
> course this would mean changing the function signature to what I suggested
> before.

Sure, that sounds good to me.

> >
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.cpp#ne...
> > > src/compiler/ParseHelper.cpp:217: void TParseContext::warning(TSourceLoc
> loc,
> > > const char* reason, const char* token)
> > > same here.
> > >
> > > https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h
> > > File src/compiler/ParseHelper.h (right):
> > >
> > >
> >
>
https://codereview.appspot.com/6310067/diff/1/src/compiler/ParseHelper.h#newc...
> > > src/compiler/ParseHelper.h:76: const char* extraInfoFormat, ...)
> > > I do not think we need variable-arguments here. I think the following
> > signature
> > > should suffice:
> > > error(TSourceLoc loc, const char *reason, const char* token, const char*
> > extra);
> > >
> > > Could you check?
> >
> > Unfortunately, we use the varargs in error() in at least 8 places, for
> > situations like these:
> >
> > ParseHelper.cpp:
> > error(line, "", op, "cannot convert from '%s' to '%s'", right.c_str(),
> > left.c_str());
> >
> > glslang_tab.cpp:
> > context->error((yyvsp[(2) - (4)].lex).line, "", "[", "field selection out of
> > range '%d'", (yyvsp[(3) -
> >
>
(4)].interm.intermTypedNode)->getAsConstantUnion()->getUnionArrayPointer()->getIConst());
> >
> > Are you okay with keeping it this way?
>
> I know this is getting outside the scope of what you are trying to do, but can
> we fix these 8 places? That would make thing much cleaner.

Sure, I'lll refactor these and update the patch for your feedback. I don't mind
:)

Another question-
What version of flex do you use to generate the tokenizer?

[Alok Priyadarshi, 2012-06-27 16:20:13]

> Another question-
> What version of flex do you use to generate the tokenizer?

I use bison 2.4.2 and flex 2.5.35. These are the most recent versions available
with cygwin. If you are not using any version-specific feature, using a newer
version to generate the parser should be fine (as long as it works).

[mvujovic, 2012-06-27 17:35:32]

Hi Alok,

I refactored error formatting to use the insertion operator (<<) instead of
varargs. I created a helper class "TDiagnosticBuilder" to make this happen.
TDiagnosticBuilder extends TInfoSinkBase in order to leverage its insertion
operator functions, which perform nice formatting for various types.

What do you think?

Thanks,
Max

https://codereview.appspot.com/6310067/diff/13001/src/compiler/DiagnosticBuil...
File src/compiler/DiagnosticBuilder.h (right):

https://codereview.appspot.com/6310067/diff/13001/src/compiler/DiagnosticBuil...
src/compiler/DiagnosticBuilder.h:34: class TDiagnosticBuilder : public
TInfoSinkBase {
This is a new class to help format diagnostic messages. We can use it to stream
in extra info using "<<" instead of using varargs.

https://codereview.appspot.com/6310067/diff/13001/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

https://codereview.appspot.com/6310067/diff/13001/src/compiler/ParseHelper.cp...
src/compiler/ParseHelper.cpp:210: error(line, "", op) << "cannot convert from '"
<< right << "' to '" << left << "'";
Here is an example of the new way of formatting error messages with extra info.

https://codereview.appspot.com/6310067/diff/13001/src/compiler/ParseHelper.h
File src/compiler/ParseHelper.h (right):

https://codereview.appspot.com/6310067/diff/13001/src/compiler/ParseHelper.h#...
src/compiler/ParseHelper.h:72: TDiagnosticBuilder error(TSourceLoc loc, const
char *reason, const char* token, const char* extraInfo=0);
This is error()'s new signature.

[Alok Priyadarshi, 2012-06-27 18:05:36]

On 2012/06/27 17:35:32, mvujovic wrote:
> Hi Alok,
> 
> I refactored error formatting to use the insertion operator (<<) instead of
> varargs. I created a helper class "TDiagnosticBuilder" to make this happen.
> TDiagnosticBuilder extends TInfoSinkBase in order to leverage its insertion
> operator functions, which perform nice formatting for various types.
> 
> What do you think?
> 
> Thanks,
> Max
> 

It is an interesting idea. But I think it is a little confusing. I think it will
be much simpler if we formatted the "extra" string using a std::stringstream and
passed the formatted string to the error function.

[mvujovic, 2012-06-27 18:22:43]

On 2012/06/27 18:05:36, Alok Priyadarshi wrote:
> On 2012/06/27 17:35:32, mvujovic wrote:
> > Hi Alok,
> > 
> > I refactored error formatting to use the insertion operator (<<) instead of
> > varargs. I created a helper class "TDiagnosticBuilder" to make this happen.
> > TDiagnosticBuilder extends TInfoSinkBase in order to leverage its insertion
> > operator functions, which perform nice formatting for various types.
> > 
> > What do you think?
> > 
> > Thanks,
> > Max
> > 
> 
> It is an interesting idea. But I think it is a little confusing. I think it
will
> be much simpler if we formatted the "extra" string using a std::stringstream
and
> passed the formatted string to the error function.

Sure, I can use std::stringstream instead. I'll reupload a patch with that
approach.

I agree this method could be a bit too tricky. I borrowed this pattern from
clang. Their diagnostic printing looks like this:

Diag(Loc, diag::warn_unavailable) << D->getDeclName();

(http://clang.llvm.org/doxygen/SemaExpr_8cpp-source.html)

[mvujovic, 2012-06-27 20:34:18]

On 2012/06/27 18:22:43, mvujovic wrote:
> On 2012/06/27 18:05:36, Alok Priyadarshi wrote:
> > On 2012/06/27 17:35:32, mvujovic wrote:
> > > Hi Alok,
> > > 
> > > I refactored error formatting to use the insertion operator (<<) instead
of
> > > varargs. I created a helper class "TDiagnosticBuilder" to make this
happen.
> > > TDiagnosticBuilder extends TInfoSinkBase in order to leverage its
insertion
> > > operator functions, which perform nice formatting for various types.
> > > 
> > > What do you think?
> > > 
> > > Thanks,
> > > Max
> > > 
> > 
> > It is an interesting idea. But I think it is a little confusing. I think it
> will
> > be much simpler if we formatted the "extra" string using a std::stringstream
> and
> > passed the formatted string to the error function.
> 
> Sure, I can use std::stringstream instead. I'll reupload a patch with that
> approach.
> 
> I agree this method could be a bit too tricky. I borrowed this pattern from
> clang. Their diagnostic printing looks like this:
> 
> Diag(Loc, diag::warn_unavailable) << D->getDeclName();
> 
> (http://clang.llvm.org/doxygen/SemaExpr_8cpp-source.html)

I uploaded patch set 4, which uses std::stringstream to format the extra info
strings. What do you think?

[mvujovic, 2012-06-27 20:34:35]

http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.cpp...
src/compiler/ParseHelper.cpp:211: error(line, "", op,
extraInfoStream.str().c_str());
This is an example of formatting the extra info using std::stringstream.

http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.h
File src/compiler/ParseHelper.h (right):

http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.h#n...
src/compiler/ParseHelper.h:72: const char* extraInfo="");
This is the new signature for error().

[Alok Priyadarshi, 2012-06-27 21:11:36]

LGTM. Please verify if it works. Thanks for your contribution.

http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.cpp...
src/compiler/ParseHelper.cpp:211: error(line, "", op,
extraInfoStream.str().c_str());
On 2012/06/27 20:34:35, mvujovic wrote:
> This is an example of formatting the extra info using std::stringstream.

Please test if it works as intended. I suspect that the temporary string
returned by stringstream::str() will be destroyed by the time error function is
called. You might need to store the temporary string before calling error().

[mvujovic, 2012-06-27 21:43:35]

On 2012/06/27 21:11:36, Alok Priyadarshi wrote:
> LGTM. Please verify if it works. Thanks for your contribution.

Thanks for the review!

> http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.cpp
> File src/compiler/ParseHelper.cpp (right):
> 
>
http://codereview.appspot.com/6310067/diff/12018/src/compiler/ParseHelper.cpp...
> src/compiler/ParseHelper.cpp:211: error(line, "", op,
> extraInfoStream.str().c_str());
> On 2012/06/27 20:34:35, mvujovic wrote:
> > This is an example of formatting the extra info using std::stringstream.
> 
> Please test if it works as intended. I suspect that the temporary string
> returned by stringstream::str() will be destroyed by the time error function
is
> called. You might need to store the temporary string before calling error().

Thanks for catching this. It works as expected right now. However, I much feel
better putting the temporary string on the stack before the call, so we don't
depend on the implementation of std::stringstream. I uploaded patch set 5, which
does this instead (see below).

Does that look better?

http://codereview.appspot.com/6310067/diff/18002/src/compiler/ParseHelper.cpp
File src/compiler/ParseHelper.cpp (right):

http://codereview.appspot.com/6310067/diff/18002/src/compiler/ParseHelper.cpp...
src/compiler/ParseHelper.cpp:212: error(line, "", op, extraInfo.c_str());
This is the revised way of formatting extra info using std::stringstream and a
temporary std::string on the stack.

[Alok Priyadarshi, 2012-06-27 22:14:39]

lgtm

[mvujovic, 2012-06-27 22:52:41]

On 2012/06/27 22:14:39, Alok Priyadarshi wrote:
> lgtm

Thanks! Committed in r1170
(http://code.google.com/p/angleproject/source/detail?r=1170).