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 |
> > >>
> > >
> > >
> >
>
