openframeworks / openFrameworks

openFrameworks is a community-developed cross platform toolkit for creative coding in C++.
http://openframeworks.cc
Other
9.98k stars 2.55k forks source link

slight cleanup of ofParameterGroup.cpp [please merge for future work] #8131

Closed artificiel closed 1 month ago

artificiel commented 2 months ago

no code changes; naming, format, etc.

roymacdonald commented 1 month ago

This looks ok to me. But why removing the using std::string, etc? It is harmless as it is in the cpp file.

artificiel commented 1 month ago

@roymacdonald indeed removing the using in this case is not related to pollution but more of a standardisation; as you say it does not harm, but it does mean reading vector in the middle of the code does not necessarily mean std::vector and requires a confirmation. granted it's very minor, but in the context of "impersonal core code" if the gain is not truly substantial (most of the using where for things that got called once or twice) it's not a style requirement either.

(if i had to guess all these 'using' directives were simply plopped in to satisfy the compiler when using namespace std was recently removed from the whole OF, which is faster than tracking all the individual calls).

i'm in favour of specifying std:: in general as it correlates 1:1 with interface code (which we are seeing more and more, from declaration with constructors to template and constexpr code) and diminishes the possibility of having the idea of doing using namespace in an interface.

roymacdonald commented 1 month ago

I personally really disliked when using namespace std was take out of OF because lots of things broke and manually adding it where needed was just spending time. It makes sense though and in the long run it is the correct choice. In the case of this particular file, I would guess that throwing in all those using std::* was just the quick way to deal with teh removal of using namespace std.

If the h file is using std::string etc, then it makes sense to have the cpp file as such.

Thus, I think it is ok to merge this PR. I can do such but I'd rather have some else's opinion. @ofTheo maybe?

ofTheo commented 1 month ago

Looks good to me!