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

Comments are somewhat lacking #19

Open
NSExceptional opened this issue Apr 8, 2021 · 0 comments
Open

Comments are somewhat lacking #19

NSExceptional opened this issue Apr 8, 2021 · 0 comments

Comments

@NSExceptional
Copy link

NSExceptional commented Apr 8, 2021

This project is amazing! However, the lack of comments makes it difficult to learn from. 😓

For example, this entire block of code has only two comments. I'm grateful for that first one or I would be completely lost, but stuff like this would benefit greatly from having more detailed comments, like this:

// Store the offset we just computed
bounds.withMemoryRebound(to: Int.AtomicRepresentation.self, capacity: 1) {
    Int.AtomicRepresentation.atomicStore(
        immediateMemberOffset,
        at: $0,
        ordering: .releasing
    )
}

// [brief rationale on why we need to divide the offset by sizeof(Int)]
return immediateMemberOffset / MemoryLayout<Int>.size

I would add some myself, but I'm not confident I know what's going on at all in most of these blocks of code, like that last line above. To be clear, I'm not saying you need pedantic comments everywhere! This is fine for example, as the code itself is mostly self explanatory and the property itself is documented:

/// A pointer to the resilient superclass. This pointer differs in type
/// depending on what typeFlags.resilientSuperclassRefKind returns.
public var resilientSuperclass: UnsafeRawPointer? {
    guard typeFlags.classHasResilientSuperclass else {
        return nil
    }

    var offset = 0

    if flags.isGeneric {
        offset += typeGenericContext.size
    }

    return (trailing + offset).relativeDirectAddress(as: Void.self)
}

Heck, self explanatory types and properties don't need to be documented either imo. I'm really just concerned with the long blocks of code for complex operations, with little documentation on the process. These topics in Swift are not well documented at all really, as you're probably aware. It takes a lot of time and effort to do the research and testing necessary to build something like this. It would be a shame if, in the future, when the ABI-unstable parts of the runtime change and this library breaks, someone else had to duplicate your efforts because they weren't documented well enough for someone to fix it instead of just starting over from scratch :/

You can close this issue right away if you wish; I just wanted to bring this to your attention in hopes that you might be more conscious about using more comments going forward 😄 Thanks again for making this library; I can't wait to show you what I'm going to do with it!

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

No branches or pull requests

1 participant