Jump to conversation
Unresolved conversations (4)
@nickdesaulniers nickdesaulniers Nov 20, 2024
This test fails for me: ``` $ ninja libc_time_unittests ... [ RUN ] LlvmLibcCtimeS.Nullptr /android0/llvm-project/libc/test/src/time/ctime_s_test.cpp:25: FAILURE Expected: '\0' Which is: 0 To be equal to: buffer[0] Which is: 74 ``` Because `nullptr` is not a constraint violation, you can just remove this assert. ```suggestion ```
libc/test/src/time/ctime_s_test.cpp
Rajveer100
@nickdesaulniers nickdesaulniers Nov 20, 2024
Do you need this define? This header is only included for the implementation in ctime_s.cpp, right? ```suggestion ```
libc/src/time/ctime_s.h
nickdesaulniers
@nickdesaulniers nickdesaulniers Nov 20, 2024
Are you using anything from <stdint.h>? Can we remove this? ```suggestion ```
libc/src/time/ctime_s.h
Rajveer100 nickdesaulniers
@nickdesaulniers nickdesaulniers Oct 17, 2024
I wonder if we should guard this on `__STDC_WANT_LIB_EXT1__` being defined as `1`? See `K.3.8.3.3 The ctime_s function`.
libc/newhdrgen/yaml/time.yaml
Rajveer100 nickdesaulniers
Resolved conversations (25)
@nickdesaulniers nickdesaulniers Nov 19, 2024
This ASSERT should fail now, right?
Outdated
libc/test/src/time/ctime_s_test.cpp
Rajveer100
@nickdesaulniers nickdesaulniers Nov 19, 2024
Remove parens for single statement conditionals. https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Outdated
libc/src/time/ctime_s.cpp
@Rajveer100 Rajveer100 Nov 15, 2024
We do have a check. Same for the test too.
libc/src/time/ctime_s.cpp
@nickdesaulniers nickdesaulniers Nov 14, 2024
K.3.8.3.3.3: > If there is a runtime-constraint violation, s[0] is set to a null character if s is not a null pointer and maxsize is not equal zero and is not greater than RSIZE_MAX But what's funny to me is that the sentence before (K.3.8.3.3.2) literally says maxsize shall not be less than 26. So if we first check that maxsize is not less than 26, then it would implicitly mean maxsize is not zero. So if you implement my above check, I think we can elide this `buffer_size != 0` check here.
Outdated
libc/src/time/ctime_s.cpp
Rajveer100 nickdesaulniers
@nickdesaulniers nickdesaulniers Nov 14, 2024
K.3.8.3.3.2: > maxsize shall not be less than 26 and shall not be greater than RSIZE_MAX. So we're missing a comparison against 26 (and a test for `buffer_size` being 26 or less).
libc/src/time/ctime_s.cpp
Rajveer100
@nickdesaulniers nickdesaulniers Nov 14, 2024
```suggestion time_t t = 2147483648; int result = LIBC_NAMESPACE::ctime_s(buffer, sizeof(buffer), &t); ```
Outdated
libc/test/src/time/ctime_s_test.cpp
@nickdesaulniers nickdesaulniers Nov 14, 2024
```suggestion // 2038-01-19 03:14:07. Test with a valid buffer size. time_t t = 2147483647; int result = LIBC_NAMESPACE::ctime_s(buffer, sizeof(buffer), &t); ```
Outdated
libc/test/src/time/ctime_s_test.cpp
@nickdesaulniers nickdesaulniers Nov 14, 2024
```suggestion // 1970-01-01 00:00:00. Test with a valid buffer size. time_t t = 0; int result = LIBC_NAMESPACE::ctime_s(buffer, sizeof(buffer), &t); ```
Outdated
libc/test/src/time/ctime_s_test.cpp
@nickdesaulniers nickdesaulniers Nov 14, 2024
```suggestion time_t t = 0; int result = LIBC_NAMESPACE::ctime_s(buffer, 0, &t); ```
Outdated
libc/test/src/time/ctime_s_test.cpp
@nickdesaulniers nickdesaulniers Nov 14, 2024
```suggestion int result = LIBC_NAMESPACE::ctime_s(nullptr, 0, nullptr); ```
Outdated
libc/test/src/time/ctime_s_test.cpp
@nickdesaulniers nickdesaulniers Nov 14, 2024
Oh, I missed this before. I didn't spot which feature from C++20 you were using in the unit test. Can you remove this, or point out which feature you needed this change for?
Outdated
libc/test/src/time/CMakeLists.txt
@nickdesaulniers nickdesaulniers Nov 12, 2024
```suggestion (char *buffer, rsize_t buffer_size, const time_t *t_ptr)) { // TODO(https://github.com/llvm/llvm-project/issues/115907): invoke constraint handler if (buffer == nullptr || t_ptr == nullptr) ```
Outdated
libc/src/time/ctime_s.cpp
Rajveer100
@nickdesaulniers nickdesaulniers Nov 4, 2024
Don't we need to zero the first element of `buffer` in this case?
libc/src/time/ctime_s.cpp
@nickdesaulniers nickdesaulniers Nov 4, 2024
What happens if `buffer` is `nullptr` (while the other params are ok)? We'll end up dereferencing it, which we do not want to do. So I think you should just start out with: ```c++ if (buffer == nullptr || t_ptr == nullptr) return EINVAL; ``` then move on to validating the other inputs.
Outdated
libc/src/time/ctime_s.cpp
@nickdesaulniers nickdesaulniers Oct 31, 2024
What are you using from stddef.h? You should be using proxy headers (hdr/) here.
Outdated
libc/src/time/ctime_s.h
Rajveer100
@nickdesaulniers nickdesaulniers Oct 31, 2024
```suggestion #include "size_t.h" ```
Outdated
libc/include/llvm-libc-types/rsize_t.h
Rajveer100 nickdesaulniers
@zimirza zimirza Oct 30, 2024
~Is this needed since it is just returning an error?~ I did not think about the C standard. Sorry about that.
libc/src/time/ctime_s.cpp
Rajveer100 nickdesaulniers
@michaelrj-google michaelrj-google Oct 30, 2024
this typedef should be in a proper type header, not here.
Outdated
libc/src/time/ctime_s.h
Rajveer100
@nickdesaulniers nickdesaulniers Oct 17, 2024
K.3.8.3.3 describes `ctime_s`: > errno_t ctime_s(char *s, rsize_t maxsize, const time_t *timer); 2 Neither s nor timer shall be a null pointer. maxsize shall not be less than 26 and shall not be greater than RSIZE_MAX. 3 If there is a runtime-constraint violation, s[0] is set to a null character if s is not a null pointer and maxsize is not equal zero and is not greater than RSIZE_MAX. So setting `buffer[0] = '\0'` should be handled in this PR. Please do so, then add a test.
Outdated
libc/src/time/ctime_s.cpp
Rajveer100 nickdesaulniers
@nickdesaulniers nickdesaulniers Oct 8, 2024
``` /android0/llvm-project/libc/src/time/ctime_s.cpp:23:13: error: comparison between pointer and integer ('time_t (*)(time_t *) noexcept' (aka 'long (*)(long *) noexcept') and 'int') 23 | *time > cpp::numeric_limits<int32_t>::max()) { | ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ```
Outdated
libc/src/time/ctime_s.cpp
@nickdesaulniers nickdesaulniers Oct 8, 2024
``` /android0/llvm-project/libc/src/time/ctime_s.h:17:27: error: unknown type name 'size_t'; did you mean 'time_t'? 17 | int ctime_s(char *buffer, size_t buffer_size, const time_t *t_ptr); | ^~~~~~ | time_t ``` Make sure to `#include <stddef.h>`.
Outdated
libc/src/time/ctime_s.h
@michaelrj-google michaelrj-google Oct 2, 2024
`time_t` may not be a 32 bit number, and making this check may cause problems in approximately 14 years: https://en.wikipedia.org/wiki/Year_2038_problem
Outdated
libc/src/time/ctime_s.cpp
Rajveer100 nickdesaulniers
@SchrodingerZhu SchrodingerZhu Oct 2, 2024
Missing `const` qualifier?
Outdated
libc/spec/stdc.td
Rajveer100
@lntue lntue Oct 1, 2024
use `"hdr/errno_macros.h"` instead.
Outdated
libc/src/time/ctime_s.cpp
@nickdesaulniers nickdesaulniers Oct 1, 2024
It's llvm style to omit `{}` for single statement conditionals; please omit them. https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
Outdated
libc/src/time/ctime_s.cpp
nickdesaulniers Rajveer100