Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reformat using scalafmt #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Reformat using scalafmt #41

wants to merge 1 commit into from

Conversation

Kraks
Copy link
Collaborator

@Kraks Kraks commented Jul 31, 2020

This PR proposes to adopt a unified coding style and to use scalafmt to format the code. When running in sbt, you can use lms-clean/scalafmt to automatically reformat all source code files.

However, this PR is by no means enforcing a style we eventually should use, but just initiating such effort. There are tons of config options (https://scalameta.org/scalafmt/docs/configuration.html) we can tweak to find a style that everyone is comfortable with.

@Kraks Kraks requested review from feiwang3311 and TiarkRompf July 31, 2020 05:18
}
implicit class ArrayOps[A: Manifest](x: Rep[Array[A]]) {
def apply(i: Rep[Int]): Rep[A] =
x match {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer the old format

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean x match { should be on the same line of def ...?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I tried to fix it myself but didn't got too much time to read the documentation for formatting

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - the x match { should be on the same line

Copy link
Collaborator

@feiwang3311 feiwang3311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (or rather, Idk how to fight the formatter, so I surrender)

Whatever formatting is fine with me :)

Copy link
Owner

@TiarkRompf TiarkRompf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uniformity is good, and it's good to try automatic tools that enforce it.

With the results right now there is too much "de-densification" for my taste, e.g. code broken across many lines where it doesn't need to, which I think makes it harder to read, not easier. But I'm pretty sure that can be fixed.

Another question is if we really want to depend on another external tool (need to keep track of versions, etc). Are the benefits worth it?

def copyToLongArray(arr: Rep[LongArray[A]], start: Rep[Long], len: Rep[Int]) =
Adapter.g.reflectEffect("array_copyTo", Unwrap(x), Unwrap(arr), Unwrap(start), Unwrap(len))(Unwrap(x))(
Unwrap(arr)
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These formatting changes also don't seem like improvements to me. It may be worth looking at refactoring the code for clarity, but just putting the last Unwrap(...) on another line make it more difficult to read IMO.

Adapter.g.Def("list-new", mA :: (xs: List[Backend.Exp])),
Adapter.g.Def("list-new", _ :: (ys: List[Backend.Exp]))
) =>
val unwrapped_xsys = Seq(mA) ++ xs ++ ys
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer previous here, too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants