I don't have an opinion on at-top vs. at-bottom and churn vs. benefits of regularity, but I do really dislike it when fields are mixed with methods. I often like to know I've seen everything so I can feel like I have a handle on the whole state machine of the class and for this purpose I find it useful to see all the fields together.
JSContext/JSRuntime are special, though and I'm not sure stuffing all the fields up top would improve anything. For these cases, where the quantity of fields is more-or-less forced, what I like is the pattern of bundling up groups of related fields and methods into classes that are stored directly as fields of the JSContext/JSRuntime. These classes can be defined in appropriate functionality-specific headers, clearly state and assert their own local invariants on their fields, and really cut down on the size of Runtime.h. This is what we're already doing with GCRuntime, RuntimeCaches, OffThreadPromiseRuntimeState, LCovRuntime, GeckoProfiler(Thread|Runtime), and MachExceptionHandler, and I think it'd be great if we did it way more. In the limit, we could have all the runtime/context state partitioned into such classes and JSContext/JSRuntime would then just be boring lists with ctor/init/destroy methods. I think this would be great. On Thu, Sep 28, 2017 at 3:55 AM, Benjamin Bouvier <b...@mozilla.com> wrote: > Agreed with Lars. In particular regarding (b), I tend to read unknown code > by starting with the data (the "what") then look at how it's manipulated > (the "how"). This stylistic change would be a huge regression to me, and > there are already some places where the fields are grouped at the end, > which I've found these very confusing. > > That being said, I agree having the fields grouped is much better than the > messes that are JSContext or JSRuntime, sometimes intertwining fields and > methods which don't even seem related to the fields defined just before. > > Cheers, > Ben > > 2017-09-28 9:10 GMT+02:00 Lars Hansen <lhan...@mozilla.com>: > > > I dislike this proposal. (a) A lot of the code I work with already have > > fields-at-the-beginning as the predominant pattern in the smaller classes > > (jit, wasm) so this would be major churn for no gain. (b) For large > > classes this is an anti-pattern, like having all the vars at the > beginning > > of a function in C; it separates the data from the functions that work on > > that data. (c) It brings private and public parts of the code close > > together, and separates public data from public methods. > > > > --lars > > > > > > On Wed, Sep 27, 2017 at 6:18 PM, Jason Orendorff <jorendo...@mozilla.com > > > > wrote: > > > > > Hi everyone. > > > > > > I'd like to add a style rule: in a struct/class/union, put all the > fields > > > (that is, non-static member variables) together, at the end. > > > > > > Maybe this is dumb, but while working with Rust I got used to this. > Rust > > > doesn't allow methods to be defined within a struct or enum. And wow is > > it > > > easier to see what's going on. "Show me your tables..." and all that. > > > > > > (At the end is better than at the top because you *can't* always put > all > > > the fields together at the top. Sometimes you have to declare a type > > first. > > > Sometimes it's a nested class that's rather a lot of code.) > > > > > > If nobody objects in a day or two, I'll add the new rule here: > > > https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style > > > > > > -j > > > _______________________________________________ > > > dev-tech-js-engine-internals mailing list > > > dev-tech-js-engine-internals@lists.mozilla.org > > > https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals > > > > > _______________________________________________ > > dev-tech-js-engine-internals mailing list > > dev-tech-js-engine-internals@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals > > > _______________________________________________ > dev-tech-js-engine-internals mailing list > dev-tech-js-engine-internals@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals > _______________________________________________ dev-tech-js-engine-internals mailing list dev-tech-js-engine-internals@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
