ronantakizawa/codereview-bench huggingface dataset:
Repository: valkey-io/valkey
File: src/tls.c (C)

Code before:
        valkey_tls_client_ctx = NULL;
    }

#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
    // unavailable on LibreSSL
    OPENSSL_cleanup();
#endif
}

/* Callback for passing a keyfile password stored as an sds to OpenSSL */
static int tlsPasswordCallback(char *buf, int size, int rwflag, void *u) {
    UNUSED(rwflag);

    const char *pass = u;
    size_t pass_len;

    if (!pass) return -1;
    pass_len = strlen(pass);
    if (pass_len > (size_t)size) return -1;
    memcpy(buf, pass, pass_len);

    return (int)pass_len;
}

/* Check a single X509 certificate validity */
static int is_cert_valid(X509 *cert) {
    if (!cert) return 0;
    const ASN1_TIME *not_before = X509_get0_notBefore(cert);
    const ASN1_TIME *not_after = X509_get0_notAfter(cert);
    if (!not_before || !not_after) return 0;
    if (X509_cmp_current_time(not_before) > 0 ||
        X509_cmp_current_time(not_after) < 0) {
        return 0;
    }
    return 1;
}

/* Load all certificates from a directory into the X509_STORE */
static int load_ca_cert_dir(SSL_CTX *ctx, const char *ca_cert_dir) {
    if (!ca_cert_dir) return 1;

    DIR *dir;
    struct dirent *entry;
    char full_path[PATH_MAX];
    X509_STORE *store = SSL_CTX_get_cert_store(ctx);

    if (!store) {
        serverLog(LL_WARNING, "Failed to get X509_STORE from SSL_CTX");
        return 0;
    }


Review comment (suggestion):
We normally use `camelCase` for function names and `snake_case` for variables.

Consider using bool and true/false for the return value.

```suggestion
static bool isCertValid(X509 *cert) {
```

Code after:
    if (valkey_tls_client_ctx) {
        SSL_CTX_free(valkey_tls_client_ctx);
        valkey_tls_client_ctx = NULL;
    }

#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
    // unavailable on LibreSSL
    OPENSSL_cleanup();
#endif
}

/* Callback for passing a keyfile password stored as an sds to OpenSSL */
static int tlsPasswordCallback(char *buf, int size, int rwflag, void *u) {
    UNUSED(rwflag);

    const char *pass = u;
    size_t pass_len;

    if (!pass) return -1;
    pass_len = strlen(pass);
    if (pass_len > (size_t)size) return -1;
    memcpy(buf, pass, pass_len);

    return (int)pass_len;
}

/* Check a single X509 certificate validity */
static bool isCertValid(X509 *cert) {
    if (!cert) return false;
    const ASN1_TIME *not_before = X509_get0_notBefore(cert);
    const ASN1_TIME *not_after = X509_get0_notAfter(cert);
    if (!not_before || !not_after) return false;
    if (X509_cmp_current_time(not_before) > 0 ||
        X509_cmp_current_time(not_after) < 0) {
        return false;
    }
    return true;
}

/* Load all certificates from a directory into the X509_STORE
 * Returns true on success, false on failure */
static bool loadCaCertDir(SSL_CTX *ctx, const char *ca_cert_dir) {
    if (!ca_cert_dir) return true;

    DIR *dir;
    struct dirent *entry;
    char full_path[PATH_MAX];
    X509_STORE *store = SSL_CTX_get_cert_store(ctx);

    if (!store) {
        serverLog(LL_WARNING, "Failed to get X509_STORE from SSL_CTX");
Repository: valkey-io/valkey
File: src/unit/test_mutexqueue.c (C)

Code before:
    return 0;
}

/* Test: priorityOrdering */
int test_mutexQueuePriorityOrdering(int argc, char *argv[], int flags) {
    UNUSED(argc);
    UNUSED(argv);
    UNUSED(flags);

    mutexQueue *q = mutexQueueCreate();
    add(q, 10);
    priorityAdd(q, 1);
    add(q, 11);
    priorityAdd(q, 2);
    popTest(q, 1);
    popTest(q, 2);
    popTest(q, 10);
    popTest(q, 11);
    TEST_EXPECT(mutexQueuePop(q, false) == NULL);

    mutexQueueRelease(q);
    return 0;
}

/* Test: peekAndRemoveHead */
int test_mutexQueuePeekAndRemoveHead(int argc, char *argv[], int flags) {
    UNUSED(argc);
    UNUSED(argv);
    UNUSED(flags);

    mutexQueue *q = mutexQueueCreate();
    add(q, 10);
    priorityAdd(q, 1);
    add(q, 11);

    TEST_EXPECT((long)mutexQueuePeek(q, false) == 1);
    TEST_EXPECT(mutexQueueLength(q) == 3ul);

    void *removed = mutexQueuePop(q, false);
    TEST_EXPECT((long)removed == 1);
    TEST_EXPECT(mutexQueueLength(q) == 2ul);
    TEST_EXPECT((long)mutexQueuePeek(q, false) == 10);

    removed = mutexQueuePop(q, false);
    TEST_EXPECT((long)removed == 10);
    removed = mutexQueuePop(q, false);
    TEST_EXPECT((long)removed == 11);

    TEST_EXPECT(mutexQueuePeek(q, false) == NULL);
    TEST_EXPECT(mutexQueuePop(q, false) == NULL);
    TEST_EXPECT(mutexQueueLength(q) == 0ul);

Review comment (suggestion):
```suggestion
/* Test: peek */
int test_mutexQueuePeek(int argc, char *argv[], int flags) {
```

Code after:
    return 0;
}

/* Test: priorityOrdering */
int test_mutexQueuePriorityOrdering(int argc, char *argv[], int flags) {
    UNUSED(argc);
    UNUSED(argv);
    UNUSED(flags);

    mutexQueue *q = mutexQueueCreate();
    add(q, 10);
    priorityAdd(q, 1);
    add(q, 11);
    priorityAdd(q, 2);
    popTest(q, 1);
    popTest(q, 2);
    popTest(q, 10);
    popTest(q, 11);
    TEST_EXPECT(mutexQueuePop(q, false) == NULL);

    mutexQueueRelease(q);
    return 0;
}

/* Test: peek */
int test_mutexQueuePeek(int argc, char *argv[], int flags) {
    UNUSED(argc);
    UNUSED(argv);
    UNUSED(flags);

    mutexQueue *q = mutexQueueCreate();
    add(q, 10);
    priorityAdd(q, 1);
    add(q, 11);

    TEST_EXPECT((long)mutexQueuePeek(q, false) == 1);
    TEST_EXPECT(mutexQueueLength(q) == 3ul);

    void *removed = mutexQueuePop(q, false);
    TEST_EXPECT((long)removed == 1);
    TEST_EXPECT(mutexQueueLength(q) == 2ul);
    TEST_EXPECT((long)mutexQueuePeek(q, false) == 10);

    removed = mutexQueuePop(q, false);
    TEST_EXPECT((long)removed == 10);
    removed = mutexQueuePop(q, false);
    TEST_EXPECT((long)removed == 11);

    TEST_EXPECT(mutexQueuePeek(q, false) == NULL);
    TEST_EXPECT(mutexQueuePop(q, false) == NULL);
    TEST_EXPECT(mutexQueueLength(q) == 0ul);
Repository: vapor/vapor
File: Sources/Vapor/Session/Session.swift (Swift)

Code before:

public class Session {

    public static var driver: SessionDriver = MemorySessionDriver()

    var sessionIdentifier: String?

	init() {
		//do nothing
	}

	public func destroy() {
        guard let sessionIdentifier = sessionIdentifier else {
            Log.warning("Unable to destroy the session: The session has not be registered yet")
            return
        }

        Session.driver.destroySessionIdentifiedBy(sessionIdentifier)
	}

    public subscript(key: String) -> String? {
        get {
            guard let sessionIdentifier = sessionIdentifier else {
                Log.warning("Unable to read a value for '\(key)': The session has not be registered yet")
                return nil
            }

            return Session.driver.valueForKey(key, inSessionIdentifiedBy: sessionIdentifier)
        }

        set(newValue) {
            guard let sessionIdentifier = sessionIdentifier else {
                Log.warning("Unable to store a value for '\(key)': The session has not be registered yet")
                return
            }

            Session.driver.setValue(newValue, forKey: key, inSessionIdentifiedBy: sessionIdentifier)
        }
    }
}


Review comment (refactor):
No, we shouldn't need to specify it. I'll update it. I'd rather clean up files as they are touched. :)


Code after:

public class Session {

    public static var driver: SessionDriver = MemorySessionDriver()

    public internal(set) var identifier: String?

	init() {
		//do nothing
	}

	public func destroy() {
        Session.driver.destroy(self)
	}

    public subscript(key: String) -> String? {
        get {
            return Session.driver.valueFor(key: key, inSession: self)
        }

        set {
            Session.driver.set(newValue, forKey: key, inSession: self)
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Session/Session.swift (Swift)

Code before:

public class Session {

    public internal(set) var identifier: String?
    var driver: SessionDriver?

	init() {
		//do nothing
	}

	public func destroy() {
        driver?.destroy(self)
	}

    public subscript(key: String) -> String? {
        get {
            return driver?.valueFor(key: key, inSession: self)
        }

        set {
            driver?.set(newValue, forKey: key, inSession: self)
        }
    }
}


Review comment (suggestion):
This seems like it shouldn't be `Optional`, the `sessionDriver` on `Application` is non-optional, we should be able to function the same way here.  These should be setup as:

```
public let identifier: String
let driver: SessionDriver

init(driver: SessionDriver, identifier: String) {
    self.driver = driver
    self.identifier = identifier
}
```

Need to investigate the implications of this a little bit first.


Code after:

public class Session {

    public let identifier: String
    var driver: SessionDriver

    init(identifier: String, driver: SessionDriver) {
        self.identifier = identifier
        self.driver = driver
	}

	public func destroy() {
        driver.destroy(self)
	}

    public subscript(key: String) -> String? {
        get {
            return driver.valueFor(key: key, inSession: self)
        }

        set {
            driver.set(newValue, forKey: key, inSession: self)
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Middleware/Middleware.swift (Swift)

Code before:
import Foundation

/**
    Intercept and modify `Request`s and `Response`s
    using middleware. Create a class that conforms to 
    the `Middleware` protocol, then append the class
    to the `Server`s `middleware` array.
*/
public protocol Middleware {
    
    /**
        Here is where you implement your custom `Middleware`
        logic. Look at the `SessionMiddleware` to see an 
        example of `Middleware` being used.
     
        Call `handler(request)` somewhere inside your custom
        handler to get the `Response` object.
    */
    static func handle(forApplication application: Application, handler: Request.Handler) -> Request.Handler
    
}

Review comment (suggestion):
Discussed in slack a bit, I think would read better as:

``` Swift
static func handle(requestHandler: Request.Handler, for application: Application)
```

or

``` Swift
static func handle(requestHandler: Request.Handler, with application: Application)
```


Code after:
import Foundation

/**
    Intercept and modify `Request`s and `Response`s
    using middleware. Create a class that conforms to 
    the `Middleware` protocol, then append the class
    to the `Server`s `middleware` array.
*/
public protocol Middleware {
    
    /**
        Here is where you implement your custom `Middleware`
        logic. Look at the `SessionMiddleware` to see an 
        example of `Middleware` being used.
     
        Call `handler(request)` somewhere inside your custom
        handler to get the `Response` object.
    */
    static func handle(handler: Request.Handler, for application: Application) -> Request.Handler
    
}
Repository: vapor/vapor
File: Sources/Vapor/Session/Session.swift (Swift)

Code before:

public class Session {

    public internal(set) var identifier: String?
    var driver: SessionDriver?

	init() {
		//do nothing
	}

	public func destroy() {
        driver?.destroy(self)
	}

    public subscript(key: String) -> String? {
        get {
            return driver?.valueFor(key: key, inSession: self)
        }

        set {
            driver?.set(newValue, forKey: key, inSession: self)
        }
    }
}


Review comment (suggestion):
Revisiting this, I see the issue. We initialize the session long before.

How would we feel about making `session` optional on `Request`? Right now, its completely non-functional w/o a session driver, so it makes sense that it wouldn't be there without the session middleware adding it in.

@tannernelson thoughts on this?

If this is the case, on request, where we set now, we'd do:

``` Swift
let identifier = app.driver.makeSessionIdentifier()
let session = Session(driver: app.driver, identifier: identifier)
request.session = session
```

I like this since `session` is pointless w/o a driver and it kind of makes sense to have a session property w/o functionality.

The counter is that the session is set and modified by middleware, but the driver is owned by the application. Perhaps the `SessionDriver` should take on a bit more of this functionality or the middleware should own the session driver as opposed to the application.


Code after:

public class Session {

    public let identifier: String
    var driver: SessionDriver

    init(identifier: String, driver: SessionDriver) {
        self.identifier = identifier
        self.driver = driver
	}

	public func destroy() {
        driver.destroy(self)
	}

    public subscript(key: String) -> String? {
        get {
            return driver.valueFor(key: key, inSession: self)
        }

        set {
            driver.set(newValue, forKey: key, inSession: self)
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Session/Session.swift (Swift)

Code before:

public class Session {

    public internal(set) var identifier: String?
    var driver: SessionDriver?

	init() {
		//do nothing
	}

	public func destroy() {
        driver?.destroy(self)
	}

    public subscript(key: String) -> String? {
        get {
            return driver?.valueFor(key: key, inSession: self)
        }

        set {
            driver?.set(newValue, forKey: key, inSession: self)
        }
    }
}


Review comment (suggestion):
Session as an optional on request makes sense. If you delete the session middleware, then the request should not have a Session on it. 

I like your code snippet above.


Code after:

public class Session {

    public let identifier: String
    var driver: SessionDriver

    init(identifier: String, driver: SessionDriver) {
        self.identifier = identifier
        self.driver = driver
	}

	public func destroy() {
        driver.destroy(self)
	}

    public subscript(key: String) -> String? {
        get {
            return driver.valueFor(key: key, inSession: self)
        }

        set {
            driver.set(newValue, forKey: key, inSession: self)
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Config/Config.swift (Swift)

Code before:
import Foundation

public class Config {
	public static let configDir = Application.workDir + "Config"
	private let fileManager = NSFileManager.defaultManager()
	private var repository: [String: Json]

	public init(repository: [String: Json] = Dictionary(), application: Application? = nil) {
		self.repository = repository

		if let application = application {
			self.populate(application)
		}
	}

	public func has(keyPath: String) -> Bool {
		return self.get(keyPath) != nil
	}

	public func get(keyPath: String) -> Json? {
		var keys = keyPath.keys

		guard keys.count > 0 else {
			return nil
		}

		var value = self.repository[keys.removeFirst()]

		while value != nil && value != Json.NullValue && keys.count > 0 {

Review comment (suggestion):
Is the singleton workDir good on Application? I think it's fine here, but I remember at one point someone posed the idea of `Using multiple applications on one server`, so might be good to keep this stuff in mind.


Code after:
import Foundation

public class Config {
	public static let configDir = Application.workDir + "Config"
	private let fileManager = NSFileManager.defaultManager()
	private var repository: [String: Json]

	public init(repository: [String: Json] = [:], application: Application? = nil) {
		self.repository = repository

		if let application = application {
			populate(application)
		}
	}

	public func has(keyPath: String) -> Bool {
		return get(keyPath) != nil
	}

	public func get(keyPath: String) -> Json? {
		var keys = keyPath.keys

		guard keys.count > 0 else {
			return nil
		}

		var value = repository[keys.removeFirst()]

		while value != nil && value != Json.NullValue && keys.count > 0 {
Repository: vapor/vapor
File: Sources/Vapor/JSON/JSON+Merging.swift (Swift)

Code before:
extension Json {

	/** Recursively merges two Json objects */
	mutating func merge(with: Json) {
		switch with {
			case .ObjectValue(let object):
				guard case let .ObjectValue(object2) = self else {
					self = with
					return
				}

				var merged = object2

				for (key, value) in object {
					if let original = merged[key] {
						var newValue = original
						newValue.merge(value)
						merged[key] = newValue
					} else {
						merged[key] = value
					}
				}

				self = .ObjectValue(merged)
			case .ArrayValue(let array):
				guard case let .ArrayValue(array2) = self else {
					self = with
					return
				}

				self = .ArrayValue(array + array2)
			default:
				self = with
		}

	}

}


Review comment (suggestion):
might read better with `merge(with json: Json)` so internally we can use `switch json`


Code after:
extension Json {

	/** Recursively merges two Json objects */
	mutating func merge(with json: Json) {
		switch json {
			case .ObjectValue(let object):
				guard case let .ObjectValue(object2) = self else {
					self = json
					return
				}

				var merged = object2

				for (key, value) in object {
					if let original = merged[key] {
						var newValue = original
						newValue.merge(with: value)
						merged[key] = newValue
					} else {
						merged[key] = value
					}
				}

				self = .ObjectValue(merged)
			case .ArrayValue(let array):
				guard case let .ArrayValue(array2) = self else {
					self = json
					return
				}

				self = .ArrayValue(array + array2)
			default:
				self = json
		}

	}

}
Repository: vapor/vapor
File: Sources/Vapor/Config/Config.swift (Swift)

Code before:
import Foundation

public class Config {
	public static let configDir = Application.workDir + "Config"
	private let fileManager = NSFileManager.defaultManager()
	private var repository: [String: Json]

	public init(repository: [String: Json] = Dictionary(), application: Application? = nil) {
		self.repository = repository

		if let application = application {
			self.populate(application)
		}
	}

	public func has(keyPath: String) -> Bool {
		return self.get(keyPath) != nil
	}

	public func get(keyPath: String) -> Json? {
		var keys = keyPath.keys

		guard keys.count > 0 else {
			return nil
		}

		var value = self.repository[keys.removeFirst()]

		while value != nil && value != Json.NullValue && keys.count > 0 {

Review comment (suggestion):
Yeah, I would like to get us to the point of running two apps on one server (i.e. one pure http server, one websocket server). Since we inject an application into this class anyways, we could pass all this into the initializer.


Code after:
import Foundation

public class Config {
	public static let configDir = Application.workDir + "Config"
	private let fileManager = NSFileManager.defaultManager()
	private var repository: [String: Json]

	public init(repository: [String: Json] = [:], application: Application? = nil) {
		self.repository = repository

		if let application = application {
			populate(application)
		}
	}

	public func has(keyPath: String) -> Bool {
		return get(keyPath) != nil
	}

	public func get(keyPath: String) -> Json? {
		var keys = keyPath.keys

		guard keys.count > 0 else {
			return nil
		}

		var value = repository[keys.removeFirst()]

		while value != nil && value != Json.NullValue && keys.count > 0 {
Repository: vapor/vapor
File: Sources/Vapor/Config/Config.swift (Swift)

Code before:
import Foundation

public class Config {
	public static let configDir = Application.workDir + "Config"
	private let fileManager = NSFileManager.defaultManager()
	private var repository: [String: Json]

	public init(repository: [String: Json] = Dictionary(), application: Application? = nil) {
		self.repository = repository

		if let application = application {
			self.populate(application)
		}
	}

	public func has(keyPath: String) -> Bool {
		return self.get(keyPath) != nil
	}

	public func get(keyPath: String) -> Json? {
		var keys = keyPath.keys

		guard keys.count > 0 else {
			return nil
		}

		var value = self.repository[keys.removeFirst()]

		while value != nil && value != Json.NullValue && keys.count > 0 {

Review comment (refactor):
@LoganWright @tannernelson @ketzusaka I'm copying the pattern used in View -- though I think we should refactor this throughout and `Application.workDir` shouldn't be static.

@ketzusaka It's a tab, github just renders it larger.


Code after:
import Foundation

public class Config {
	public static let configDir = Application.workDir + "Config"
	private let fileManager = NSFileManager.defaultManager()
	private var repository: [String: Json]

	public init(repository: [String: Json] = [:], application: Application? = nil) {
		self.repository = repository

		if let application = application {
			populate(application)
		}
	}

	public func has(keyPath: String) -> Bool {
		return get(keyPath) != nil
	}

	public func get(keyPath: String) -> Json? {
		var keys = keyPath.keys

		guard keys.count > 0 else {
			return nil
		}

		var value = repository[keys.removeFirst()]

		while value != nil && value != Json.NullValue && keys.count > 0 {
Repository: vapor/vapor
File: Sources/Vapor/Localization/Localization.swift (Swift)

Code before:
import PathIndexable
import Foundation

public class Localization {
    private let localizationDirectoryPath: String
    private let localization: JsonDirectory

    public init(workingDirectory: String = "./") {
        let configDirectory = workingDirectory.finish("/") + "Localization/"
        self.localizationDirectoryPath = configDirectory
        self.localization = FileManager.loadDirectory(configDirectory)
            ?? JsonDirectory(name: "empty", files: [])
    }

    public subscript(_ languageCode: String, _ paths: PathIndex...) -> String {
        return self[languageCode, paths]
    }

    public subscript(_ languageCode: String, _ paths: [PathIndex]) -> String {
        return localization[languageCode, paths]?.string
            ?? localization["default", paths]?.string
            ?? "not found" // TODO: Discuss message or ""
    }
}


Review comment (suggestion):
We could have something like `Localization.default = ""`


Code after:
import PathIndexable
import Foundation

public class Localization {
    private let localizationDirectoryPath: String
    private let localization: JSONDirectory

    public init(workingDirectory: String = "./") {
        let configDirectory = workingDirectory.finish("/") + "Localization/"
        self.localizationDirectoryPath = configDirectory
        self.localization = FileManager.loadDirectory(configDirectory)
            ?? JSONDirectory(name: "empty", files: [])
    }

    public subscript(_ languageCode: String, _ paths: PathIndex...) -> String {
        return self[languageCode, paths]
    }

    public subscript(_ languageCode: String, _ paths: [PathIndex]) -> String {
        return localization[languageCode, paths]?.string
            ?? localization["default", paths]?.string
            ?? paths.map { "\($0)" }.joined(separator: ".")
    }
}