[CORE] OV file utils file_size use std file path #28462

barnasm1 · 2025-01-15T15:26:28Z

Details:

refactor file_size function to accept ov::util::Path / std::filesystem::path. Be backward compatible with previous version.

Tickets:

146659

… paths

…codings

src/common/util/include/openvino/util/file_path.hpp

src/common/util/include/openvino/util/cpp_version.hpp

praasz · 2025-01-16T06:52:56Z

src/common/util/include/openvino/util/wstring_cast_util.hpp

These functions are not cast rename file or keep them in original header.
Why these function are moved to new file?

praasz · 2025-01-16T10:59:12Z

src/common/util/include/openvino/util/file_path.hpp

+#if defined(GCC_VER_LESS_THEN_12_3) || defined(CLANG_VER_LESS_THEN_17)
+inline ov::util::Path WPath(const std::wstring& wpath) {
+    return {ov::util::wstring_to_string(wpath)};
+}
+#else
+inline ov::util::Path WPath(const std::wstring& wpath) {
+    return {wpath};
+}
+#endif
+
 }  // namespace util
 }  // namespace ov


Suggested change

#if defined(GCC_VER_LESS_THEN_12_3) || defined(CLANG_VER_LESS_THEN_17)

inline ov::util::Path WPath(const std::wstring& wpath) {

return {ov::util::wstring_to_string(wpath)};

}

#else

inline ov::util::Path WPath(const std::wstring& wpath) {

return {wpath};

}

#endif

} // namespace util

} // namespace ov

} // namespace util

} // namespace ov

#if defined(GCC_VER_LESS_THEN_12_3) || defined(CLANG_VER_LESS_THEN_17)

template<>

ov::util::Path::path<std::wstring>(const std::wstring& source, format fmt) : path(ov::util::wstring_to_string(source), std::move(fmt)) {}

#endif

Consider add specialization for compiler version where is bug with path creation then WPath function is not required and path can be create always in same way.
Optionally: If any issue because specialization already defined add some trait check to detect it and avoid define it in OV.

praasz · 2025-01-16T11:00:19Z

src/common/util/include/openvino/util/cpp_version.hpp

+#endif
+
+#if defined(__GNUC__) && (__GNUC__ < 12 || __GNUC__ == 12 && __GNUC_MINOR__ < 3)
+#    define GCC_VER_LESS_THEN_12_3


Suggested change

# define GCC_VER_LESS_THEN_12_3

# define GCC_VER_LESS_THAN_12_3

Use than in case where something is compared.

praasz · 2025-01-16T11:16:17Z

src/common/util/include/openvino/util/file_util.hpp

@@ -172,26 +161,30 @@ bool directory_exists(const std::string& path);
 bool directory_exists(const std::wstring& path);
 #endif

+inline ov::util::Path cut_android_path(const ov::util::Path& file_name) {


This function is not required can be still internal part of file_size

praasz · 2025-01-16T11:26:40Z

src/common/util/include/openvino/util/file_util.hpp

-    if (pos != std::string::npos) {
-        file_name = file_name.substr(0, pos);
-    }
+inline int64_t file_size(const ov::util::Path& path) {


Suggested change

inline int64_t file_size(const ov::util::Path& path) {

inline int64_t file_size(const ov::util::Path& path) {

#if defined(__ANDROID__) || defined(ANDROID)

// apply cuting if std::filesytem not support it well.

#endif

return std::filesystem::file_size(path);

}

This function should probably be wrapper to std implementation if works.

praasz · 2025-01-16T11:32:55Z

src/core/tests/file_util.cpp

+        }
+        {
+            std::ofstream outfile("test_file_21.txt");
+            outfile << "This is a test file." << std::endl;


Suggested change

outfile << "This is a test file." << std::endl;

Remove there is no need to print messages if there is no failures.

praasz · 2025-01-16T11:35:36Z

src/core/tests/file_util.cpp

+        {
+            std::ofstream outfile("test_file_21.txt");
+            outfile << "This is a test file." << std::endl;
+            outfile.close();


Is it required? It should be closed when outfile will go out of scope of this block.

praasz · 2025-01-16T11:37:23Z

src/core/tests/file_util.cpp

+    ov::util::Path file_name = std::get<0>(GetParam());
+    ov::util::Path expected = std::get<1>(GetParam());


Suggested change

ov::util::Path file_name = std::get<0>(GetParam());

ov::util::Path expected = std::get<1>(GetParam());

const auto& [file_name, expected] = GetParam();

In cpp17 it should work.

praasz · 2025-01-16T11:44:39Z

src/common/util/include/openvino/util/file_util.hpp

 /**
 * @brief      Returns file size for file
 * @param[in]  path  The file name
 * @return     file size
 */
-inline int64_t file_size(const char* path) {


Still this version should be kept and marked deprecated before remove.

praasz · 2025-01-16T11:45:07Z

src/common/util/include/openvino/util/wstring_cast_util.hpp

@@ -0,0 +1,31 @@
+// Copyright (C) 2018-2024 Intel Corporation


Suggested change

// Copyright (C) 2018-2024 Intel Corporation

// Copyright (C) 2018-2025 Intel Corporation

Check all modified files

t-jankowski · 2025-01-16T10:14:15Z

src/common/util/include/openvino/util/cpp_version.hpp

+#endif
+
+#if defined(__GNUC__) && (__GNUC__ < 12 || __GNUC__ == 12 && __GNUC_MINOR__ < 3)
+#    define GCC_VER_LESS_THEN_12_3


Suggested change

# define GCC_VER_LESS_THEN_12_3

# define GCC_VER_LESS_THAN_12_3

t-jankowski · 2025-01-16T10:14:34Z

src/common/util/include/openvino/util/cpp_version.hpp

+#endif
+
+#if defined(__clang__) && __clang_major__ < 17
+#    define CLANG_VER_LESS_THEN_17


Suggested change

# define CLANG_VER_LESS_THEN_17

# define CLANG_VER_LESS_THAN_17

t-jankowski · 2025-01-16T10:20:44Z

src/common/util/include/openvino/util/file_path.hpp

@@ -30,5 +37,15 @@ using Path = std::filesystem::path;
 using Path = std::experimental::filesystem::path;
 #endif

+#if defined(GCC_VER_LESS_THEN_12_3) || defined(CLANG_VER_LESS_THEN_17)


Suggested change

#if defined(GCC_VER_LESS_THEN_12_3) || defined(CLANG_VER_LESS_THEN_17)

#if defined(GCC_VER_LESS_THAN_12_3) || defined(CLANG_VER_LESS_THAN_17)

t-jankowski · 2025-01-16T10:22:14Z

src/common/util/include/openvino/util/file_path.hpp

 namespace ov {
 namespace util {

 #if defined(OPENVINO_HAS_FILESYSTEM)
+// There are known issues related with usage of std::filesystem::path unocode represenataion:


Suggested change

// There are known issues related with usage of std::filesystem::path unocode represenataion:

// There are known issues with usage of std::filesystem::path unicode representation:

barnasm1 added 5 commits January 15, 2025 15:18

move wide character string conversions to separate file

887dd2b

refactor

04671c7

refactor file size functions to use ov::util::Path and handle Android…

b334528

… paths

add unit tests for cut_android_path function

c0ec93e

add unit tests for file size functions with various file types and en…

dce5d59

…codings

barnasm1 added the category: Core OpenVINO Core (aka ngraph) label Jan 15, 2025

barnasm1 self-assigned this Jan 15, 2025

barnasm1 requested review from a team as code owners January 15, 2025 15:26

github-actions bot added the category: build OpenVINO cmake script / infra label Jan 15, 2025

barnasm1 and others added 4 commits January 15, 2025 15:40

code format

701cd96

prevent variable redefinition for android

97f2a96

refactor test to use ov::util::Path for Unicode file output

c0345b1

Merge branch 'master' into ov_file_path_util_file_size

66615dc

praasz self-assigned this Jan 16, 2025

praasz added this to the 2025.1 milestone Jan 16, 2025

add Windows-specific includes for wstring_cast_util

a1e8053

ilya-lavrenov reviewed Jan 16, 2025

View reviewed changes

src/common/util/include/openvino/util/file_path.hpp Outdated Show resolved Hide resolved

src/common/util/include/openvino/util/file_path.hpp Outdated Show resolved Hide resolved

src/common/util/include/openvino/util/cpp_version.hpp Outdated Show resolved Hide resolved

barnasm1 added 2 commits January 16, 2025 12:26

fix comment typo

f34b2d2

refactor cpp_version.hpp and file_path.hpp for compiler version checks

2d071bf

praasz reviewed Jan 16, 2025

View reviewed changes

t-jankowski requested changes Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE] OV file utils file_size use std file path #28462

[CORE] OV file utils file_size use std file path #28462

barnasm1 commented Jan 15, 2025

praasz Jan 16, 2025

praasz Jan 16, 2025

praasz Jan 16, 2025

praasz Jan 16, 2025

praasz Jan 16, 2025 •

edited

Loading

praasz Jan 16, 2025

praasz Jan 16, 2025

praasz Jan 16, 2025

praasz Jan 16, 2025

praasz Jan 16, 2025

t-jankowski Jan 16, 2025

t-jankowski Jan 16, 2025

t-jankowski Jan 16, 2025

t-jankowski Jan 16, 2025

	# define GCC_VER_LESS_THEN_12_3
	# define GCC_VER_LESS_THAN_12_3

-inline int64_t file_size(const ov::util::Path& path) {
+inline int64_t file_size(const ov::util::Path& path) {
+    #if defined(__ANDROID__) || defined(ANDROID)
+    // apply cuting if std::filesytem not support it well.
+    #endif
+    return std::filesystem::file_size(path);
+}

		ov::util::Path file_name = std::get<0>(GetParam());
		ov::util::Path expected = std::get<1>(GetParam());

	ov::util::Path file_name = std::get<0>(GetParam());
	ov::util::Path expected = std::get<1>(GetParam());
	const auto& [file_name, expected] = GetParam();

		@@ -0,0 +1,31 @@
		// Copyright (C) 2018-2024 Intel Corporation

	// Copyright (C) 2018-2024 Intel Corporation
	// Copyright (C) 2018-2025 Intel Corporation

	# define CLANG_VER_LESS_THEN_17
	# define CLANG_VER_LESS_THAN_17

	#if defined(GCC_VER_LESS_THEN_12_3) \|\| defined(CLANG_VER_LESS_THEN_17)
	#if defined(GCC_VER_LESS_THAN_12_3) \|\| defined(CLANG_VER_LESS_THAN_17)

	// There are known issues related with usage of std::filesystem::path unocode represenataion:
	// There are known issues with usage of std::filesystem::path unicode representation:

[CORE] OV file utils file_size use std file path #28462

Are you sure you want to change the base?

[CORE] OV file utils file_size use std file path #28462

Conversation

barnasm1 commented Jan 15, 2025

Details:

Tickets:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praasz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praasz Jan 16, 2025 •

edited

Loading