Closed Bug 223255 Opened 21 years ago Closed 15 years ago

deCOMtamination naming nit: no "Get" in infallible/non-null accessor names

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: brendan, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

See bug 222134 comment 24. If you're deCOMtaminating an XPCOM interface pointer or string getter, which returns a strong ref or owned copy, drop the Get from the front of the method name: - nsCOMPtr<nsIDocument> parentDoc; - aDocument->GetParentDocument(getter_AddRefs(parentDoc)); + nsIDocument *parentDoc = aDocument->ParentDocument(); Let "Get" connote "add a share in ownership" or "transfer ownership", and the lack of "Get" connote "weak reference to named member". It'd be good to get this meme spread about quickly, unless someone objects. /be
ok, I'll use this convention in bug 221866 :)
Some things that we get are unambiguously nouns, but some aren't. (Consider GetStyleContent() vs. StyleContent(). Perhaps it should have been named ContentStyle(), but |nsStyleContent* c = frame->GetContentStyle()| is a little strange.) We certainly haven't been doing this so far, and it might keep things more consistent if we don't start, although I was originally leaning this way (but roc wasn't, IIRC).
If your weak link accessor computes something and can fail (return null), then it should be called something other than the member-like noun-name. GetWeakFoo()? Past naming mistakes are no excuse for other mistakes! ;-) /be
We don't need to use method naming conventions to indicate whether a strong or weak reference is returned. already_AddRefed does that for us. We might want to classify methods returning pointers into the following three classes by reading their signatures or call sites: A) Methods that never fail and never return null. B) Methods that never fail but may return a null result. C) Methods that can fail and return null to indicate failure. I don't believe that it is very important to distinguish B from C. The caller has to handle null in either case, and how null should be handled depends not just on whether the callee failed, but on whether the caller wants to recover, whether the caller can handle a successful null result or it treats a successful null result a failure anyway, etc. This can just be documented in the method's header file. I do agree that it would be helpful to identify methods of type A, methods that cannot return null and are therefore safe in chains of GetFoo()->GetBar()->GetBaz()->SayHelloKitty(). I'm not sure what the best way would be to do that. I guess I could swallow brendan's suggestion and have non-Get methods like PresContext(), StyleContext(), StyleContent() etc ... I could live with noun-ambiguous names. For one thing, these methods would usually be taking no parameters. Such methods should assert that their result is non-null. I fear getting from here to there, though. A search and replace script would work for some names, but not for all.
> A) Methods that never fail and never return null. > B) Methods that never fail but may return a null result. > C) Methods that can fail and return null to indicate failure. What I had in mind was distinguishing A from B and C via Foo() instead of GetFoo(). I agree on B and C collapsing into the same state that requires care in calling. Given already_AddRefed (which is far from universally used), I agree that we don't care about strong vs. weak -- except again, it is important to any reader of doc->Foo()->Bar()->...() to know that Foo and Bar return weak refs. And for the reader, it helps to drop the Get. Now too much of this can lead to Hungarian notation, which serves no purpose with a good compiler other than to ugly-up names with type and mode cybercrud prefixes. But here, I'm not asking that unnecessary information be encoded in the name -- I'm arguing for removal of a misleading verb prefix, Get. It's true that Get is overloaded in OOP in general, but in Mozilla, we can make it mean "fallibly compute or acquire some kind of strong ref" if we choose. At least, I think we can -- if we can keep pushing already_AddRefed, a latecomer to the Mozilla party, we can promulgate this Foo() over GetFoo() naming convention. As for who will do what, I volunteer to go over bryner's changes and make the necessary patch, in a little while (after 1.5a is out). /be
so should we differentiate between functions returning an already_AddRefed (strong) and ones returning a rawpointer (weak)?
Jonas, there is no need to differentiate between them... name a reasonable use case where such differentiation would help? ;)
sicking: I think roc's point is that if people use already_AddRefed<T> for strong ref return type, then the method naming convention can continue to be Get (I agree: Get implies strong), and more to the point, people can't go wrong by calling rv = doc->GetFoo()->GetBar(getter_AddRefs(bar)) if GetFoo returns an already_AddRefed, because the compiler will catch the mistake. Readability doesn't weigh heavily here. Some might prefer yet another naming convention to distinguish GetFoo from GetBar, but I don't see the point, and I think another convention would be (a) uglier; (b) not adopted widely or at least consistently. So, I think no new naming convention is needed for strong ref r.v. (via already_AddRefed) vs. strong ref return via out param (GetBar in the example). Both are valid forms of getters; both have their uses, even though the GetFoo type is not usable in a pure XPCOM interface. But, if you mean should we distinguish between Foo(), which falls into roc's class (A) and returns a weak ref (and never null) from a GetFoo() that returns an already_AddRefed, then of course I agree! I'm all in favor of this Foo() convention. ;-) /be
Fixing summary. /be
Summary: deCOMtamination naming nit: no Get in weak-ref accessor names → deCOMtamination naming nit: no "Get" in infallible/non-null accessor names
I'd like to hear dbaron and bz voice an opinion before we seal this in blood.
I do feel that it would be good to differentiate A from (B or C). I'm tired of inconsistencies in whether things are null-checked... ;) The actual convention we decide on for differentiating does not matter to me so much, really. One thing with the Foo() converntion is that we have existing classes in the tree that use Foo() in somewhat (but maybe not quite) that way (eg nsXULElement, some stuff in http, etc). We would want to look over such uses, as possible, and see whether they correspond to the new meaning invested in the convention.
There's also D. Methods that may fail and may also return null as a non-failure condition. I encountered one of these with my nsIDocument deCOMtamination.
Methods of type D should always return nsresult. I like the short names for methods of type A since they'll often be used in a chain. I say we go ahead with this. I will start the ball rolling by converting some obvious methods to the new syntax. I don't see what bz was referring to in nsXULElement...
Hmm ... now that I look at methods like "nsStyleStruct* GetStyleData(nsStyleStructID aSID);" I start to wonder whether there are going to be ambiguities. If a method happens to return a pointer and its name does not start with "Get", one might just assume that it never returns null ... what what if it's not actually a getter method? I guess it's most confusing when the name is not obviously a verb, a la dbaron's comment #2. So how about other naming conventions? Can we name methods starting with, say, "The"? nsIPresContext* ThePresContext() { return TheStyleContext()->TheRuleNode()->ThePresContext(); } nsIPresContext* APresContext() { return AStyleContext()->ARuleNode()->APresContext(); } nsIPresContext* _PresContext() { return _StyleContext()->_RuleNode()->_PresContext(); } nsIPresContext* MPresContext() { return MStyleContext()->MRuleNode()->MPresContext(); } Hmm, I dunno.
My gut feeling is that the problem already exists if functions have so similar names but don't do similar things, independant of this newly proposed naming convension. In such cases the function should simply get a different name. I'm not sure which exact functions you are talking about so I could definitly be wrong.
I have one more crazy idea: use GET for emphasis: nsIPresContext* GETPresContext() { return GETStyleContext()->GETRuleNode()->GETPresContext(); } "Go and fetch me a prescontext, son, and if you can't find one, DON'T BOTHER COMING HOME"
Ah, yes, those would have to be changed.
GETMeOutOfThisUglyNamingConvention(); Please, can't we just use nouns and noun phrases? /be
Except methods, denoted with (), are actions, which are verbs. Naming them pseudo-imperatively as nouns is common enough abuse, I suppose. However, the rule: "A method expressed as a noun, without prefix Get, acts to retrieve a strongly referenced object" (or the reverse) while good for code brevity, and easy for mavens to understand, is very hard for learners to absorb. This is because they are asked to detect the absence of a meaningless qualifier as a trigger to deducing the semantics. That chore is not easy, concrete thinking for them. Slightly clearer is to use "Weak" instead of "Get" for weak references. Both method kinds are getters, implicitly, so why use Get on only half of them? That is inconsistent. The business of verb abuse remains, but at least with a "Weak" method prefix, the status of the object so retrieved is blindingly clear. It is also a warning to the user that invoking such a method results in an object that is not as formal as it might otherwise be. "Weak" thus acts as a very meaningful qualifier. I argue that the benefits of marking when an object must be "fully referenced" are far outweighed by the benefits of making code more accessible so that learners' eyes can gaze with confidence upon it. Both masters may be served with "Weak", but not with "Get".
(Verbosity for learners? Not for me; I'm a Unix/C hacker. Weed out those who can't keep up without crutch-words, Hungarian notation, and other such noise ;-) A couple of counterpoints: In general, learners are not edified for long before they become adept, but everyone suffers from the verbosity, forever. Is the Unix cp command better named copy? I think not. That's a general point; on to specifics. I challenge the assertion that "[the noun-phrase for weak-ref fetchers rule] is very hard for learners to absorb." What evidence is there for "hard", never mind the otiose "very"? On the contrary, rv = doc->BaseURI()->GetSpec(spec); is less ambiguous, more concise, and arguably easier to learn than rv = doc->GetBaseURI()->GetSpec(spec); where Get is used in two different, incompatible ways. (And as all of us originally on this bug agreed, we don't want GetBaseURI, in the above a method *not* returning an nsresult and not adding a reference, to be confused with GetSpec, which is a pure XPCOM method (albeit with string result). GetFoo or a similar naming convention should mean XPCOM/strong; WeakFoo or Foo should mean non-XPCOM/weak.) So, on to "Weak" vs. "". "Weak" is not a verb, so I'm not sure what happened to the first paragraph's argument. More, the benefit of Weak littering the code, when with naked data member references (as in private/protected class/subclass code) we would have had: rv = doc->mBaseURI->GetSpec(spec); which is more like the noun-phrase idea than any other proposal, seems to both belabor the obvious, and make a naming distinction without a difference. And again, it taxes everyone outside the class implementation with the burden of "Weak" noise substrings. I think we still have consensus among the deCOMtaminators. If so, I'll document this under http://www.mozilla.org/projects/xpcom/. /be
> Both method kinds are getters, implicitly Not really. The methods we are talking about here are what would just be member accesses if it were not for possibly having to call cross-module and for this pesky data protection thing people like to have. ;) Consider how the following code matches with the concept of methods as "verbs": foo->Bar() = "test"; Where Bar() is declared as: char*& Bar(); Think about what the code does and how easy it is to describe if the method is considered to be a verb vs how easy the it is to describe if the method is considered to be a noun. In my opinion, blinkering yourself in the way you describe is highly detrimental to understanding the code you are looking at; catering to such blinkered thinking should not be an API design criterion.
Brendan, mind summarizing what we've agreed on in terms of the A,B,C,D classification of comment 4 and comment 12?
A) Methods that never fail and never return null. B) Methods that never fail but may return a null result. C) Methods that can fail and return null to indicate failure. D. Methods that may fail and may also return null as a non-failure condition. Summary rule: For A, use Foo(). For B, C, and D, use GetFoo(). As roc points out, the issue of weak vs. strong reference result is independent of fallibility (Nigel, you wouldn't want us saying "InfalliblyWeakFoo()" or "InfalliblyStrongFoo()", I trust!) and is type-checked by the compiler. If you have a case A where the result should be a strong ref, return an already_AddRefed<T>. Distinguishing B-D by naming convention seems fruitless to me, based on our experience with XPCOM and deCOMtamination. /be
What brendan said, but I have one further question. For methods of type D, which should we prefer? nsresult GetFoo(nsIFoo** aResult); // returns strong (addrefed) ref nsIFoo* GetFoo(nsresult* aResult); // returns weak ref ? I think I prefer the latter for efficiency, although it departs further from XPCOM.
Oh, yeah. We do have agreement on the issue of methods of type A. Make it so! As for comment 26, I think I favor the second syntax. As far as that goes, would it make sense to have the methods handle a null nsresult* and to default it to null, a la do_QueryInterface and company? That will make the lives of callers who don't care about why the pointer is null maximally simple.
> I challenge the assertion that "[the noun-phrase for weak-ref fetchers rule] is > very hard for learners to absorb." What evidence is there for "hard", never > mind the otiose "very"? Viewing others' code is an obvious scenario for example-driven learning. Two fundamentals of example-driven learning are to be concrete and explicit. The absence of "Get" implying extra semantics does not help either fundamental. Say it as plain as a tree on a hill if you want it understood. > Unix brevity. With 23 years allegiance I agree. But brevity will not correct an absence of concrete information. It just codes (in the symbolic sense) information already present into less space. Nothing wrong with that; it is no argument in favour of an reverse-absent indicator, though. > What happened to the first para's argument. I merely point out that there is a bigger picture, but declined arguing it here since only the smaller picture "Get" / "Weak" is on-topic. > Weak is not a noun I proposed it as an occassional prefix, just as Get is proposed to become an occassional prefix. prefixes are word-parts, not words or nouns. All this: ' "Weak" is not a verb ... "Weak" noise substrings.' seems a little blurry. You propose to litter the code with Get and yet equal littering with Weak (or some other term) is apparently worse on some yet-to-be-defined basis. On Boris' remarks: I accept the world is wide; and accessors may do more than just access local members. And yet the beauty of COM/XPCOM is in its implicit simplicity for *ordinary* programmers. At the C++ level there is plenty of punctuation soup to be had, but that shouldn't be used to deconstruct the very common major cases that are to suffer naming conventions under this bug. And yes, of course, constructors and function/pointer references can have noun names. My remarks were restricted to explicit stating of method names in a typical way. Now [expects howls of anguish from everyone], it is natural that XPIDL definitions will follow the lead of C++ uses of XPCOM at some point - the trickle down effect. Yes I know there are implementation classes that are separate in some cases. The users of scripted XPIDL are certainly not all UNIX lovers, they do not embrace that ethos, and they may even be in majority Microserfs. Regardless of their Lin/Win preference, many are less than code wizards. If you want to promote Mozilla as a robust platform and draw a developer crowd, this audience is important, because amongst the horde are a few gems that will be very helpful folk. Your decision here is not just a club decision about deCOMtaminating; it has real impact on the future digestability of the platform. There are more average programmers available to be enthusiastic about Mozilla than there are highly competant ones; that is the market reality. What goes for HTML and JavaScript goes for XPIDL, albeit to a lesser degree. Surely you do not want a large impedence mismatch between a set of XPIDLs suitable for broad consumption and an underlying implementation? That is foolish. Or do you propose a formally maintained translation of C++ XPCOM semantics into a semantically separate set of XPIDL names? I doubt that would ever work except in the most restricted case; and it would be a huge maintenance and performance worry. I do not mean the simple translation done be xpidlgen. And yes,I know that weak references are handled extensively when XPconnect is involved. In all this I have yet to see one good argument why Get is as good as Weak, evenfor C++ coders, except that 4 letters are involved rather than 3, and that brevity suits a few ports of Mozilla. At no time have I suggested long names are better. If I were to substitute a proposal for "W" or "Wk" over "Get", then by all logic here it should get up? I merely point out the fallaciousness of debating a single character. It is the reverse-absence of "Get" that is the most worrying, though. It raises the barrier to access. It forces more menial beginner tasks onto your expert time, because there will be fewer motivated learners helping out.
regarding comment 27: I don't like having the nsresult as an optional argument for the very reason that it is easy to ignore. We already have way too much code that doesn't do proper errorhandling so we should IMHO not encourage that. IMHO If a function can fail but can also return null as success-value it should always use: nsresult GetFoo(nsIFoo** aResult); as calling convention. I realize that it's going to be more refcounting, but i doubt that that is going to make a big difference. It's much more important to have code that functions well rather then to save those extra few percent in performance.
> And yet the beauty of COM/XPCOM is in its implicit simplicity for *ordinary* > programmers. We're very explicitly NOT talking about COM/XPCOM here. We're talking internal, probably-non-virtual, probably-inlined, certainly-not-XPCOM methods. > it is natural that XPIDL definitions will follow the lead of C++ uses of XPCOM > at some point - the trickle down effect. 1) We're not talking about XPCOM 2) The proposed syntax is (on purpose) fundamentally not compatible with XPIDL or the way XPCOM works, thus could not trickle out to XPIDL. > And yes,I know that weak references are handled extensively when XPconnect is > involved. XPConnect is not involved in any way in any of the methods under discussion. Please do your research on the background to this discussion before bringing in a whole slew of irrelevancies, ok?
> For methods of type D, which should we prefer? > > nsresult GetFoo(nsIFoo** aResult); // returns strong (addrefed) ref > nsIFoo* GetFoo(nsresult* aResult); // returns weak ref D) Methods that may fail and may also return null as a non-failure condition. In light of bryner's definition of D, how is the nsIFoo* (nsresult* aResult) form any more efficient? The caller has to check aResult no matter the return value (null does not mean failure). I say use XPCOM for all (D) variants. So I agree with Sicking's comment 29. /be
nigel: deCOMtamination means elimination of gratuitous XPCOM uses. No one said "Weak" loses to "Get" because it has one more character. People use Get all over, it's part of XPCOM (XPIDL => C++ prepends Get and Set to generate methods for an attribute), we are used to it. "Weak", we are not used to, and no one likes it. /be
No problems all. I realise I sound like some kind of space alien when I occassionally contribute to these bugs, but I have a goal of sorts, which (in part) is to stretch thinking out towards learning consequences. If I ever manage to launch my rocket one day I want you all to be used to me by then and not panic ;-). My daily work practices are somewhat different to yours. I have spent many years in the roles you occupy (but rarely in open source), and I appreciate the frustration you have with me (in the role I take here). You'll just have to assume my remarks aren't in English for now. Boris & Brendan, I do understand that XPCOM is not "related" to the subject at hand in a simple API sense. But my interest in relatedness, and in exposure of symbolic information, is not always cast in terms of connected bits and bytes. There's lots to argue here, especially many assumptions sprayed around, but you've had a full dose of me, so I'll shush up for now.
>nsresult GetFoo(nsIFoo** aResult); doesn't that make it equally easy to ignore the return value?
> In light of bryner's definition of D, how is the nsIFoo* (nsresult* aResult) > form any more efficient? Because you can return a weak pointer and avoid the cost of reference counting. >>nsresult GetFoo(nsIFoo** aResult); > doesn't that make it equally easy to ignore the return value? Amen. In fact, I claim that if you use the "nsIFoo* GetFoo(nsresult*)" version, it's harder to ignore the return value because (assuming we don't accept null there) you have to provide a variable to put it in.
roc: you're right that the return value is more efficiently propagated than out params, on many compiler/target-architectures (and never less efficiently). The issue of weak vs. strong reference seems irrelevant, again: one could return a weak ref directly, or via the out param. The only issue is efficiency, or so I think you mean. So, in the non-null weak pointer case, the caller does not need to load the nsresult out param that it passed by address. Only in the null return value case must that result code be checked to disambiguate null-ok from null-failure. This could make for a noticeable cycle savings over against the proper XPCOM signature, depending on call frequency. I'm not sure nsIFoo* (nsresult*) makes it harder to ignore the nsresult, though. If my interpretation of your efficiency argument is correct, people will ignore the nsresult for non-null returns, and the only hazard is that they'll treat null as null-ok always and thereby hide a failure. That seems about as likely to me as people "pulling a hyatt" ;-) and ignoring the nsresult r.v. of a proper XPCOM signature. Do we really want to document and promote this signature as yet another special form? What do others think? /be
> The issue of weak vs. strong reference seems irrelevant, again: one could return > a weak ref directly, or via the out param. Except that we don't allow weak pointers to be returned in out params, currently.
IMHO we should not add that as a new signature. First of all i seriously doubt that the extra addref/release will make a big differance. So far we havn't really seen huge improvements speedwise from the deCOMtamination, at least of content, and we've been changing a lot of functions, not just reducing refcoutning but also converting virtual functions into inline ones. I'm not saying it's a waste of effort, the code looks a lot nicer and there has been some speedincrease. But I doubt that the few functions that fall into D will make a difference, feel free to prove me wrong and i'll reconsider. Second a function returning an nsresult should be indication enough for people that errors need to be checked for. People seems to have fallen out of this habit, but hopefully the deCOMtamination process will help this since we'll have fewer functions that needlessly return an nsresult.
I basically agree, Jonas, I don't want to introduce a new style of signature, I just want to make sure we make the decision for the right reasons. Converting nsIFrame::GetFrameType to return a weak rather than a strong pointer --- but leaving it virtual, since it is overridden in many places --- reduced btek Tp by 1-2%. Those addrefs and releases do matter. However I agree with you that functions of type D are not so common. (In fact it might be better to try to avoid creating functions of type D if we can, because they're the most complex for users to deal with.)
Re: comment 38: deCOMtamination can win significant time perf improvement, but it always wins code size reductions. Re: comment 39: I agree we should avoid type D, and not over-optimize it with yet another special form in deCOMtaminated code. Sounds like we're in agreement again. Should I write all this up and put it someplace on mozilla.org? Perhaps under projects/xpcom, but perhaps not, since it's deCOMtamination. In case we put the deCOMtamination doc elsewhere, a link from somewhere under projects/xpcom would be good. Rob, don't you have a deCOMtamination doc on your site? /be
I wrote: > [deCOMtamination] always wins code size reductions. I meant object code, but source code becomes simpler too, substantially (the two are related). Just wanted to make sure no one is looking for cycle savings only, or ahead of code size savings. /be
"Com addresses software design in a very pragmatic way", from Box's Essential COM. Removing COM from where it shouldn't have been can be and should be doc'ed under "what not to do, what we are doing about it, and how you can help". :-)
Assignee: layout.misc-code → nobody
QA Contact: layout.misc-code
Blocks: deCOM
Should this bug still be open?
I don't think it needs to no. Not sure what resolution to do here since this was more of a policy change than an actual specific code change. Marking FIXED since we've followed this policy fairly well since.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Could someone update https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide to talk about noun-phrase-named infallible getters vs. GetFoo-named fallible ones? /be
Keywords: dev-doc-needed
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.