0.8.1: LGTM flagged an always-true comparison

[kcgen]

[Cyan4973]

assert() is for debug, while returning error is for runtime.

[kcgen]

I think this issue that the LGTM knows that secretSize never comes from user-side data and instead is deterministic, and known at compile-time.

So it's basically a developer/code-consistency check. Any voilation of this will be caught in the CI unit tests, when run with assertions.

It's a bit like having: if (sizeof(int) != 4) return false;, which is certainly valid, but better left to assertions.

[Cyan4973]

secretSize never comes from user-side data and instead is deterministic, and known at compile-time.

Well, known at compile time is not clear to me.
XXH3_generateSecret() is a user-facing entry point. The code can't control what kind of value users provide as secretSize.
Using any value < XXH3_SECRET_SIZE_MIN is invalid, and clearly documented as such,
but that doesn't mean it will never happen.
I actually fully expect that some users will not read the doc and just put there any value that comes to their mind.
And there is also no guarantee that such value will be provided as a fixed constant at compile time.

So it's basically a developer/code-consistency check. Any voilation of this will be caught in the CI unit tests, when run with assertions.

Yes it is.
In essence, XXH3_generateSecret() offers a narrow contract.
Unfortunately, that doesn't mean that the contract can be checked statically at compile time.
The intention is to catch such issue as part of the user's application CI test suite, aka at runtime, thanks to assert().

better left to assertions.

In theory, yes, no user should break the contract, and code should be tested with assert() enabled first.
Now, in practice, what happens if someone nonetheless provides a small secretSize value, and on top of that, never test in debug mode and run the code directly in production mode, with no assert enabled ?
At least, with a return error;, there is a clearly defined behavior to this situation.

I appreciate that the answer to above scenario is not an obvious trivial clear/cut decision, there are arguments to both sides. In this case though, what pushed me towards providing a return error value is the fact that the same design is already in place for other established public symbols of libxxhash. So it feels like merely applying the local coding convention of the repository.

[kcgen]

I agree @Cyan4973 , thanks for the explanation.

I was wondering why Lgtm didn't also flag the assert criteria as well, and it now makes sense.

In the debug case, the only logical way execution ever makes it to if statement is when secretSize >= 126, so that's how Lgtm deduced it at compile time.

(If we gave Lgtm a release build, then it would not come to this conclusion, but it would flag many other errors that asserts are meant to catch).

Is it possible to hide the if for non-release builds?(maybe preprocessor check for NDEBUG)

This is the only instance flagged, so is quite a unique check in the codebase.

[Cyan4973]

Checking debug mode for the if is likely going to work.

[kcgen]

Checking debug mode for the if is likely going to work.

Confirmed! This is no longer flagged: