Skip to content

Cleanup printf #1480

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.

Sign up for GitHub

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

Merged
KevinRansom merged 1 commit into from
Aug 25, 2016
Merged

Cleanup printf #1480

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 17 additions & 20 deletions src/fsharp/FSharp.Core/printf.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,10 +936,7 @@ module internal PrintfImpl =
System.Diagnostics.Debug.Assert((i = n), "i = n")
buf.[i] <- ty
buf
go ty 0

[<Literal>]
let ContinuationOnStack = -1
go ty 0

type private PrintfBuilderStack() =
let args = Stack(10)
Expand All @@ -951,7 +948,7 @@ module internal PrintfImpl =
arr.[start + i] <- s.Pop()
arr

member this.GetArgumentAndTypesAsArrays
member __.GetArgumentAndTypesAsArrays
Copy link
Contributor

@KevinRansom KevinRansom Aug 24, 2016

Choose a reason for hiding this comment

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

@forki @dsyme
I am not a fan of this change, I know you have used it in other cleanup PRs, however, I don't recall a discussion where this was the agreed style.

If we were to agree to a common style for this my preference would be in fact be 'this', since that is what it in fact is. I assume that you want to indicate that the this pointer is unused, but that is not an entirely interesting distinction, given that we are not writing in C# and we are immutable by default. Anyway ... let's have a discussion about a coding standard for this case before we carry on with the clean up.

Kevin

Copy link
Contributor Author

@forki forki Aug 25, 2016

Choose a reason for hiding this comment

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

Obviously I'm pro __ as this is what many projects in the ecosystem use. (also suggested by FSharpLint)

Copy link
Contributor

@cloudRoutine cloudRoutine Aug 25, 2016

Choose a reason for hiding this comment

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

another reason people tend to replace this. with __. is when the identifier isn't used it comes up as an unused declaration

while __. is clean.

If this/self isn't being used it's just visual noise, like superfluous parentheses.

(
argsArraySize, argsArrayStartPos, argsArrayTotalCount,
typesArraySize, typesArrayStartPos, typesArrayTotalCount
Expand All @@ -960,7 +957,7 @@ module internal PrintfImpl =
let typesArray = stackToArray typesArraySize typesArrayStartPos typesArrayTotalCount types
argsArray, typesArray

member this.PopContinuationWithType() =
member __.PopContinuationWithType() =
System.Diagnostics.Debug.Assert(args.Count = 1, "args.Count = 1")
System.Diagnostics.Debug.Assert(types.Count = 1, "types.Count = 1")

Expand All @@ -969,7 +966,7 @@ module internal PrintfImpl =

cont, contTy

member this.PopValueUnsafe() = args.Pop()
member __.PopValueUnsafe() = args.Pop()

member this.PushContinuationWithType (cont : obj, contTy : Type) =
System.Diagnostics.Debug.Assert(this.IsEmpty, "this.IsEmpty")
Expand All @@ -983,17 +980,17 @@ module internal PrintfImpl =

this.PushArgumentWithType(cont, contTy)

member this.PushArgument(value : obj) =
member __.PushArgument(value : obj) =
args.Push value

member this.PushArgumentWithType(value : obj, ty) =
member __.PushArgumentWithType(value : obj, ty) =
args.Push value
types.Push ty

member this.HasContinuationOnStack(expectedNumberOfArguments) =
member __.HasContinuationOnStack(expectedNumberOfArguments) =
types.Count = expectedNumberOfArguments + 1

member this.IsEmpty =
member __.IsEmpty =
System.Diagnostics.Debug.Assert(args.Count = types.Count, "args.Count = types.Count")
args.Count = 0

Expand Down Expand Up @@ -1247,7 +1244,7 @@ module internal PrintfImpl =
else
buildPlain n prefix

member this.Build<'T>(s : string) : PrintfFactory<'S, 'Re, 'Res, 'T> * int =
member __.Build<'T>(s : string) : PrintfFactory<'S, 'Re, 'Res, 'T> * int =
parseFormatString s typeof<'T> :?> _, (2 * count + 1) // second component is used in SprintfEnv as value for internal buffer

/// Type of element that is stored in cache
Expand Down Expand Up @@ -1311,23 +1308,23 @@ module internal PrintfImpl =
let buf : string[] = Array.zeroCreate n
let mutable ptr = 0

override this.Finalize() : 'Result = k (String.Concat(buf))
override this.Write(s : string) =
override __.Finalize() : 'Result = k (String.Concat(buf))
override __.Write(s : string) =
buf.[ptr] <- s
ptr <- ptr + 1
override this.WriteT(s) = this.Write s

type StringBuilderPrintfEnv<'Result>(k, buf) =
inherit PrintfEnv<Text.StringBuilder, unit, 'Result>(buf)
override this.Finalize() : 'Result = k ()
override this.Write(s : string) = ignore(buf.Append(s))
override this.WriteT(()) = ()
override __.Finalize() : 'Result = k ()
override __.Write(s : string) = ignore(buf.Append(s))
override __.WriteT(()) = ()

type TextWriterPrintfEnv<'Result>(k, tw : IO.TextWriter) =
inherit PrintfEnv<IO.TextWriter, unit, 'Result>(tw)
override this.Finalize() : 'Result = k()
override this.Write(s : string) = tw.Write s
override this.WriteT(()) = ()
override __.Finalize() : 'Result = k()
override __.Write(s : string) = tw.Write s
override __.WriteT(()) = ()

let inline doPrintf fmt f =
let formatter, n = Cache<_, _, _, _>.Get fmt
Expand Down