apple / swift-log

A Logging API for Swift
https://apple.github.io/swift-log/
Apache License 2.0
3.55k stars 298 forks source link

Use StaticString for Parameter file and function #136

Closed ShivaHuang closed 4 years ago

ShivaHuang commented 4 years ago

Abstract

The proposal from SSWG: Server Logging API suggest using String for parameters like #file and #function.

func log(level: Logger.Level,
             message: Logger.Message,
             metadata: Logger.Metadata?,
             file: String, function: String, line: UInt)

But is using StaticString a better design in practice?

Current Situation

Log functions take String for #file and #function. But some logging frameworks (backends) take StaticString for these parameters.

Problem

The main problem is String can not convert to StaticString, so for those frameworks take StaticString as parameters, there is no way to build a LogHandler for them.

The only workaround I found is discard these information. For those backends don't care about the #file or #function this will be fine, like swift-log-oslog.

However, the others will always get the #file and #function in LogHandler, not the actual place in the program. This will be a big problem.

Proposed Solution

If we changed the parameter type from String to StaticString, those backends require StaticString can take it without problems. And those require String can easily convert it to what they want. A simple extension can do the trick.

extension StaticString {
    @inlinable
    var stringValue: String {
        self.withUTF8Buffer { String(decoding: $0, as: UTF8.self) }
    }
}

The cons of this change is, it breaks the API compatibility, log handler providers need to update their code for this chage.

Any suggestions about this idea?

ktoso commented 4 years ago

I believe @weissi explored this topic in great depth with specific focus on OSLog so he should be able to answer best here.

weissi commented 4 years ago

I think String is better than StaticString, it should in fact be faster.

@ShivaHuang I would suggest that everybody switch to String instead of StaticString. But do you have a concrete logging backend in mind where you'd need to pass file as a StaticString?

ShivaHuang commented 4 years ago

Great point!

I misunderstood the performance and memory usage between String and StaticString. And I agree that String is overall more suitable than StaticString.

The logging backend use StaticString I encounter is CocoaLumberjack.

However, after digging into their code, they also convert the StaticString to String before passing it to the underlay OC code. here I think I should submit a PR to CocoaLumberjack for that. 😉

BTW, while searching some similar use case in Apple's SDK, I found some functions use StaticString for the same propose. like

Will Apple also change their code? 🤣

weissi commented 4 years ago

@ShivaHuang until Swift 4.2, StaticString was much faster which is why XCTAssertEqual was designed the way it is. Same for assertionFailure etc, now they can’t easily change that because of source/ABI compat.

Also: StaticString should just be 1 word, there’s no reason except that nobody noticed until the ABI was fixed 😬

ShivaHuang commented 4 years ago

I'll close this issue.

Thank you @ktoso and @weissi, I learned a lot in this discussion.