-Wshadow about rawspeed HOT 9 CLOSED

I admit that this warning might help in finding real issues and I was trying to get an idea what other people think about the above struct A situation. So the whole -Wshadow situation seems controversial at best, which is the reason why it is not included in -Wall. It just produces way to much noise with perfectly valid code. I found an interesting bug report: https://llvm.org/bugs/show_bug.cgi?id=16088, which led to https://reviews.llvm.org/rL267957.

So from what I see at a short glance, this would be 'ideal'. Maybe we could disable the warning in general but enable it if we have a proper clang compiler? Or may just postpone this issue until we ran out of more important ones :).

from rawspeed.

We did have had issues in darktable that would have been detected by -Wshadow, that is why the flag got enabled.
Let's just disable it for now completely, and probable re-enable it as error after most of your stuff is upstreamed. Just prepend # before these 3 lines: cmake/compiler-warnings.cmake

from rawspeed.

Done. Close after #31 is merged.

from rawspeed.

I'd appreciate if we could agree on some consistent way of how to deal with the naming issues surrounding class members before any global search-and-replace type of change gets pushed.

Currently there are different inconsistent patterns in the code like starting some member names with an m but not others. Then there are getter/setter methods for some properties but not for others. From what I remember they are at least pretty consistently prefixed with get and set.

My personal preference is to have a concise notation, so I am no fan of the 'Windows-flavored' habit of prefixing members with an m (also to some extend, because I am no fan of Windows ;)). So having members named size or pixelPitch is my preference.

Having that said, I also like the libstdc++ way of specifying getters/setters like vector::size() which obviously requires to come up with a different name for the private member, like size_ or if it 'must' be mSize. But since the code now uses the get/set prefix pattern, sticking to that seems a good choice, though. On the other hand, if we should decide to generally discourage direct access to member variables and roll out tons of new get/set methods, then the overall code-bloat would be smaller if we chose the libstdc++ way and live with an additional character pre- or postfix for the member variables. That would also resolve the issue with the constructor member initialization mentioned above.

What is your opinion on this subject?

from rawspeed.

I'd appreciate if we could agree on some consistent way of how to deal with the naming issues surrounding class members before any global search-and-replace type of change gets pushed.

See #48. Does not change members.

Currently there are different inconsistent patterns in the code like starting some member names with an m but not others. Then there are getter/setter methods for some properties but not for others. From what I remember they are at least pretty consistently prefixed with get and set.

My personal preference is to have a concise notation, so I am no fan of the 'Windows-flavored' habit of

< reworded >
no prefixing members with an m
(also to some extend, because I am no fan of Windows ;)).
having members named size or pixelPitch is my preference.

Agree on all three points here

Having that said, I also like the libstdc++ way of specifying getters/setters like vector::size() which obviously requires to come up with a different name for the private member, like size_ or if it 'must' be mSize. But since the code now uses the get/set prefix pattern, sticking to that seems a good choice, though. On the other hand, if we should decide to generally discourage direct access to member variables and roll out tons of new get/set methods, then the overall code-bloat would be smaller if we chose the libstdc++ way and live with an additional character pre- or postfix for the member variables. That would also resolve the issue with the constructor member initialization mentioned above.

No comment on getters/setters. I see no reason for their existence here. Especially since it's a copylib with no stable/guaranteed API/ABI/

What is your opinion on this subject?

from rawspeed.

No comment on getters/setters. I see no reason for their existence here. Especially since it's a copylib with no stable/guaranteed API/ABI/

Ok. I'm no fan of getters/setters for their own sake. I think they are mandatory in cases where the setting of a member without checks or without also updating related state would break the invariants of the object, meaning the object would end up in an inconsistent/broken state. A good example would be the errors member of the RawImageData class where you can directly modify the vector while forgetting about the accompanying mutex.

So we conclude as follows?:

• we'll stick to the get/set prefix and add or even remove accessors where appropriate
• get rid of the m prefix (sooner or later)

from rawspeed.

A good example would be the errors member of the RawImageData class where you can directly modify the vector while forgetting about the accompanying mutex.

Yes, that is a good example indeed.

• we'll stick to the get/set prefix and add or even remove accessors where appropriate
• get rid of the m prefix (sooner or later)

I believe i can agree to that in general.

from rawspeed.

close or still not happy with this?

from rawspeed.

Just forgot about :)

from rawspeed.