Jump to conversation
Unresolved conversations (0)
Nice work!

Nice work!

All of your conversations have been resolved.

Resolved conversations (15)
@nickdesaulniers nickdesaulniers Nov 7, 2024
```suggestion for (; next < end; next += size) if (compar(key, next) == 0) return const_cast<cpp::byte *>(next); ```
Outdated
libc/src/search/lfind.cpp
@nickdesaulniers nickdesaulniers Nov 4, 2024
Can we just not declare `next` as `const`? If anything, `end` should be `const` and `next` should not?
Outdated
libc/src/search/lfind.cpp
nickdesaulniers duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
```suggestion for (; next < end; next += size) if (compar(key, next) == 0) // According to IEEE Std 1003.1-2024 we are expected to accept a const // reference to base, but return a non-const reference to the element it // contains. return const_cast<cpp::byte *>(next); ```
Outdated
libc/src/search/lfind.cpp
@nickdesaulniers nickdesaulniers Nov 4, 2024
```suggestion constexpr size_t len = 3; int list[len] = {1, 2, 3}; ```
libc/test/src/search/lfind_test.cpp
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
```suggestion int list[3] = {1, 2, 3}; size_t len = 3; ```
libc/test/src/search/lfind_test.cpp
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
```suggestion constexpr size_t len = 3; int list[len] = {1, 2, 3}; ```
libc/test/src/search/lfind_test.cpp
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
```suggestion constexpr size_t len = 3; int list[len] = {1, 2, 3}; ```
libc/test/src/search/lfind_test.cpp
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
This could also be: ```suggestion return *reinterpret_cast<int *>(a) == *reinterpret_cast<int *>(b); ``` which I think is more straightforward for what the compare function is doing.
Outdated
libc/test/src/search/lfind_test.cpp
nickdesaulniers duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
```suggestion #include <stddef.h> // size_t ```
Outdated
libc/src/search/lfind.h
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
Does POSIX say anything about these parameters being `nullptr`? Perhaps we should check the 4 pointer members first before attempting to dereference any of them?
Outdated
libc/src/search/lfind.cpp
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
```suggestion if (compar(key, next) == 0) return next; ``` https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Outdated
libc/src/search/lfind.cpp
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
Usually with interfaces that accept the number of members and a size, there's a potential for `*nmemb * size` to overflow, which should be checked by the library function. We should do that here. We seem to have a `mul_overflow` helper fn in __support/memory_size.h, perhaps that should be moved to __support/math_extras.h with the others, then used here?
Outdated
libc/src/search/lfind.cpp
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
You'd have to declare a type for the function pointer first, then use that as an ArgSpec. If you look at how other functions that have function pointer parameters, such as bsearch or qsort, those should provide an example.
Outdated
libc/spec/posix.td
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
These first two parameters need to be `const void *`. If you do a full build, you can `find <build dir> -name search.h` and check that the function signature generated matches the spec.
Outdated
libc/newhdrgen/yaml/search.yaml
duncpro
@nickdesaulniers nickdesaulniers Nov 4, 2024
The indentation looks off here. Can you triple check it please? One thing I highly recommend is modifying your editor to visually differentiate between tabs and spaces.
libc/config/baremetal/arm/entrypoints.txt
duncpro