Skip to content

Commit

Permalink
Rename FileDataLoader::From to ::from (#381)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #381

This is a commonly-used method that violates our style guide.
ghstack-source-id: 200971163
exported-using-ghexport

Reviewed By: cccclai

Differential Revision: D49351745

fbshipit-source-id: 7f3a6b7f02db5357a0fb97f71e9695077734f2ac
  • Loading branch information
dbort authored and facebook-github-bot committed Sep 18, 2023
1 parent 4ee204f commit e2dd0be
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 8 deletions.
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

0 comments on commit e2dd0be

Please sign in to comment.