Skip to content
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.

Sign up for GitHub

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

Bump to C++17 for std::filesystem #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bump to C++17 for std::filesystem #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c284ce3
cmake: use C++17
franklange Mar 24, 2021
178498a
audiostream: use std::filesystem instead of boost
franklange Mar 24, 2021
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
3 changes: 1 addition & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ else()
find_package(NIBoost REQUIRED)
add_library(Boost::boost ALIAS boost_header_only_interface)
add_library(Boost::iostreams ALIAS boost_iostreams)
add_library(Boost::filesystem ALIAS boost_filesystem)
add_library(Boost::system ALIAS boost_system)
add_library(Boost::program_options ALIAS boost_program_options)

Expand Down Expand Up @@ -104,7 +103,7 @@ endif()
foreach( target audiostream audiostream_test pcm_test generator )
if( TARGET ${target} )
if( NIMEDIA_OPEN_SOURCE )
set_property( TARGET ${target} PROPERTY CXX_STANDARD 14)
set_property( TARGET ${target} PROPERTY CXX_STANDARD 17)
if( NIMEDIA_TREAT_WARNINGS_AS_ERRORS )
ni_treat_warnings_as_errors( ${target} )
endif()
Expand Down
2 changes: 1 addition & 1 deletion audiostream/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ target_include_directories ( audiostream
)

target_link_libraries ( audiostream PUBLIC pcm
PRIVATE Boost::iostreams Boost::filesystem Boost::system ${codec_libraries})
PRIVATE Boost::iostreams Boost::system ${codec_libraries})

if(MSVC)
target_compile_options( audiostream PRIVATE /wd4351 ) # MSVC new behaviour warning
Expand Down
6 changes: 3 additions & 3 deletions audiostream/doc/ifstream.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ The constructor will throw a runtime error if the file format is not supported o
Example code:

```cpp
void analyze_file( const std::string& filename )
void analyze_file( const std::filesystem::path& filename )
{
audio::ifstream stream;
try
Expand All @@ -36,7 +36,7 @@ void analyze_file( const std::string& filename )
You can also allow only a specific container:

```cpp
void analyze_wav_file( const std::string& filename )
void analyze_wav_file( const std::filesystem::path& filename )
{
audio::ifstream stream;
try
Expand All @@ -58,7 +58,7 @@ An `audio::ifstream` also supports multi stream containers, which allows us to s
By default, an `audio::ifstream` will open the Stems Master Stream, but is also capable of opening any of the other streams by simply passing the desired stream index as argument to the constructor:

```cpp
void analyze_stem_stream( const std::string& filename, size_t stream_index )
void analyze_stem_stream( const std::filesystem::path& filename, size_t stream_index )
{
audio::ifstream stream;
try
Expand Down
4 changes: 3 additions & 1 deletion audiostream/inc/ni/media/audio/aiff/aiff_ofstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include <ni/media/audio/aiff/aiff_ofstream_info.h>
#include <ni/media/audio/ofstream.h>

#include <filesystem>

namespace audio
{

Expand All @@ -38,7 +40,7 @@ class aiff_ofstream : public ofstream
aiff_ofstream( aiff_ofstream&& );
aiff_ofstream& operator=( aiff_ofstream&& );

aiff_ofstream( const std::string& file, const info_type& info );
aiff_ofstream( const std::filesystem::path& file, const info_type& info );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can break existing code. If a user passes an argument of a type that can implicitly convert to std::string, it might not work if the signature now expects a std::filesystem::path. See https://gcc.godbolt.org/z/49x1vKMEq.

How about overloading instead of replacing?

Copy link
Author

@franklange franklange Mar 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing this up! I didn't consider this tbh. Though I'm slightly more okay with this being a breaking change but that's of course not up to me :)

Would it be acceptable if users that are affected by this adjust like this?
https://gcc.godbolt.org/z/KaTca6o19

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for overloading, not everybody is ready to use std::filesystem, yet. Imho it should mirror the ctors of: https://en.cppreference.com/w/cpp/io/basic_fstream/basic_fstream
In order to fix the bug, it is important that you can pass in a std::wstring instead of a std::string on Windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heyhey, thanks for the feedback!

In order to fix the bug, it is important that you can pass in a std::wstring instead of a std::string on Windows.

Not sure I agree with the overload though because isn't this exactly what fs::path is trying to solve?

But in any case, just to make sure I understand what you guys are suggesting, it would be something like this?
https://godbolt.org/z/86cPTd95o

In other words: platform macro for string/wstring and additionally a template overload for path because naive overloading doesn't work?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is ultimately up to the maintainers, but here's my opinion: I think having a constructor for a std fs path is a good idea. I would try to avoid breaking changes because they might annoy users. In your approach, the point of the template is so that when passing a const char*, the compiler does not consider the fs path ctor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I basically took this from the mentioned fstream overload implementation:
fstream string + path overload

I think in this particular case I disagree with trying to avoid a "breaking" change because:
a) somehow I don't see how it makes sense from a library's perspective to offer a string and a path ctor other than for backwards compatibility reasons but maybe I'm still missing something and
b) it only affects those clients that defined an implicit conversion operator to string for their type and all they'd have to do to fix this would be to change the operator to fs::path. It's actually sort of weird that the compiler can't figure out the transitivity on its own from type -> string -> lib(path) and just needs some slight help here.

But ya, I can only open the PR :)


const info_type& info() const override;
};
Expand Down
5 changes: 3 additions & 2 deletions audiostream/inc/ni/media/audio/ifstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <ni/media/audio/ifstream_info.h>
#include <ni/media/audio/istream.h>

#include <filesystem>
#include <string>

namespace audio
Expand All @@ -37,8 +38,8 @@ class ifstream : public istream

ifstream();

ifstream( const std::string& file );
ifstream( const std::string& file, info_type::container_type container, size_t stream_index = 0 );
ifstream( const std::filesystem::path& file );
ifstream( const std::filesystem::path& file, info_type::container_type container, size_t stream_index = 0 );

ifstream( ifstream&& );
ifstream& operator=( ifstream&& );
Expand Down
6 changes: 3 additions & 3 deletions audiostream/inc/ni/media/audio/ifstream_support.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@

#include <ni/media/audio/ifstream_info.h>

#include <filesystem>
#include <map>
#include <string>

namespace audio
{

// a map of extension => container type
using ifstream_container_map = std::map<std::string, ifstream_info::container_type>;
using ifstream_container_map = std::map<std::filesystem::path, ifstream_info::container_type>;
Copy link
Author

@franklange franklange Mar 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I wasn't sure if this might be overkill but motivation was that extensions are also paths.

auto ifstream_supported_formats() -> const ifstream_container_map&;

} // namespace audio


Expand Down
4 changes: 3 additions & 1 deletion audiostream/inc/ni/media/audio/wav/wav_ofstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include <ni/media/audio/ofstream.h>
#include <ni/media/audio/wav/wav_ofstream_info.h>

#include <filesystem>

namespace audio
{

Expand All @@ -39,7 +41,7 @@ class wav_ofstream : public ofstream
wav_ofstream( wav_ofstream&& );
wav_ofstream& operator=( wav_ofstream&& );

wav_ofstream( const std::string& file, const info_type& info );
wav_ofstream( const std::filesystem::path& file, const info_type& info );

const info_type& info() const override;
};
Expand Down
6 changes: 3 additions & 3 deletions audiostream/src/ni/media/audio/aiff/aiff_file_sink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@

#include <boost/iostreams/device/file_descriptor.hpp>

#include <boost/filesystem/path.hpp>
#include <filesystem>

class aiff_file_sink : public aiff_sink<boost::iostreams::file_descriptor_sink>
{
using base_type = aiff_sink<boost::iostreams::file_descriptor_sink>;

public:
explicit aiff_file_sink( const info_type& info, const std::string& path )
: base_type( info, boost::filesystem::path( path ), BOOST_IOS::binary | BOOST_IOS::out )
explicit aiff_file_sink( const info_type& info, const std::filesystem::path& path )
: base_type( info, path, BOOST_IOS::binary | BOOST_IOS::out )
{
}
};
6 changes: 3 additions & 3 deletions audiostream/src/ni/media/audio/aiff/aiff_file_source.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@

#include <boost/iostreams/device/file_descriptor.hpp>

#include <boost/filesystem/path.hpp>
#include <filesystem>

class aiff_file_source : public aiff_source<boost::iostreams::file_descriptor_source>
{
using base_type = aiff_source<boost::iostreams::file_descriptor_source>;

public:
explicit aiff_file_source( const std::string& path )
: base_type( boost::filesystem::path( path ), BOOST_IOS::binary | BOOST_IOS::in )
explicit aiff_file_source( const std::filesystem::path& path )
: base_type( path, BOOST_IOS::binary | BOOST_IOS::in )
{
}
};
2 changes: 1 addition & 1 deletion audiostream/src/ni/media/audio/aiff/aiff_ofstream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ aiff_ofstream& aiff_ofstream::operator=( aiff_ofstream&& other )

//----------------------------------------------------------------------------------------------------------------------

aiff_ofstream::aiff_ofstream( const std::string& file, const info_type& info )
aiff_ofstream::aiff_ofstream( const std::filesystem::path& file, const info_type& info )
: ofstream( make_stream_buffer( aiff_file_sink( info, file ) ), make_info( info ) )
{
}
Expand Down
7 changes: 3 additions & 4 deletions audiostream/src/ni/media/audio/flac/flac_file_source.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,15 @@

#include <boost/iostreams/device/file_descriptor.hpp>

#include <boost/filesystem/path.hpp>
#include <filesystem>

class flac_file_source : public source_impl<flac_source<boost::iostreams::file_descriptor_source>>
{
using base_type = source_impl<flac_source<boost::iostreams::file_descriptor_source>>;

public:
explicit flac_file_source( const std::string& path )
: base_type(
boost::iostreams::file_descriptor_source( boost::filesystem::path( path ), BOOST_IOS::binary | BOOST_IOS::in ) )
explicit flac_file_source( const std::filesystem::path& path )
: base_type( boost::iostreams::file_descriptor_source( path, BOOST_IOS::binary | BOOST_IOS::in ) )
{
}
};
6 changes: 3 additions & 3 deletions audiostream/src/ni/media/audio/ifstream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ ifstream::ifstream( std::unique_ptr<streambuf> sb, std::unique_ptr<info_type> in

//----------------------------------------------------------------------------------------------------------------------

ifstream::ifstream( const std::string& file )
ifstream::ifstream( const std::filesystem::path& file )
: ifstream()
{
#if NIMEDIA_ENABLE_ITUNES_DECODING
if ( is_itunes_url( file ) )
{
auto source = avassetreader_source( file, 0 );
auto source = avassetreader_source( file.string(), 0 );
auto info = source.info();
*this = ifstream( make_stream_buffer( std::move( source ) ), std::make_unique<info_type>( std::move( info ) ) );
return;
Expand All @@ -69,7 +69,7 @@ ifstream::ifstream( const std::string& file )

//----------------------------------------------------------------------------------------------------------------------

ifstream::ifstream( const std::string& file, ifstream_info::container_type container, size_t stream_index )
ifstream::ifstream( const std::filesystem::path& file, ifstream_info::container_type container, size_t stream_index )
: ifstream()
{
using container_type = ifstream_info::container_type;
Expand Down
26 changes: 12 additions & 14 deletions audiostream/src/ni/media/audio/iotools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include <boost/algorithm/string/predicate.hpp>
#include <boost/predef.h>

#include <boost/filesystem.hpp>
#include <map>

namespace audio
Expand Down Expand Up @@ -97,13 +96,13 @@ auto ofstream_map() -> const std::map<std::string, ofstream_info::container_type
//----------------------------------------------------------------------------------------------------------------------

template <class Map>
auto container_of( const std::string& url, const Map& map ) -> boost::optional<typename Map::mapped_type>
auto container_of( const std::filesystem::path& url, const Map& map ) -> boost::optional<typename Map::mapped_type>
{
auto extension = extension_from_url( url );
const auto extension = extension_from_url( url );
if ( extension.empty() )
return {};

auto it = map.find( boost::to_lower_copy( extension ) );
const auto it = map.find( boost::to_lower_copy( extension.string() ) );
if ( it == map.end() )
return {};

Expand All @@ -114,15 +113,15 @@ auto container_of( const std::string& url, const Map& map ) -> boost::optional<t

//----------------------------------------------------------------------------------------------------------------------

bool is_itunes_url( const std::string& url )
bool is_itunes_url( const std::filesystem::path& url )
{
return boost::starts_with( url, "ipod-library://" );
return boost::starts_with( url.string(), "ipod-library://" );
}

//----------------------------------------------------------------------------------------------------------------------


std::string extension_from_url( const std::string& url )
std::filesystem::path extension_from_url( const std::filesystem::path& url )
{
#if NIMEDIA_ENABLE_ITUNES_DECODING
if ( is_itunes_url( url ) )
Expand All @@ -132,38 +131,37 @@ std::string extension_from_url( const std::string& url )
else
#endif
{
auto path = boost::filesystem::path( url );
if ( is_regular_file( path ) && path.has_extension() )
return path.extension().string();
if ( std::filesystem::is_regular_file( url ) && url.has_extension() )
return url.extension();
}
return {};
}

//----------------------------------------------------------------------------------------------------------------------

auto ifstream_container( const std::string& url ) -> boost::optional<ifstream_info::container_type>
auto ifstream_container( const std::filesystem::path& url ) -> boost::optional<ifstream_info::container_type>
{
return container_of( url, ifstream_supported_formats() );
}


//----------------------------------------------------------------------------------------------------------------------

auto ofstream_container( const std::string& url ) -> boost::optional<ofstream_info::container_type>
auto ofstream_container( const std::filesystem::path& url ) -> boost::optional<ofstream_info::container_type>
{
return container_of( url, ofstream_map() );
}

//----------------------------------------------------------------------------------------------------------------------

bool can_read_file( const std::string& url )
bool can_read_file( const std::filesystem::path& url )
{
return ifstream_container( url ) != boost::none;
}

//----------------------------------------------------------------------------------------------------------------------

bool can_read_file( const std::string& url, std::set<ifstream_info::container_type> supported_containers )
bool can_read_file( const std::filesystem::path& url, std::set<ifstream_info::container_type> supported_containers )
{
if ( auto container = ifstream_container( url ) )
return supported_containers.find( *container ) != supported_containers.end();
Expand Down
17 changes: 9 additions & 8 deletions audiostream/src/ni/media/audio/iotools.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,22 @@

#include <boost/optional.hpp>

#include <filesystem>
#include <set>
#include <string>

namespace audio
{

auto ifstream_container( const std::string& url ) -> boost::optional<ifstream_info::container_type>;
auto ofstream_container( const std::string& url ) -> boost::optional<ofstream_info::container_type>;
auto ifstream_container( const std::filesystem::path& url ) -> boost::optional<ifstream_info::container_type>;
auto ofstream_container( const std::filesystem::path& url ) -> boost::optional<ofstream_info::container_type>;

auto is_itunes_url( const std::string& url ) -> bool;
auto extension_from_url( const std::string& url ) -> std::string;
auto is_itunes_url( const std::filesystem::path& url ) -> bool;
auto extension_from_url( const std::filesystem::path& url ) -> std::filesystem::path;

bool can_read_file( const std::string& url );
bool can_read_file( const std::string& url, std::set<ifstream_info::container_type> supported_containers );
bool can_read_file( const std::filesystem::path& url );
bool can_read_file( const std::filesystem::path& url, std::set<ifstream_info::container_type> supported_containers );

bool can_write_file( const std::string& url );
bool can_write_file( const std::string& url, std::set<ofstream_info::container_type> supported_containers );
bool can_write_file( const std::filesystem::path& url );
bool can_write_file( const std::filesystem::path& url, std::set<ofstream_info::container_type> supported_containers );
}
9 changes: 4 additions & 5 deletions audiostream/src/ni/media/audio/mp3/mp3_file_source.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,16 @@

#include <boost/iostreams/device/file_descriptor.hpp> // basic_file

#include <boost/filesystem/path.hpp>
#include <filesystem>

class mp3_file_source : public os_source<boost::iostreams::file_descriptor_source>
{
using base_type = os_source<boost::iostreams::file_descriptor_source>;

public:
explicit mp3_file_source( const std::string& path )
: base_type(
boost::iostreams::file_descriptor_source( boost::filesystem::path( path ), BOOST_IOS::binary | BOOST_IOS::in ),
audio::ifstream_info::container_type::mp3 )
explicit mp3_file_source( const std::filesystem::path& path )
: base_type( boost::iostreams::file_descriptor_source( path, BOOST_IOS::binary | BOOST_IOS::in ),
audio::ifstream_info::container_type::mp3 )
{
}
};
11 changes: 5 additions & 6 deletions audiostream/src/ni/media/audio/mp4/mp4_file_source.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,17 @@

#include <boost/iostreams/device/file_descriptor.hpp>

#include <boost/filesystem/path.hpp>
#include <filesystem>

class mp4_file_source : public os_source<boost::iostreams::file_descriptor_source>
{
using base_type = os_source<boost::iostreams::file_descriptor_source>;

public:
explicit mp4_file_source( const std::string& path, size_t stream = 0 )
: base_type(
boost::iostreams::file_descriptor_source( boost::filesystem::path( path ), BOOST_IOS::binary | BOOST_IOS::in ),
audio::ifstream_info::container_type::mp4,
stream )
explicit mp4_file_source( const std::filesystem::path& path, size_t stream = 0 )
: base_type( boost::iostreams::file_descriptor_source( path, BOOST_IOS::binary | BOOST_IOS::in ),
audio::ifstream_info::container_type::mp4,
stream )
{
}
};
Loading