fix: redudant moves and unused variables warnings #114

AndreCostaaa · 2025-07-01T12:51:15Z

Hi fantastic project you have here

While integrating it into a project with -Wall -Wextra -Wpedantic I got a bunch of warnings that I figured would resolve and contribute upstream

Let me know what you think

To reproduce the warnings:

❯ cmake -B build -DCMAKE_CXX_FLAGS="-Wall -Wextra -Wpedantic"
-- The C compiler identification is GNU 14.3.1
-- The CXX compiler identification is GNU 14.3.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Could NOT find simdjson (missing: simdjson_DIR)
-- fastgltf: Found simdjson (Version 3.12.3)
-- Configuring done (0.4s)
-- Generating done (0.0s)
-- Build files have been written to: /home/andre/dev/lvgl/fastgltf/build
❯ cmake --build build -j$(nproc)
[ 40%] Building CXX object CMakeFiles/fastgltf.dir/src/fastgltf.cpp.o
[ 40%] Building CXX object CMakeFiles/fastgltf.dir/deps/simdjson/simdjson.cpp.o
[ 80%] Building CXX object CMakeFiles/fastgltf.dir/src/base64.cpp.o
[ 80%] Building CXX object CMakeFiles/fastgltf.dir/src/io.cpp.o
In file included from /home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp:51:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp: In static member function ‘static fastgltf::Expected<fastgltf::GltfDataBuffer> fastgltf::GltfDataBuffer::FromPath(const std::filesystem::__cxx11::path&)’:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:628:41: warning: redundant move in return statement [-Wredundant-move]
  628 |                         return std::move(buffer);
      |                                ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:628:41: note: remove ‘std::move’ call
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp: In static member function ‘static fastgltf::Expected<fastgltf::GltfDataBuffer> fastgltf::GltfDataBuffer::FromBytes(const std::byte*, std::size_t)’:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:636:41: warning: redundant move in return statement [-Wredundant-move]
  636 |                         return std::move(buffer);
      |                                ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:636:41: note: remove ‘std::move’ call
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp: In static member function ‘static fastgltf::Expected<fastgltf::MappedGltfFile> fastgltf::MappedGltfFile::FromPath(const std::filesystem::__cxx11::path&)’:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:703:41: warning: redundant move in return statement [-Wredundant-move]
  703 |                         return std::move(buffer);
      |                                ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:703:41: note: remove ‘std::move’ call
In file included from /home/andre/dev/lvgl/fastgltf/src/io.cpp:29:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp: In static member function ‘static fastgltf::Expected<fastgltf::GltfDataBuffer> fastgltf::GltfDataBuffer::FromPath(const std::filesystem::__cxx11::path&)’:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:628:41: warning: redundant move in return statement [-Wredundant-move]
  628 |                         return std::move(buffer);
      |                                ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:628:41: note: remove ‘std::move’ call
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp: In static member function ‘static fastgltf::Expected<fastgltf::GltfDataBuffer> fastgltf::GltfDataBuffer::FromBytes(const std::byte*, std::size_t)’:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:636:41: warning: redundant move in return statement [-Wredundant-move]
  636 |                         return std::move(buffer);
      |                                ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:636:41: note: remove ‘std::move’ call
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp: In static member function ‘static fastgltf::Expected<fastgltf::MappedGltfFile> fastgltf::MappedGltfFile::FromPath(const std::filesystem::__cxx11::path&)’:
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:703:41: warning: redundant move in return statement [-Wredundant-move]
  703 |                         return std::move(buffer);
      |                                ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/include/fastgltf/core.hpp:703:41: note: remove ‘std::move’ call
/home/andre/dev/lvgl/fastgltf/src/io.cpp: In member function ‘virtual fastgltf::span<std::byte> fastgltf::GltfDataBuffer::read(std::size_t, std::size_t)’:
/home/andre/dev/lvgl/fastgltf/src/io.cpp:103:77: warning: unused parameter ‘padding’ [-Wunused-parameter]
  103 | fg::span<std::byte> fg::GltfDataBuffer::read(std::size_t count, std::size_t padding) {
      |                                                                 ~~~~~~~~~~~~^~~~~~~
/home/andre/dev/lvgl/fastgltf/src/io.cpp: In member function ‘virtual fastgltf::span<std::byte> fastgltf::MappedGltfFile::read(std::size_t, std::size_t)’:
/home/andre/dev/lvgl/fastgltf/src/io.cpp:297:77: warning: unused parameter ‘padding’ [-Wunused-parameter]
  297 | fg::span<std::byte> fg::MappedGltfFile::read(std::size_t count, std::size_t padding) {
      |                                                                 ~~~~~~~~~~~~^~~~~~~
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp: In member function ‘fastgltf::Expected<fastgltf::Asset> fastgltf::Parser::parse(simdjson::dom::object, fastgltf::Category)’:
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp:1621:25: warning: redundant move in return statement [-Wredundant-move]
 1621 |         return std::move(asset);
      |                ~~~~~~~~~^~~~~~~
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp:1621:25: note: remove ‘std::move’ call
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp: In member function ‘fastgltf::Expected<fastgltf::ExportResult<std::__cxx11::basic_string<char> > > fastgltf::Exporter::writeGltfJson(const fastgltf::Asset&, fastgltf::ExportOptions)’:
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp:7039:21: warning: redundant move in return statement [-Wredundant-move]
 7039 |     return std::move(result);
      |            ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp:7039:21: note: remove ‘std::move’ call
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp: In member function ‘fastgltf::Expected<fastgltf::ExportResult<std::vector<std::byte, std::allocator<std::byte> > > > fastgltf::Exporter::writeGltfBinary(const fastgltf::Asset&, fastgltf::ExportOptions)’:
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp:7136:21: warning: redundant move in return statement [-Wredundant-move]
 7136 |     return std::move(result);
      |            ~~~~~~~~~^~~~~~~~
/home/andre/dev/lvgl/fastgltf/src/fastgltf.cpp:7136:21: note: remove ‘std::move’ call
[100%] Linking CXX static library libfastgltf.a
[100%] Built target fastgltf

Pending spnda/fastgltf#114 merge

spnda · 2025-07-01T13:47:42Z

While I see the idea, I personally think these are invalid warnings. The code you referenced is templated code, and while the cases where the warnings appear are for types that do not have a copy-constructor, it would be unexpected behavior for types that do have a copy constructor. I explicitly want to use the move constructor in all of these places.

GCC 15.1 locally also throws these warnings, but LLVM 20 does not. I mean technically one could write some if constexpr code to switch between these two cases I feel like that'd be a little weird:

if constexpr (std::is_copy_constructible_v<T>) {
    return std::move(value);
} else {
    return value;
}

Therefore I feel like the proposed changes shouldn't be merged. I understand that people use -Werror, so perhaps a workaround of some sort would be applicable like perhaps disabling the warning for the fastgltf source files. However, I would also say that when people use -Werror they should make sure to only use that warning on their source files, and not on libraries, since things like this can happen.

AndreCostaaa · 2025-07-01T19:28:52Z

Hi @spnda

I was a bit confused by what you meant and only then realized that I had actually removed std::move from places where it definitely makes sense to keep them. Just fixed this PR

Could I bother to take a look at this again ? You will find that the actual std::move that should be removed and were causing warnings were from functions that try to move while returning local variables which actually prevents the compiler from performing some optimizations like constructing the object in the caller's memory location directly and it's why the warning arises

Sorry for the confusion

spnda · 2025-07-01T20:04:25Z

Could I bother to take a look at this again ? You will find that the actual std::move that should be removed and were causing warnings were from functions that try to move while returning local variables which actually prevents the compiler from performing some optimizations like constructing the object in the caller's memory location directly and it's why the warning arises

Yes, return std::move(foo) is usually bad practice since it will make the compiler unable to perform RVO or NRVO. However, that's not what's happening here. These functions return Expected<T> but the type being moved is Tz What's actually happening here is we're constructing a Expected<T> using the Expected(T&&) constructor, and then relying on the compiler to perform RVO on that object. You can check this by specifying -Wnrvo (iirc that's the name), which will give a warning when the compiler is unable to perform RVO, and that shouldn't be the case here. Please correct me if I'm wrong, though.

AndreCostaaa · 2025-07-01T20:11:12Z

The changes to Expected<T> are the ones that weren't actually producing warnings and I had removed them by mistake. I forced-push this branch to revert those changes

src/io.cpp

spnda · 2025-07-01T20:19:49Z

The changes to Expected<T> are the ones that weren't actually producing warnings and I had removed them by mistake. I forced-push this branch to revert those changes

All of the changes apart from the unused parameter warning fixes are for functions that return Expected<T>. They might not be templates, and have a defined type, but I disagree with GCC and think they should be explicitly moved to avoid any future issues where objects are accidentally copied. The copy constructor is, at the end of the day, the default, so I think it should be made explicit that we want to move here. In all other cases this would be required, apart from here.

include/fastgltf/core.hpp

src/io.cpp

AndreCostaaa · 2025-07-01T20:34:16Z

For member templated variables I completely agree!

What I'm modifying is cases where we return a local variable and in that case it doesn't make sense to be explicit about the move since we would prevent the compiler from performing RVO (thank you for pointing me to the correct term)

spnda

Given that you've changed all standard factory constructors of GltfDataBuffer, you can also change the one guarded by FASTGLTF_CPP_20, which would have the same issue.

fastgltf/include/fastgltf/core.hpp

Lines 639 to 647 in ee55a81

    
           #if FASTGLTF_CPP_20 
        
           		static Expected<GltfDataBuffer> FromSpan(std::span<std::byte> data) noexcept { 
        
           			GltfDataBuffer buffer(data); 
        
           			if (buffer.buffer.get() == nullptr) { 
        
           				return buffer.error; 
        
           			} 
        
           			return std::move(buffer); 
        
           		} 
        
           #endif

Also, since we've established the warnings to be correct, what about the templated functions? Could those be re-added?

AndreCostaaa · 2025-07-03T10:07:37Z

Given that you've changed all standard factory constructors of GltfDataBuffer, you can also change the one guarded by FASTGLTF_CPP_20, which would have the same issue.

Done, there's also one guarded by __ANDROID__ that should have the same problem but since I can't test i would rather not touch it

Also, since we've established the warnings to be correct, what about the templated functions? Could those be re-added?

The only std::move left in core.hpp (besides the one guarded by __ANDROID__) is this one:

		[[nodiscard]] T&& operator*() && noexcept {
			assert(err == Error::None);
			return std::move(value);
		}

Which is moving a member variable so I believe this is necessary as the default here would be to copy to not invalidate a member variable

There's some in types.hpp but also return member variables like this one so I also believe it's correct to use std::move in those cases

spnda · 2025-07-03T10:11:19Z

Done, there's also one guarded by __ANDROID__ that should have the same problem but since I can't test i would rather not touch it

If it's a local variable you can just change it, it ought to work.

The only std::move left in core.hpp (besides the one guarded by __ANDROID__) is this one:
		[[nodiscard]] T&& operator*() && noexcept {
			assert(err == Error::None);
			return std::move(value);
		}
Which is moving a member variable so I believe this is necessary as the default here would be to copy to not invalidate a member variable

There's some in types.hpp but also return member variables like this one so I also believe it's correct to use std::move in those cases

Yea for member variables this is still necessary, as far as I understood only local variables are promoted to "xvalues" that are moved automatically.

AndreCostaaa · 2025-07-03T10:15:48Z

If it's a local variable you can just change it, it ought to work.

Sure thing, it's done :)

AndreCostaaa · 2025-07-03T10:20:41Z

Thank you very much 🚀

spnda · 2025-07-03T14:48:48Z

@AndreCostaaa May I ask if this is for LVGL or for one of your own projects? I'm currently doing embedded stuff with Zephyr so it's nice to see this project making its way into embedded environments, too, if that's the case.

AndreCostaaa · 2025-07-03T14:56:26Z

Hi @spnda

This is indeed for LVGL, we've recently added 3D support and we are working on adding GLTF support :)

AndreCostaaa added a commit to AndreCostaaa/lv_gltf_viewer that referenced this pull request Jul 1, 2025

build: use downstream version from fastgltf to avoid build warnings

497c896

Pending spnda/fastgltf#114 merge

AndreCostaaa closed this Jul 1, 2025

AndreCostaaa force-pushed the fix-warnings branch from a0a63b3 to 06d47e8 Compare July 1, 2025 19:18

AndreCostaaa reopened this Jul 1, 2025

spnda requested changes Jul 1, 2025

View reviewed changes

src/io.cpp Outdated Show resolved Hide resolved

src/io.cpp Outdated Show resolved Hide resolved

AndreCostaaa commented Jul 1, 2025

View reviewed changes

include/fastgltf/core.hpp Show resolved Hide resolved

src/io.cpp Outdated Show resolved Hide resolved

AndreCostaaa force-pushed the fix-warnings branch from 64ff538 to ee55a81 Compare July 3, 2025 09:36

AndreCostaaa requested a review from spnda July 3, 2025 09:36

spnda requested changes Jul 3, 2025

View reviewed changes

AndreCostaaa force-pushed the fix-warnings branch from ee55a81 to 9122526 Compare July 3, 2025 09:57

AndreCostaaa added 2 commits July 3, 2025 12:14

fix: remove redudant moves to avoid warnings

57696f6

fix: unused parameter warnings

9ca1dbb

AndreCostaaa force-pushed the fix-warnings branch from 9122526 to 9ca1dbb Compare July 3, 2025 10:14

spnda approved these changes Jul 3, 2025

View reviewed changes

spnda merged commit 4e22613 into spnda:main Jul 3, 2025

AndreCostaaa deleted the fix-warnings branch July 3, 2025 10:17

spnda added the enhancement New feature or request label Oct 16, 2025

	#if FASTGLTF_CPP_20
	static Expected<GltfDataBuffer> FromSpan(std::span<std::byte> data) noexcept {
	GltfDataBuffer buffer(data);
	if (buffer.buffer.get() == nullptr) {
	return buffer.error;
	}
	return std::move(buffer);
	}
	#endif

fix: redudant moves and unused variables warnings #114

fix: redudant moves and unused variables warnings #114

Uh oh!

Conversation

AndreCostaaa commented Jul 1, 2025

Uh oh!

spnda commented Jul 1, 2025

Uh oh!

AndreCostaaa commented Jul 1, 2025

Uh oh!

spnda commented Jul 1, 2025

Uh oh!

AndreCostaaa commented Jul 1, 2025

Uh oh!

Uh oh!

Uh oh!

spnda commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndreCostaaa commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spnda left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreCostaaa commented Jul 3, 2025

Uh oh!

spnda commented Jul 3, 2025

Uh oh!

AndreCostaaa commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndreCostaaa commented Jul 3, 2025

Uh oh!

spnda commented Jul 3, 2025

Uh oh!

AndreCostaaa commented Jul 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spnda commented Jul 1, 2025 •

edited

Loading

AndreCostaaa commented Jul 1, 2025 •

edited

Loading

spnda left a comment •

edited

Loading

AndreCostaaa commented Jul 3, 2025 •

edited

Loading