Remove unused function in pipe.c - #14
Conversation
604420c to
58b1e67
Compare
jserv
left a comment
There was a problem hiding this comment.
Always write complete English sentences in Git commit messages.
d096b17 to
1f54ad2
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
| } | ||
|
|
||
| static inline void pipe_put_byte(pipe_t *p, char c) | ||
| static inline __attribute__((__unused__)) void pipe_put_byte(pipe_t *p, char c) |
There was a problem hiding this comment.
It looks like these two functions have no callers.
Why are we adding compiler attributes here instead of just removing them?
| CFLAGS += -Wall -Wextra -Wshadow -Wno-unused-parameter -Werror | ||
| CROSS_COMPILE ?= riscv32-unknown-elf- | ||
| CC_DEFAULT := $(CROSS_COMPILE)gcc | ||
| CC_IS_CLANG := $(shell $(CC_DEFAULT) --version 2>/dev/null | grep -qi clang && echo 1) |
There was a problem hiding this comment.
I find this approach a bit odd. Why append the gcc suffix and then check for clang? It feels a bit roundabout.
visitorckw
left a comment
There was a problem hiding this comment.
So what I'm seeing now is just one commit that marks the functions as unused using a compiler attribute, but this doesn't seem to line up with the PR title 'LLVM support'?
And again, where is this function intended to be used? What is the reason for marking these functions with a compiler attribute instead of removing them entirely?
Without an explanation for this, this is a NAK from me.
|
These functions are currently unused and I don’t intend to use them. If you prefer that I remove them entirely, I can do that instead. |
I think he was just expressing a preference for
I'm not necessarily against keeping these functions. But if we really decide to keep them, I do really think we need to explain why we need to keep them - under what circumstances they are valuable or how they will be used in future plans - rather than mindlessly adding attributes as a workaround. |
visitorckw
left a comment
There was a problem hiding this comment.
Also, the term "certain configurations" is way too confusing. Specifically, what configurations are we talking about? The changelog needs improvement.
|
I agree with the compiler warning here. The CI pipeline can also help prevent similar unused functions from being introduced in future PRs. |
Remove unused function pipe_get_byte and pipe_put_byte
|
Thank @G36maid for contributing! |
This PR fixes build errors for building Linmo with the LLVM toolchain.
related to #2 , #12
Changes
kernel/pipe.c