I’d like to gather some feedback from other developers on the topic of using the this keyword when accessing public members and methods.
We already do this in some parts of Blender, but the style-guide doesn’t mention it at all.
This is what would be added to the style-guide:
Use this-> prefix when accessing methods and public data members.
class X {
float my_float_;
public:
int my_int;
public:
void foo() {
/* `my_int` is public, so `this->` is used */
this->my_int = 42;
my_float_ = 3.14f;
}
private:
/* `foo` and `bar` are methods, so `this->` is used */
void bar() {
this->foo();
}
void baz() {
this->bar();
}
};
I don’t like this idea, for a few reasons. (Note: “Don’t like” does not mean “hate” In general I prefer more documentation rather than less, but I’m not so sure about this one… )
Off the top of my head, I can’t think of other coding standard that does this. Not that Blender needs to do “what everybody else does”… but if other standards don’t do this, maybe Blender shouldn’t? I generally follow Google’s C++ coding standards for my own work, not “because Google”, but because they do a decent job explaining the “why” behind the decisions. Here’s their section on variable names: Google C++ Style Guide
If a method is so long and convoluted that it makes determining if a variable is a part of the method or a member of the class, maybe refactoring is the best way to solve this v. using this->
Is this proposal addressing a problem that has been an issue in the past? Or is this addressing general code cleanup and documentation?
If you look at our style guide it is pretty close to that of google. The variable names section matches ours. From a quick search on their page, I can’t seem to find anything on the this keyword.
To me this is more about “can this function mutate the object?” and less about how long a function is. Here is a quote from @jacqueslucke:
When I read other peoples code that doesn’t use this when calling methods, I’m always unsure if this is passed in as implicit parameter or if it’s a free-standing function.
Using this-> makes it immediatly clear that the function being called can use the this pointer.
We’re already using this style in parts of Blender but it was never written into the style-guide. So this is an attempt at making it clear for everyone, so we can be consistent in the future.
Thanks for the additional information! Makes sense to me. And since it sounds like this is formalizing something that is already in practice, I’m going to change my vote and thumbs up this… and of course, end up following what eventually ends up in the style guide.
For something that is aimed to become an official style guide it is important to have motivational part: what is the problem which the proposal is solving.
The example of the proposal seems inconsistent with itself. The code follows the current naming convention for methods without distinguishing private from public ones, but from the text it seems that you also propose to change naming of private methods. Which one is the source of truth?
P.S. Link to the trailing underscore seems broken?
Sorry, I must have misread members as methods, and mixed together with some further questions from my side, and caused confusion.
Thing is, if the goal is to make it clear whether this is used implicitly, or something is a free-standing function, we can’t distinguish it based on access modifier of a method. So for this goal something else needs to be added to the proposal.
If that is not the main goal to be solved by the proposal, then there is this downside of code becoming very verbose. Which I am not sure how I feel about to make required by all developers to follow.
we can’t distinguish it based on access modifier of a method
Could you elaborate on this a bit. I’m not sure I understand.
I should have clarified that this is something we already do some parts of Blender, that the style-guide doesn’t meantion anything and that this proposal is meant to resolve this situation.
I’m not sure that this will make a lot of code more verbose. It’s not like we’re adding dozens of characters here - just 6. I think that;s a good trade-off for better readability.
When I read other peoples code that doesn’t use this when calling methods, I’m always unsure if this is passed in as implicit parameter or if it’s a free-standing function.
The main case where it would matter is when a method or a member is used in lambda. When you see out = foo(); or some_interesting_output = my_int; in lambda you don’t know how careful you need to be about the lifetime of lambda and the object, because there might be an implicit this involved in the call. If you see out = this->foo(); or out = this->my_int; you will know for sure that there is this.
Due to the naming convention for private members, seeing out = another_int_; you can deduct that you access a private field and that implicit this is involved. It is less straight-forward, but you can see if from seeing a local piece of code.
Now, if you see out = bar(); you don’t know if there is implicit this involved, and as per current proposal you don’t have to use explicit this as bar() is private, and the proposal says " Use this-> prefix when accessing methods and public data members".
There are two ways from this situation:
Always require this-> when accessing class members and methods. Is a simple rule,. barely takes brain power when typing code, but is also the most verbose. Although, it is also how Python requires you to access object methods and data.
Require to have explicit this-> regardless of access modifiers when in lambda. It solves all cases of possible confusion in the practical example of what this proposal addresses, and is less verbose.
If we follow the latter, then question is: do we really want to increase verbosity level when accessing methods and data outside of lambda?
There surely might be something else what we can do, but those are immediate thoughts on the subject.
The actual PR actually seems much more clear, especially the example. With all the discussions in there the proposal seems fine, and addresses all the points I’ve risen in the previous comment.
It worth updating the example in the post here to the latest actual state. And perhaps not split discussion between two places in the future.
In general I prefer more documentation rather than less, but I’m not so sure about this one… )