Skip to content

Conversation

@alkuzin
Copy link
Contributor

@alkuzin alkuzin commented Jan 15, 2025

No description provided.

@NotYourFox
Copy link
Contributor

NotYourFox commented Jan 15, 2025

Okay, so, first of all: don't forget to sync your forks before making changes to avoid conflicts and wasted work, as with your case kstdint was entirely removed in @0Nera's MR #24
And second: I did already add .vscode to .gitignore in MR #23 for the exact same reason. So, again, sync your forks, you are not the only contributor!

Regarding your commit to kstdlib.h, I think it is better to use macros for things like bytes_to_bits (and vice versa) and clear/set/test bit functions, as there is little to no reason to keep them as inline functions, also acknowledging the fact that "inline" keyword is rather a hint, than an obligation to the compiler. Hence, this is not really the best practice.

@NotYourFox
Copy link
Contributor

NotYourFox commented Jan 15, 2025

P.S. I would like to request @0Nera's attention to the latter detail before his approval of this MR, to see if the current state of is "fine enough" and if my recommendations are necessary to follow or not in this case. Still, I encourage @alkuzin to address this and judge the necessity of applying the above himself.
I would be pleased to recieve feedback on this before the merge happens.

@0Nera 0Nera added the invalid This doesn't seem right label Jan 15, 2025
@0Nera 0Nera closed this Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants