There is at least potential for performance difference with the extra level
of indirection of `private val` compared to `private[this] val`; so from
that perspective, `private[this]` or closing over an unannotated
constructor parameter is preferable to using a `private val` parameter. On
the other hand, having a field secretly spring into existence as soon an
unannotated parameter is closed over does present problems. On the third
hand, annotating constructor parameters with `private` or `private[this]`
is not the expected Scala way (i.e. unannotated parameters) of accessing
those parameters privately within their respective classes.
In other words, I don't have a good answer, just more things to consider.
On Wed, Feb 12, 2014 at 2:14 PM, Aaron Davidson <ilike...@gmail.com> wrote:
> Thanks for the clarification, Mark. So "private val" generates the Scala
> getter whereas "private[this] val" does not, and the field-ified
> constructor parameter mimics "private[this] val".
>
> However, the distinction between those two seems less important than the
> distinction between a constructor parameter and a field. In particular, the
> existence of the Scala private getter won't affect serialization and makes
> the shadowing explicit. Other than some weird uses of reflection, I'm not
> sure how the difference could cause an issue.
>
>
> On Wed, Feb 12, 2014 at 2:02 PM, Mark Hamstra <m...@clearstorydata.com
> >wrote:
>
> > It's actually a little more complicated than that, mostly due to the
> > difference between private and private[this]. Allow me to demonstrate:
> >
> > package dummy
> >
> > class Foo1(a: Int, b: Int) {
> > private val c = a + b
> > }
> >
> > class Foo2(a: Int, b: Int) {
> > private[this] val c = a + b
> > }
> >
> > class Foo3(a: Int, b: Int) {
> > def getB = b
> > }
> >
> > class Foo4(a: Int, private val b: Int) {
> > def getB = b
> > }
> >
> > class Foo5(a: Int, private[this] val b: Int) {
> > def getB = b
> > }
> >
> >
> > scalac Foo.scala
> >
> > Now let's look at what we've got using `javap -c -private` on the
> resulting
> > classes:
> >
> > public class dummy.Foo1 {
> >
> > private final int c;
> >
> >
> > private int c();
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: getfield #13 // Field c:I
> >
> > 4: ireturn
> >
> >
> > public dummy.Foo1(int, int);
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: invokespecial #20 // Method
> > java/lang/Object."<init>":()V
> >
> > 4: aload_0
> >
> > 5: iload_1
> >
> > 6: iload_2
> >
> > 7: iadd
> >
> > 8: putfield #13 // Field c:I
> >
> > 11: return
> >
> > }
> >
> > ...compare that to Foo2 and note how `c` isn't a plain field in Foo1, but
> > has an accessor method `private int c()`:
> >
> >
> > public class dummy.Foo2 {
> >
> > private final int c;
> >
> >
> > public dummy.Foo2(int, int);
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: invokespecial #15 // Method
> > java/lang/Object."<init>":()V
> >
> > 4: aload_0
> >
> > 5: iload_1
> >
> > 6: iload_2
> >
> > 7: iadd
> >
> > 8: putfield #17 // Field c:I
> >
> > 11: return
> >
> > }
> >
> >
> > Okay? So you really need private[this] if you want to generate plain
> Java
> > fields.
> >
> > Now let's look at what happens when you close over an unannotated
> > constructor parameter:
> >
> >
> > public class dummy.Foo3 {
> >
> > private final int b;
> >
> >
> > public int getB();
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: getfield #14 // Field b:I
> >
> > 4: ireturn
> >
> >
> > public dummy.Foo3(int, int);
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: iload_2
> >
> > 2: putfield #14 // Field b:I
> >
> > 5: aload_0
> >
> > 6: invokespecial #21 // Method
> > java/lang/Object."<init>":()V
> >
> > 9: return
> >
> > }
> >
> >
> > ...which is not the same as what you get if you annotate `b` with
> `private
> > val` -- notice `private int b()`:
> >
> >
> > public class dummy.Foo4 {
> >
> > private final int b;
> >
> >
> > private int b();
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: getfield #13 // Field b:I
> >
> > 4: ireturn
> >
> >
> > public int getB();
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: invokespecial #18 // Method b:()I
> >
> > 4: ireturn
> >
> >
> > public dummy.Foo4(int, int);
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: iload_2
> >
> > 2: putfield #13 // Field b:I
> >
> > 5: aload_0
> >
> > 6: invokespecial #23 // Method
> > java/lang/Object."<init>":()V
> >
> > 9: return
> >
> > }
> >
> >
> > ...however, compare Foo3 to what you get when `b` is `private[this] val`:
> >
> >
> > public class dummy.Foo5 {
> >
> > private final int b;
> >
> >
> > public int getB();
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: getfield #14 // Field b:I
> >
> > 4: ireturn
> >
> >
> > public dummy.Foo5(int, int);
> >
> > Code:
> >
> > 0: aload_0
> >
> > 1: iload_2
> >
> > 2: putfield #14 // Field b:I
> >
> > 5: aload_0
> >
> > 6: invokespecial #21 // Method
> > java/lang/Object."<init>":()V
> >
> > 9: return
> >
> > }
> >
> >
> >
> >
> > On Wed, Feb 12, 2014 at 1:29 PM, Aaron Davidson <ilike...@gmail.com>
> > wrote:
> >
> > > Regarding styling: as we all know, constructor parameters in Scala are
> > > automatically upgraded to "private val" fields if they're referenced
> > > outside of the constructor. For instance:
> > > class Foo(a: Int, b: Int) {
> > > def getB = b
> > > }
> > >
> > > In the above case, 'b' is actually a "private val" field of Foo,
> whereas
> > > 'a' never left the constructor's stack.
> > >
> > > Is this usage kosher, or should we prefer that such fields are marked
> > > "private val" explicitly if they're intended to be used outside of the
> > > constructor? This behavior is often harmless, but it has some evil
> > > implications with regards to serialization and shadowing during
> > > inheritance. It's especially concerning when a field starts out as a
> > > constructor parameter and during a later patch becomes a field, and now
> > > we're serializing it.
> > >
> > >
> > > On Mon, Feb 10, 2014 at 9:00 PM, Aaron Davidson <ilike...@gmail.com>
> > > wrote:
> > >
> > > > Alright, makes sense -- consistency is more important than special
> > casing
> > > > for possible readability benefits. That is one of the main points
> > behind
> > > a
> > > > style guide after all. I switch my vote for (1) to Shivaram's
> proposal
> > as
> > > > well.
> > > >
> > > >
> > > > On Mon, Feb 10, 2014 at 4:40 PM, Evan Chan <e...@ooyala.com> wrote:
> > > >
> > > >> +1 to the proposal.
> > > >>
> > > >> On Mon, Feb 10, 2014 at 2:56 PM, Michael Armbrust
> > > >> <mich...@databricks.com> wrote:
> > > >> > +1 to Shivaram's proposal. I think we should try to avoid
> functions
> > > >> with
> > > >> > many args as much as possible so having a high vertical cost here
> > > isn't
> > > >> the
> > > >> > worst thing. I also like the visual consistency.
> > > >> >
> > > >> > FWIW, (based on a cursory inspection) in the scala compiler they
> > don't
> > > >> seem
> > > >> > to ever orphan the return type from the closing parenthese. It
> > seems
> > > >> there
> > > >> > are two main accepted styles:
> > > >> >
> > > >> > def mkSlowPathDef(clazz: Symbol, lzyVal: Symbol, cond: Tree,
> > > >> syncBody:
> > > >> > List[Tree],
> > > >> > stats: List[Tree], retVal: Tree): Tree = {
> > > >> >
> > > >> > and
> > > >> >
> > > >> > def tryToSetIfExists(
> > > >> > cmd: String,
> > > >> > args: List[String],
> > > >> > setter: (Setting) => (List[String] => Option[List[String]])
> > > >> > ): Option[List[String]] =
> > > >> >
> > > >> >
> > > >> > On Mon, Feb 10, 2014 at 2:36 PM, Shivaram Venkataraman <
> > > >> > shiva...@eecs.berkeley.edu> wrote:
> > > >> >
> > > >> >> Yeah that was my proposal - Essentially we can just have two
> > styles:
> > > >> >> The entire function + parameterList + return type fits in one
> line
> > or
> > > >> >> when it doesn't we wrap parameters into lines.
> > > >> >> I agree that it makes the code a more verbose, but it'll make
> code
> > > >> >> style more consistent.
> > > >> >>
> > > >> >> Shivaram
> > > >> >>
> > > >> >> On Mon, Feb 10, 2014 at 2:13 PM, Aaron Davidson <
> > ilike...@gmail.com>
> > > >> >> wrote:
> > > >> >> > Shivaram, is your recommendation to wrap the parameter list
> even
> > if
> > > >> it
> > > >> >> fits,
> > > >> >> > but just the return type doesn't? Personally, I think the cost
> of
> > > >> moving
> > > >> >> > from a single-line parameter list to an n-ine list is pretty
> > high,
> > > >> as it
> > > >> >> > takes up a lot more space. I am even in favor of allowing a
> > > parameter
> > > >> >> list
> > > >> >> > to overflow into a second line (but not a third) instead of
> > > spreading
> > > >> >> them
> > > >> >> > out, if it's a private helper method (where the parameters are
> > > >> probably
> > > >> >> not
> > > >> >> > as important as the implementation, unlike a public API).
> > > >> >> >
> > > >> >> >
> > > >> >> > On Mon, Feb 10, 2014 at 1:42 PM, Shivaram Venkataraman
> > > >> >> > <shiva...@eecs.berkeley.edu> wrote:
> > > >> >> >>
> > > >> >> >> For the 1st case wouldn't it be better to just wrap the
> > parameters
> > > >> to
> > > >> >> >> the next line as we do in other cases ? For example
> > > >> >> >>
> > > >> >> >> def longMethodName(
> > > >> >> >> param1,
> > > >> >> >> param2, ...) : Long = {
> > > >> >> >> }
> > > >> >> >>
> > > >> >> >> Are there a lot functions which use the old format ? Can we
> just
> > > >> stick
> > > >> >> >> to the above for new functions ?
> > > >> >> >>
> > > >> >> >> Thanks
> > > >> >> >> Shivaram
> > > >> >> >>
> > > >> >> >> On Mon, Feb 10, 2014 at 11:33 AM, Reynold Xin <
> > > r...@databricks.com>
> > > >> >> wrote:
> > > >> >> >> > +1 on both
> > > >> >> >> >
> > > >> >> >> >
> > > >> >> >> > On Mon, Feb 10, 2014 at 1:34 AM, Aaron Davidson <
> > > >> ilike...@gmail.com>
> > > >> >> >> > wrote:
> > > >> >> >> >
> > > >> >> >> >> There are a few bits of the Scala style that are
> > underspecified
> > > >> by
> > > >> >> >> >> both the Scala
> > > >> >> >> >> style guide <http://docs.scala-lang.org/style/> and our
> own
> > > >> >> >> >> supplemental
> > > >> >> >> >> notes<
> > > >> >> >> >>
> > > >> >> >> >>
> > > >> >>
> > > >>
> > >
> https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide
> > > >> >.
> > > >> >> >> >> Often, this leads to inconsistent formatting within the
> > > >> codebase, so
> > > >> >> >> >> I'd
> > > >> >> >> >> like to propose some general guidelines which we can add to
> > the
> > > >> wiki
> > > >> >> >> >> and
> > > >> >> >> >> use in the future:
> > > >> >> >> >>
> > > >> >> >> >> 1) Line-wrapped method return type is indented with two
> > spaces:
> > > >> >> >> >> def longMethodName(... long param list ...)
> > > >> >> >> >> : Long = {
> > > >> >> >> >> 2
> > > >> >> >> >> }
> > > >> >> >> >>
> > > >> >> >> >> *Justification: *I think this is the most commonly used
> style
> > > in
> > > >> >> Spark
> > > >> >> >> >> today. It's also similar to the "extends" style used in
> > > classes,
> > > >> with
> > > >> >> >> >> the
> > > >> >> >> >> same justification: it is visually distinguished from the
> > > >> 4-indented
> > > >> >> >> >> parameter list.
> > > >> >> >> >>
> > > >> >> >> >> 2) URLs and code examples in comments should not be
> > > line-wrapped.
> > > >> >> >> >> Here<
> > > >> >> >> >>
> > > >> >> >> >>
> > > >> >>
> > > >>
> > >
> >
> https://github.com/apache/incubator-spark/pull/557/files#diff-c338f10f3567d4c1d7fec4bf9e2677e1L29
> > > >> >> >> >> >is
> > > >> >> >> >> an example of the latter.
> > > >> >> >> >>
> > > >> >> >> >> *Justification*: Line-wrapping can cause confusion when
> > trying
> > > to
> > > >> >> >> >> copy-paste a URL or command. Can additionally cause IDE
> > issues
> > > >> or,
> > > >> >> >> >> avoidably, Javadoc issues.
> > > >> >> >> >>
> > > >> >> >> >> Any thoughts on these, or additional style issues not
> > > explicitly
> > > >> >> >> >> covered in
> > > >> >> >> >> either the Scala style guide or Spark wiki?
> > > >> >> >> >>
> > > >> >> >
> > > >> >> >
> > > >> >>
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> --
> > > >> Evan Chan
> > > >> Staff Engineer
> > > >> e...@ooyala.com |
> > > >>
> > > >
> > > >
> > >
> >
>
