graphitemaster / glsl-parser

A GLSL parser
MIT License
257 stars 30 forks source link

Warnings shadowing variables #7

Closed bkaradzic closed 8 years ago

bkaradzic commented 8 years ago

There are bunch of warnings about shadowing variables (add -Wshadow to Makefile).

Example:

ast.h: In constructor ‘glsl::astMemory::astMemory(T*)’:
ast.h:18:24: warning: declaration of ‘data’ shadows a member of ‘glsl::astMemory’ [-Wshadow]
     astMemory(T *data) : data((void*)data), dtor(&astDestroy<T>) { }
                        ^
ast.h:19:11: note: shadowed declaration is here
     void *data;

I can send PR with fix for these, but not sure what you would prefer for naming them?

graphitemaster commented 8 years ago

This is a silly warning

bkaradzic commented 8 years ago

This warning is default in VS2015 when building with warning level 4, adding it to GCC makes both compilers produce similar warnings, and helps maintain no-warnings.

graphitemaster commented 8 years ago

in general I'd like to cleanup glsl-parser to use m_ prefix on member variables but I haven't gotten around to that.

bkaradzic commented 8 years ago

Cool, that would fix it. Another cleanup suggestion is to typedef all std::vector usage, that way std::vector can be swapped with something that has compatible interface in one place. For example typedef std::vector<chars> Buffer; or something similar.

graphitemaster commented 8 years ago

Took your idea regarding std::vector and added a wrapper around it in util.h. If you have C++11 you can at least easily use your own by doing something like

template <typename T>
using vector = myVector<T>;

If not you should beable to replace the one instance of std::vector in the wrapper with myVector to achieve the same thing in anything < C++11.

bkaradzic commented 8 years ago

Actually my idea was slightly simpler.

typedef std::vector<char> Buffer; then I can change that typedef to use different implementation of STL I provide, typedef someother_stl::vector<char> Buffer;.

Anyhow your change does the same.

graphitemaster commented 8 years ago

the problem with that is I have more than just one instantation of a vector with a type. The proper solution is

template <typename T>
using vector = std::vector<T>;

But that is C++11. This wrapper is the only other technique I could think of to do it. Your method would require a typedef for every possible instantation.

bkaradzic commented 8 years ago

Yeah, I meant you typedef them all. :)

Like: typedef std::vector<astLayoutQualifier*> AstLayoutQualifierVector;

And then instead: vector<astLayoutQualifier*> &qualifiers = variable->layoutQualifiers;

In the rest of the code you just write: AstLayoutQualifierVector &qualifiers = variable->layoutQualifiers;

graphitemaster commented 8 years ago

That is too tedious and would require more changes than the wrapper :)

bkaradzic commented 8 years ago

I can do the work and send you PR... :)

bkaradzic commented 8 years ago

Btw, I really like this part about library:

Portable and embeddable

    Written in portable C++03.
        Only uses std::vector from the standard library
    Exception free
    Doesn't use virtual functions
    Small (~90 KB)
    Permissive (MIT)
bkaradzic commented 8 years ago

It's Orthodox C++ compliant: https://gist.github.com/bkaradzic/2e39896bc7d8c34e042b :)

graphitemaster commented 8 years ago

That is how C++ is supposed to be written. I too share very similar regards to this "modern C++" world. I have a style guide I use that defines this stuff too: https://github.com/graphitemaster/neothyne/blob/master/docs/STYLE.md