PickNikRobotics / data_tamer

C++ library for Fearless Timeseries Logging
MIT License
219 stars 21 forks source link

Fix compile error #23

Closed danmou closed 2 months ago

danmou commented 2 months ago

With master I get the following compile error (using CMake 3.30.2 and GCC 14.2.1 on Linux):

$ cmake --build build/Debug --parallel
[  5%] Building CXX object CMakeFiles/data_tamer.dir/src/sinks/mcap_sink.cpp.o
[ 10%] Building CXX object CMakeFiles/data_tamer.dir/src/data_tamer.cpp.o
[ 15%] Building CXX object CMakeFiles/data_tamer.dir/src/channel.cpp.o
[ 20%] Building CXX object CMakeFiles/data_tamer.dir/src/data_sink.cpp.o
[ 25%] Building CXX object CMakeFiles/data_tamer.dir/src/types.cpp.o
In file included from /home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/logged_value.hpp:4,
                 from /home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/channel.hpp:6,
                 from /home/daniel/git/data_tamer/data_tamer_cpp/src/channel.cpp:1:
/home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/details/locked_reference.hpp: In member function ‘DataTamer::LockedPtr<T>& DataTamer::LockedPtr<T>::operator=(DataTamer::LockedPtr<T>&&)’:
/home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/details/locked_reference.hpp:60:12: error: cannot convert ‘DataTamer::Mutex**’ to ‘DataTamer::Mutex*’ in assignment
   60 |   mutex_ = &other.mutex_;
      |            ^~~~~~~~~~~~~
      |            |
      |            DataTamer::Mutex**
In file included from /home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/logged_value.hpp:4,
                 from /home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/channel.hpp:6,
                 from /home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/data_tamer.hpp:3,
                 from /home/daniel/git/data_tamer/data_tamer_cpp/src/data_tamer.cpp:1:
/home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/details/locked_reference.hpp: In member function ‘DataTamer::LockedPtr<T>& DataTamer::LockedPtr<T>::operator=(DataTamer::LockedPtr<T>&&)’:
/home/daniel/git/data_tamer/data_tamer_cpp/include/data_tamer/details/locked_reference.hpp:60:12: error: cannot convert ‘DataTamer::Mutex**’ to ‘DataTamer::Mutex*’ in assignment
   60 |   mutex_ = &other.mutex_;
      |            ^~~~~~~~~~~~~
      |            |
      |            DataTamer::Mutex**
make[2]: *** [CMakeFiles/data_tamer.dir/build.make:90: CMakeFiles/data_tamer.dir/src/data_tamer.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [CMakeFiles/data_tamer.dir/build.make:76: CMakeFiles/data_tamer.dir/src/channel.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:154: CMakeFiles/data_tamer.dir/all] Error 2
make: *** [Makefile:146: all] Error 2

It seems the issue was introduced in #18. I'm a bit confused how it wasn't discovered in all this time, doesn't seem like it should be compiler-dependent? Anyway the fix was simple.

henrygerardmoore commented 2 months ago

Your change is definitely correct. I think the reason it wasn't caught is that nothing uses LockedPtr<T>::operator= so it just isn't built in the code-base as-is (AKA not covered by tests). Probably your new version of gcc catches this anyway (or you were using that operator= assignment so it tried and failed to build it). I'm using 13.2.0, which doesn't catch this. Thank you for the change! I'm going to add a usage of the LockedPtr to an example and will have a PR with that shortly.

henrygerardmoore commented 2 months ago

I created #24 to address the lack of tests for this. It still doesn't test the move constructor, but I'm not sure of the use case of that anyway. To test your branch, I added a usage of the move constructor for locked pointer, but it was a contrived usage just to get it to build that function. Do you have a better usage example? If so, it would be great if you could comment it on #24 or just create a separate PR with an example or test utilizing it.

danmou commented 2 months ago

Thanks, that makes sense! I'm new to this library so I'm not aware of a use case for that move constructor (but presumably no one has used it so far, or they would have encountered the bug).