Skip to content

[executorch] Rename FileDataLoader::From to ::from #381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion extension/data_loader/file_data_loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ FileDataLoader::~FileDataLoader() {
::close(fd_);
}

Result<FileDataLoader> FileDataLoader::From(
Result<FileDataLoader> FileDataLoader::from(
const char* file_name,
size_t alignment) {
ET_CHECK_OR_RETURN_ERROR(
Expand Down
9 changes: 8 additions & 1 deletion extension/data_loader/file_data_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,17 @@ class FileDataLoader : public DataLoader {
* could not be found.
* @retval Error::MemoryAllocationFailed Internal memory allocation failure.
*/
static Result<FileDataLoader> From(
static Result<FileDataLoader> from(
const char* file_name,
size_t alignment = alignof(std::max_align_t));

/// DEPRECATED: Use the lowercase `from()` instead.
__ET_DEPRECATED static Result<FileDataLoader> From(
const char* file_name,
size_t alignment = alignof(std::max_align_t)) {
return from(file_name, alignment);
}

// Movable to be compatible with Result.
FileDataLoader(FileDataLoader&& rhs) noexcept
: file_name_(rhs.file_name_),
Expand Down
32 changes: 26 additions & 6 deletions extension/data_loader/test/file_data_loader_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ TEST_P(FileDataLoaderTest, InBoundsLoadsSucceed) {

// Wrap it in a loader.
Result<FileDataLoader> fdl =
FileDataLoader::From(tf.path().c_str(), alignment());
FileDataLoader::from(tf.path().c_str(), alignment());
ASSERT_EQ(fdl.error(), Error::Ok);

// size() should succeed and reflect the total size.
Expand Down Expand Up @@ -113,7 +113,7 @@ TEST_P(FileDataLoaderTest, OutOfBoundsLoadFails) {
TempFile tf(data, sizeof(data));

Result<FileDataLoader> fdl =
FileDataLoader::From(tf.path().c_str(), alignment());
FileDataLoader::from(tf.path().c_str(), alignment());
ASSERT_EQ(fdl.error(), Error::Ok);

// Loading beyond the end of the data should fail.
Expand All @@ -133,7 +133,7 @@ TEST_P(FileDataLoaderTest, OutOfBoundsLoadFails) {

TEST_P(FileDataLoaderTest, FromMissingFileFails) {
// Wrapping a file that doesn't exist should fail.
Result<FileDataLoader> fdl = FileDataLoader::From(
Result<FileDataLoader> fdl = FileDataLoader::from(
"/tmp/FILE_DOES_NOT_EXIST_EXECUTORCH_MMAP_LOADER_TEST");
EXPECT_NE(fdl.error(), Error::Ok);
}
Expand All @@ -145,15 +145,15 @@ TEST_P(FileDataLoaderTest, BadAlignmentFails) {

// Creating a loader with default alignment works fine.
{
Result<FileDataLoader> fdl = FileDataLoader::From(tf.path().c_str());
Result<FileDataLoader> fdl = FileDataLoader::from(tf.path().c_str());
ASSERT_EQ(fdl.error(), Error::Ok);
}

// Bad alignments fail.
const std::vector<size_t> bad_alignments = {0, 3, 5, 17};
for (size_t bad_alignment : bad_alignments) {
Result<FileDataLoader> fdl =
FileDataLoader::From(tf.path().c_str(), bad_alignment);
FileDataLoader::from(tf.path().c_str(), bad_alignment);
ASSERT_EQ(fdl.error(), Error::InvalidArgument);
}
}
Expand All @@ -164,7 +164,7 @@ TEST_P(FileDataLoaderTest, MoveCtor) {
std::string contents = "FILE_CONTENTS";
TempFile tf(contents);
Result<FileDataLoader> fdl =
FileDataLoader::From(tf.path().c_str(), alignment());
FileDataLoader::from(tf.path().c_str(), alignment());
ASSERT_EQ(fdl.error(), Error::Ok);
EXPECT_EQ(fdl->size().get(), contents.size());

Expand All @@ -184,6 +184,26 @@ TEST_P(FileDataLoaderTest, MoveCtor) {
EXPECT_EQ(0, std::memcmp(fb->data(), contents.data(), fb->size()));
}

// Test that the deprecated From method (capital 'F') still works.
TEST_P(FileDataLoaderTest, DEPRECATEDFrom) {
// Write some heterogeneous data to a file.
uint8_t data[256];
for (int i = 0; i < sizeof(data); ++i) {
data[i] = i;
}
TempFile tf(data, sizeof(data));

// Wrap it in a loader.
Result<FileDataLoader> fdl =
FileDataLoader::From(tf.path().c_str(), alignment());
ASSERT_EQ(fdl.error(), Error::Ok);

// size() should succeed and reflect the total size.
Result<size_t> size = fdl->size();
ASSERT_EQ(size.error(), Error::Ok);
EXPECT_EQ(*size, sizeof(data));
}

// Run all FileDataLoaderTests multiple times, varying the return value of
// `GetParam()` based on the `testing::Values` list. The tests will interpret
// the value as "alignment".
Expand Down