[Support] Add cross-platform memory stream utility #121
Conversation
Add memstream.h with MemStream and FprintAdapter classes that provide a FILE* interface backed by memory. On Linux, uses open_memstream. On Windows, uses a temporary file approach for compatibility. Update logging.h to use the new cross-platform FprintAdapter, making FprintToString an alias for backward compatibility. Signed-off-by: Rob Suderman <[email protected]>
There was a problem hiding this comment.
LGTM modulo comments. FprintToString was initially added by @AaronStGeorge so would be great if Aaron can chime in (post-merge is fine too).
| @@ -0,0 +1,219 @@ | |||
| // Copyright 2025 Advanced Micro Devices, Inc. | |||
There was a problem hiding this comment.
| // Copyright 2025 Advanced Micro Devices, Inc. | |
| // Copyright 2026 Advanced Micro Devices, Inc. |
| // On Windows, it uses a pure C++ implementation with temporary files. | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
There was a problem hiding this comment.
Please include memstream.h, target_platform.h and python_utils.h in the main fusilli.h.
| @@ -0,0 +1,175 @@ | |||
| // Copyright 2025 Advanced Micro Devices, Inc. | |||
There was a problem hiding this comment.
| // Copyright 2025 Advanced Micro Devices, Inc. | |
| // Copyright 2026 Advanced Micro Devices, Inc. |
| #include <cstdio> | ||
| #include <cstdlib> | ||
| #include <cstring> | ||
| #include <string> | ||
|
|
||
| #include "fusilli/support/target_platform.h" |
There was a problem hiding this comment.
nit: Move fusilli include up before the system includes
| if (GetTempPathA(MAX_PATH, tempPath) == 0) { | ||
| return; | ||
| } | ||
| if (GetTempFileNameA(tempPath, "mem", 0, tempFileName) == 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit: remove braces for single line ifs
| bool isValid() const { return stream_ != nullptr; } | ||
|
|
||
| // Retrieve the contents as a string. | ||
| std::string str() { |
There was a problem hiding this comment.
Several file operations here could fail - fflush, fseek. Consider checking for success or returning empty on failure.
| } | ||
|
|
||
| // Get current size of the stream. | ||
| size_t size() { |
There was a problem hiding this comment.
same (consider error checking / returning zero size on failure)
| bool isValid() const { return stream_ != nullptr; } | ||
|
|
||
| // Retrieve the contents as a string. | ||
| std::string str() { |
There was a problem hiding this comment.
Both str and size have side effects (fflush modifies internal state) despite it not being apparent from the name. Consider documenting this in the docstring.
| // Example usage: | ||
| // std::string output; | ||
| // { | ||
| // FprintAdapter adapter(output); | ||
| // fprintf(adapter, "Value: %d", 42); | ||
| // } // adapter destructor copies content to output | ||
| // // output now contains "Value: 42" |
There was a problem hiding this comment.
Since this is a potential footgun, worth documenting that if nothing is written (no fprintf), it clears any output previously stored
std::string output = "initial";
{
FprintAdapter adapter(output);
}
// output is now empty ""| fwrite(data, 1, sizeof(data) - 1, ms); | ||
| std::string result = ms.str(); | ||
| REQUIRE(result.size() == sizeof(data) - 1); | ||
| REQUIRE(memcmp(result.data(), data, sizeof(data) - 1) == 0); |
There was a problem hiding this comment.
minor nit: could be simplified as
REQUIRE(result == std::string(data, sizeof(data) - 1));| #include <fcntl.h> | ||
| #include <vector> |
| #include <fcntl.h> | ||
| #include <vector> | ||
| #elif FUSILLI_PLATFORM_WINDOWS | ||
| #include <stdio.h> |
There was a problem hiding this comment.
nit: maybe redundant (since you have up top)
2 participants
Add memstream.h with MemStream and FprintAdapter classes that provide a FILE* interface backed by memory. On Linux, uses open_memstream. On Windows, uses a temporary file approach for compatibility.
Update logging.h to use the new cross-platform FprintAdapter, making FprintToString an alias for backward compatibility.