Have offsetFromContainer() / offsetFromAncestorContainer() take a RenderElement& instead of a RenderObject*. The argument passed is never null and the type should be a RenderElement as the argument is a container.
Created attachment 239840 [details] Patch
Created attachment 239843 [details] Patch
Comment on attachment 239843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239843&action=review rs=me > Source/WebCore/rendering/RenderBox.cpp:1915 > + auto* container = this->container(repaintContainer, &containerSkipped); IMHO, "auto" is bad here, the type is 100% non obvious. > Source/WebCore/rendering/RenderBox.cpp:1964 > + auto* container = this->container(ancestorToStopAt, &ancestorSkipped); ditto > Source/WebCore/rendering/RenderBoxModelObject.cpp:2640 > + auto* container = this->container(); ditto > Source/WebCore/rendering/RenderObject.cpp:1874 > - auto nextContainer = currContainer->container(); > + auto* nextContainer = currContainer->container(); gosh I hate auto sometimes... It is just making code hard to follow :(
Comment on attachment 239843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239843&action=review >> Source/WebCore/rendering/RenderBox.cpp:1915 >> + auto* container = this->container(repaintContainer, &containerSkipped); > > IMHO, "auto" is bad here, the type is 100% non obvious. I think Darin prefers auto* in this case because this way it always uses the same type returned by container(). For e.g., it is quite common for people to assign to a RenderObject* even though the function returns a RenderElement* nowadays. That said, I don't feel strongly either way. In this case, I do agree the type is not obvious.
Created attachment 239846 [details] Patch
Comment on attachment 239846 [details] Patch Clearing flags on attachment: 239846 Committed r174722: <http://trac.webkit.org/changeset/174722>
All reviewed patches have been landed. Closing bug.
At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious?
(In reply to comment #8) > At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious? For me it is all about reading/reviewing/understanding code. I would rather spend 5 minutes making the code explicit when writing it than creating cognitive load for anyone reading the code. I like that "auto" always get the right type even after refactoring of disconnected parts of the code but the code readability suffers so much that the cost far outweigh the gains.
(In reply to comment #9) > (In reply to comment #8) > > At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious? > > For me it is all about reading/reviewing/understanding code. I would rather spend 5 minutes making the code explicit when writing it than creating cognitive load for anyone reading the code. What I’m not clear on is whether this cognitive load is an intrinsic result of this coding style, or a result of the fact that we are accustomed to the other style. I often don’t need to know the types of things; the names of the functions we are calling on them seem sufficient. Every time I see an explicit type name such as RenderObject I wonder if the type is correct. We do not see the type names of function results or data members each time we use them. Is there some reason we need these type names urgently for local variables, but not those other things?
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious? > > > > For me it is all about reading/reviewing/understanding code. I would rather spend 5 minutes making the code explicit when writing it than creating cognitive load for anyone reading the code. > > What I’m not clear on is whether this cognitive load is an intrinsic result of this coding style, or a result of the fact that we are accustomed to the other style. I often don’t need to know the types of things; the names of the functions we are calling on them seem sufficient. Every time I see an explicit type name such as RenderObject I wonder if the type is correct. > > We do not see the type names of function results or data members each time we use them. Is there some reason we need these type names urgently for local variables, but not those other things? Funny you mention that, you always have me remove my local variables :) I do use local variables to make types explicit whenever I feel that it clarifies the flow. Let's talk about this face to face when you have 5 minutes, this is not the best forum.