[libc-commits] [libc] [libc]: Clean up unnecessary function pointers in scanf (PR #121215)
via libc-commits
libc-commits at lists.llvm.org
Fri Dec 27 08:31:39 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-libc
Author: Vinay Deshmukh (vinay-deshmukh)
<details>
<summary>Changes</summary>
Resolves #<!-- -->115394
1. Move definition of `Reader::getc` from `.h` to `.cpp` similar to where `Reader::ungetc` is defined.
2. Remove function pointer members to define them once per platform in .cpp
3. Tried to test in "overlay mode": via `ninja libc check-libc` - https://libc.llvm.org/overlay_mode.html#id3 but the build fails locally due to the issue mentioned in this issue has appeared for me: https://github.com/llvm/llvm-project/issues/102145
Also, full build doesn't yet build on macos m1 AFAIK
---
Full diff: https://github.com/llvm/llvm-project/pull/121215.diff
3 Files Affected:
- (modified) libc/src/stdio/scanf_core/reader.cpp (+64-1)
- (modified) libc/src/stdio/scanf_core/reader.h (+3-23)
- (modified) libc/src/stdio/scanf_core/vfscanf_internal.h (+1-30)
``````````diff
diff --git a/libc/src/stdio/scanf_core/reader.cpp b/libc/src/stdio/scanf_core/reader.cpp
index ec1f5c098dc7a6..ccfcd220e19c21 100644
--- a/libc/src/stdio/scanf_core/reader.cpp
+++ b/libc/src/stdio/scanf_core/reader.cpp
@@ -6,13 +6,75 @@
//
//===----------------------------------------------------------------------===//
+#include "hdr/types/FILE.h"
#include "src/stdio/scanf_core/reader.h"
#include "src/__support/macros/config.h"
+#include "src/__support/File/file.h"
+
#include <stddef.h>
+
+
+
namespace LIBC_NAMESPACE_DECL {
namespace scanf_core {
+namespace internal {
+
+#if defined(LIBC_TARGET_ARCH_IS_GPU)
+// The GPU build provides FILE access through the host operating system's
+// library. So here we simply use the public entrypoints like in the SYSTEM_FILE
+// interface. Entrypoints should normally not call others, this is an exception.
+// FIXME: We do not acquire any locks here, so this is not thread safe.
+LIBC_INLINE int getc(void *f) {
+ return LIBC_NAMESPACE::getc(reinterpret_cast<::FILE *>(f));
+}
+
+LIBC_INLINE void ungetc(int c, void *f) {
+ LIBC_NAMESPACE::ungetc(c, reinterpret_cast<::FILE *>(f));
+}
+
+#elif !defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE)
+
+LIBC_INLINE int getc(void *f) {
+ unsigned char c;
+ auto result =
+ reinterpret_cast<LIBC_NAMESPACE::File *>(f)->read_unlocked(&c, 1);
+ size_t r = result.value;
+ if (result.has_error() || r != 1)
+ return '\0';
+
+ return c;
+}
+
+LIBC_INLINE void ungetc(int c, void *f) {
+ reinterpret_cast<LIBC_NAMESPACE::File *>(f)->ungetc_unlocked(c);
+}
+
+#else // defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE)
+
+// Since ungetc_unlocked isn't always available, we don't acquire the lock for
+// system files.
+LIBC_INLINE int getc(void *f) { return ::getc(reinterpret_cast<::FILE *>(f)); }
+
+LIBC_INLINE void ungetc(int c, void *f) {
+ ::ungetc(c, reinterpret_cast<::FILE *>(f));
+}
+#endif // LIBC_COPT_STDIO_USE_SYSTEM_FILE
+
+} // namespace internal
+
+char Reader::getc() {
+ ++cur_chars_read;
+ if (rb != nullptr) {
+ char output = rb->buffer[rb->buff_cur];
+ ++(rb->buff_cur);
+ return output;
+ }
+ // This should reset the buffer if applicable.
+ return static_cast<char>(internal::getc(input_stream));
+}
+
void Reader::ungetc(char c) {
--cur_chars_read;
if (rb != nullptr && rb->buff_cur > 0) {
@@ -23,7 +85,8 @@ void Reader::ungetc(char c) {
--(rb->buff_cur);
return;
}
- stream_ungetc(static_cast<int>(c), input_stream);
+ internal::ungetc(static_cast<int>(c), input_stream);
}
+
} // namespace scanf_core
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/stdio/scanf_core/reader.h b/libc/src/stdio/scanf_core/reader.h
index f984fd93789109..e29ea6ea7d8917 100644
--- a/libc/src/stdio/scanf_core/reader.h
+++ b/libc/src/stdio/scanf_core/reader.h
@@ -16,9 +16,6 @@
namespace LIBC_NAMESPACE_DECL {
namespace scanf_core {
-using StreamGetc = int (*)(void *);
-using StreamUngetc = void (*)(int, void *);
-
// This is intended to be either a raw string or a buffer syncronized with the
// file's internal buffer.
struct ReadBuffer {
@@ -29,38 +26,21 @@ struct ReadBuffer {
class Reader {
ReadBuffer *rb;
-
void *input_stream = nullptr;
-
- // TODO: Remove these unnecessary function pointers
- StreamGetc stream_getc = nullptr;
- StreamUngetc stream_ungetc = nullptr;
-
size_t cur_chars_read = 0;
public:
// TODO: Set buff_len with a proper constant
LIBC_INLINE Reader(ReadBuffer *string_buffer) : rb(string_buffer) {}
- LIBC_INLINE Reader(void *stream, StreamGetc stream_getc_in,
- StreamUngetc stream_ungetc_in,
+ LIBC_INLINE Reader(void *stream,
ReadBuffer *stream_buffer = nullptr)
- : rb(stream_buffer), input_stream(stream), stream_getc(stream_getc_in),
- stream_ungetc(stream_ungetc_in) {}
+ : rb(stream_buffer), input_stream(stream){}
// This returns the next character from the input and advances it by one
// character. When it hits the end of the string or file it returns '\0' to
// signal to stop parsing.
- LIBC_INLINE char getc() {
- ++cur_chars_read;
- if (rb != nullptr) {
- char output = rb->buffer[rb->buff_cur];
- ++(rb->buff_cur);
- return output;
- }
- // This should reset the buffer if applicable.
- return static_cast<char>(stream_getc(input_stream));
- }
+ char getc();
// This moves the input back by one character, placing c into the buffer if
// this is a file reader, else c is ignored.
diff --git a/libc/src/stdio/scanf_core/vfscanf_internal.h b/libc/src/stdio/scanf_core/vfscanf_internal.h
index 67126431fcded5..84d074711b8fb6 100644
--- a/libc/src/stdio/scanf_core/vfscanf_internal.h
+++ b/libc/src/stdio/scanf_core/vfscanf_internal.h
@@ -38,14 +38,6 @@ LIBC_INLINE void flockfile(::FILE *) { return; }
LIBC_INLINE void funlockfile(::FILE *) { return; }
-LIBC_INLINE int getc(void *f) {
- return LIBC_NAMESPACE::getc(reinterpret_cast<::FILE *>(f));
-}
-
-LIBC_INLINE void ungetc(int c, void *f) {
- LIBC_NAMESPACE::ungetc(c, reinterpret_cast<::FILE *>(f));
-}
-
LIBC_INLINE int ferror_unlocked(::FILE *f) { return LIBC_NAMESPACE::ferror(f); }
#elif !defined(LIBC_COPT_STDIO_USE_SYSTEM_FILE)
@@ -58,21 +50,6 @@ LIBC_INLINE void funlockfile(FILE *f) {
reinterpret_cast<LIBC_NAMESPACE::File *>(f)->unlock();
}
-LIBC_INLINE int getc(void *f) {
- unsigned char c;
- auto result =
- reinterpret_cast<LIBC_NAMESPACE::File *>(f)->read_unlocked(&c, 1);
- size_t r = result.value;
- if (result.has_error() || r != 1)
- return '\0';
-
- return c;
-}
-
-LIBC_INLINE void ungetc(int c, void *f) {
- reinterpret_cast<LIBC_NAMESPACE::File *>(f)->ungetc_unlocked(c);
-}
-
LIBC_INLINE int ferror_unlocked(FILE *f) {
return reinterpret_cast<LIBC_NAMESPACE::File *>(f)->error_unlocked();
}
@@ -85,12 +62,6 @@ LIBC_INLINE void flockfile(::FILE *) { return; }
LIBC_INLINE void funlockfile(::FILE *) { return; }
-LIBC_INLINE int getc(void *f) { return ::getc(reinterpret_cast<::FILE *>(f)); }
-
-LIBC_INLINE void ungetc(int c, void *f) {
- ::ungetc(c, reinterpret_cast<::FILE *>(f));
-}
-
LIBC_INLINE int ferror_unlocked(::FILE *f) { return ::ferror(f); }
#endif // LIBC_COPT_STDIO_USE_SYSTEM_FILE
@@ -103,7 +74,7 @@ LIBC_INLINE int vfscanf_internal(::FILE *__restrict stream,
const char *__restrict format,
internal::ArgList &args) {
internal::flockfile(stream);
- scanf_core::Reader reader(stream, &internal::getc, internal::ungetc);
+ scanf_core::Reader reader(stream);
int retval = scanf_core::scanf_main(&reader, format, args);
if (retval == 0 && internal::ferror_unlocked(stream))
retval = EOF;
``````````
</details>
https://github.com/llvm/llvm-project/pull/121215
More information about the libc-commits
mailing list