Refactor: simplify printXXX functions usages by apocelipes · Pull Request #625 · boyter/scc

Simplify printXXX functions usages. This PR makes code more simple and no harm to performance.

Why

Current usages of printXXX functions would check the Verbose/Trace/... flags twice (or 3 times for printXXXf functions). This is inefficient. printXXXf also calls printXXX, and all these functions cannot be inlined, that means the performance will be slowed down when we display some trace log (CI prints the trace log).

For code readability, the origin code has too much redundant code. There is also inappropriate coupling between functions.

For these reasons I did a refactoring of the whole printXXX function family.

Changes

In the first commit, I did:

  • Simplify the printXXX functions' code by introduce doPrint and prepareMsg. prepareMsg will reuse its first paramater if we do not need call sprintf. Now these functions are no longer coupled.
  • This makes all printXXX functions can be inlined. So there is no performance impact even if we delete the outside if statements.
  • Move printXXX functions to trace.go.
  • Add tests for all these things.

image

In the second one:

  • Since printXXX now can be inlined, the outside if Verbose/Debug condition is redundant. We already checked the flags in printXXX. The runtime logic doesn't change when these if statements are removed either:
// Origin code looks like:
if Trace {
        // printTraceF("trace: %s", msg) inline into:
        if Trace {
                doPrint(os.Stdout, levelTrace, "trace: %s", msg)
        }
}

// After removing:
// printTraceF("trace: %s", msg) inline into:
if Trace {
        doPrint(os.Stdout, levelTrace, "trace: %s", msg)
}
  • Use printXXXF("template %s", arg1, arg2...) instead of printXXX(fmt.Sprintf("template %s", arg1, arg2...)), the former is more simple and faster.
  • printXXX functions usages changed: Dont put them into if Verbose/Debug/Trace {...} blocks, call it directly:
// Before
func A() {
        do some work
        if Verbose {
                printWarn("a warning message")
        }
}

// After
func A() {
        do some work
        printWarn("a warning message") // won't print if the "Verbose" flag is false and no performance regressions
}

I splitted the changes into 2 commits so that reviewers may easily do reviewing. Feel free to squash the commits or not if you are merging this PR.