compile: avoid min macro pollution #942
Conversation
src/nvme/util.h
Outdated
| @@ -560,7 +560,7 @@ char *kv_keymatch(const char *kv, const char *key); | |||
| */ | |||
| char *startswith(const char *s, const char *prefix); | |||
|
|
|||
| #define min(x, y) ((x) > (y) ? (y) : (x)) | |||
| #define nvme_min(x, y) ((x) > (y) ? (y) : (x)) | |||
There was a problem hiding this comment.
I think we should not expose this macro at all. And there should be already the min macro from the ccan 'library'. Can you drop this one and include the ccan/minmax.h header in the c files?
There was a problem hiding this comment.
yup, we can, but we need cast the type, see updated code.
test/cpp.cc
Outdated
| static int max_compile_test() | ||
| { | ||
| return std::max(1, 2); | ||
| } |
There was a problem hiding this comment.
This is unrelated with the above change.
There was a problem hiding this comment.
yup, I think min and max should always appear in pairs, so I just added it, remove now.
zhangjian3032
force-pushed
the
dev-avoid-macro-pollution
branch
from
79a7219 to
a485847
Compare
src/nvme/ioctl.c
Outdated
| @@ -503,7 +504,7 @@ static int read_ana_chunk(int fd, enum nvme_log_ana_lsp lsp, bool rae, | |||
| } | |||
|
|
|||
| while (*read < to_read) { | |||
| __u32 len = min(log_end - *read, NVME_LOG_PAGE_PDU_SIZE); | |||
| __u32 len = min((int)(log_end - *read), NVME_LOG_PAGE_PDU_SIZE); | |||
There was a problem hiding this comment.
I think it would be better to use min_t here:
`min_t(int, log_end - *read, NVME_LOG_PAGE_PDU_SIZE);`
There was a problem hiding this comment.
thanks, make sense.
There was a problem hiding this comment.
Sorry, thinko on my side. The final type is __u32, so we should also cast to __u32 and not int.
There was a problem hiding this comment.
the result of subtracting two u8* pointers is a signed integer, use int also makes sense
| static int min_compile_test() | ||
| { | ||
| return std::min(1, 2); | ||
| } |
There was a problem hiding this comment.
My point is that the whole change to test/cpp.c should be in a different patch. I don't mind if you add the min and max to catch build errors, just in a separate patch.
zhangjian3032
force-pushed
the
dev-avoid-macro-pollution
branch
2 times, most recently
from
9d3a939 to
3d9d863
Compare
The generic min macro in util.h is too common and can conflict with other libraries. use ccan/minmax in the source files to avoid pollution. Before renaming the macro, the cpp's std::min will fail to compile. ``` ./test/cpp.cc: In function ‘int min_compile_test()’: ../src/nvme/util.h:563:19: error: expected unqualified-id before ‘(’ token 563 | #define min(x, y) ((x) > (y) ? (y) : (x)) ``` Signed-off-by: Jian Zhang <[email protected]>
zhangjian3032
force-pushed
the
dev-avoid-macro-pollution
branch
from
3d9d863 to
5a9d3a4
Compare
The generic min macro in util.h is too common and can conflict with other libraries. Rename it to nvme_min to avoid pollution.
Before renaming the macro, the cpp's std::min will fail to compile.