Skip to content

Commit

Permalink
ShaderTools: port away from std::pair.
Browse files Browse the repository at this point in the history
Here the benefit is especially clear -- as Containers::Pair is trivially
copyable with trivial types, all growable arrays can make use of
std::realloc() while with the STL variant a silly constructor, copy
constructor, destructor had to be used.

Additionally, we no longer need to take explicit care of libc++ and MSVC
STL where returning a std::pair<bool, Containers::String> as

    return {{}, Containers::String{..., <deleter>}};

would caused an unnecessary copy instead of a move, losing the custom
deleter in the process. Yay!

There's a <Corrade/Containers/PairStl.h> include for backwards
compatibility purposes, but obviously it would only work for the return
type of validate*() and cases where an initializer list was passed to a
list-of-pairs-taking functions, and not a concretely typed ArrayView.
Those functions were though mostly the linker API which isn't
implemented by any plugin yet, so it shouldn't be *that* breaking to
users. Neverteless, I'm trying to do this breaking change rather sooner
than later to prevent pain further down the road when the Vulkan APIs
and SPIR-V pipeline gets widely used.
  • Loading branch information
mosra committed Apr 5, 2022
1 parent 8f311ad commit 7f4500d
Show file tree
Hide file tree
Showing 8 changed files with 226 additions and 229 deletions.
14 changes: 7 additions & 7 deletions doc/snippets/MagnumShaderTools.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <unordered_map>
#include <Corrade/Containers/Array.h>
#include <Corrade/Containers/Optional.h>
#include <Corrade/Containers/Pair.h>
#include <Corrade/Containers/String.h>
#include <Corrade/Containers/StringStl.h> /** @todo drop when file callbacks are <string>-free */
#include <Corrade/Utility/Macros.h> /* CORRADE_LINE_STRING */
Expand All @@ -47,14 +48,13 @@ PluginManager::Manager<ShaderTools::AbstractConverter> manager;
Containers::Pointer<ShaderTools::AbstractConverter> converter =
manager.loadAndInstantiate("AnyShaderConverter");

bool valid;
Containers::String message;
if(converter) std::tie(valid, message) =
Containers::Pair<bool, Containers::String> validMessage;
if(converter) validMessage =
converter->validateFile(ShaderTools::Stage::Unspecified, "file.spv");
if(!converter || !valid)
Error{} << "Validation failed:" << message;
else if(!message.isEmpty())
Warning{} << "Validation succeeded with warnings:" << message;
if(!converter || !validMessage.first())
Error{} << "Validation failed:" << validMessage.second();
else if(!validMessage.second().isEmpty())
Warning{} << "Validation succeeded with warnings:" << validMessage.second();
else
Debug{} << "Validation passed";
/* [AbstractConverter-usage-validation] */
Expand Down
85 changes: 43 additions & 42 deletions src/Magnum/ShaderTools/AbstractConverter.cpp

Large diffs are not rendered by default.

45 changes: 26 additions & 19 deletions src/Magnum/ShaderTools/AbstractConverter.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@
/* For *ToData() APIs that used to return just an Array before */
#include <Corrade/Containers/Optional.h>

/* For validate*() that used to return a std::pair. Similar case is with
setDefinitions(), there a hope is that a std::initializer_list was used and
thus the type change won't break much code; lastly the link APIs that used
std::pair heavily are not implemented by any plugin yet so changing them
should be still fine. */
#include <Corrade/Containers/PairStl.h>

#include "Magnum/ShaderTools/Stage.h"
#endif

Expand Down Expand Up @@ -720,10 +727,10 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* in @ref magnum-shaderconverter "magnum-shaderconverter".
* @see @ref ConverterFlag::PreprocessOnly
*/
void setDefinitions(Containers::ArrayView<const std::pair<Containers::StringView, Containers::StringView>> definitions);
void setDefinitions(Containers::ArrayView<const Containers::Pair<Containers::StringView, Containers::StringView>> definitions);

/** @overload */
void setDefinitions(std::initializer_list<std::pair<Containers::StringView, Containers::StringView>> definitions);
void setDefinitions(std::initializer_list<Containers::Pair<Containers::StringView, Containers::StringView>> definitions);

/**
* @brief Set optimization level
Expand Down Expand Up @@ -778,7 +785,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
*
* @see @ref features(), @ref validateFile()
*/
std::pair<bool, Containers::String> validateData(Stage stage, Containers::ArrayView<const void> data);
Containers::Pair<bool, Containers::String> validateData(Stage stage, Containers::ArrayView<const void> data);

/**
* @brief Validate a shader
Expand All @@ -799,7 +806,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* @ref magnum-shaderconverter "magnum-shaderconverter".
* @see @ref features(), @ref validateData()
*/
std::pair<bool, Containers::String> validateFile(Stage stage, Containers::StringView filename);
Containers::Pair<bool, Containers::String> validateFile(Stage stage, Containers::StringView filename);

/**
* @brief Convert shader data to a data
Expand Down Expand Up @@ -874,15 +881,15 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
#else
Implementation::OptionalButAlsoArray<char>
#endif
linkDataToData(Containers::ArrayView<const std::pair<Stage, Containers::ArrayView<const void>>> data);
linkDataToData(Containers::ArrayView<const Containers::Pair<Stage, Containers::ArrayView<const void>>> data);

/** @overload */
#if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT)
Containers::Optional<Containers::Array<char>>
#else
Implementation::OptionalButAlsoArray<char>
#endif
linkDataToData(std::initializer_list<std::pair<Stage, Containers::ArrayView<const void>>> data);
linkDataToData(std::initializer_list<Containers::Pair<Stage, Containers::ArrayView<const void>>> data);

/**
* @brief Link shader data together to a file
Expand All @@ -895,10 +902,10 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* @see @ref features(), @ref linkFilesToFile(),
* @ref linkFilesToData(), @ref linkDataToData()
*/
bool linkDataToFile(Containers::ArrayView<const std::pair<Stage, Containers::ArrayView<const void>>> data, Containers::StringView filename);
bool linkDataToFile(Containers::ArrayView<const Containers::Pair<Stage, Containers::ArrayView<const void>>> data, Containers::StringView filename);

/** @overload */
bool linkDataToFile(std::initializer_list<std::pair<Stage, Containers::ArrayView<const void>>> data, Containers::StringView filename);
bool linkDataToFile(std::initializer_list<Containers::Pair<Stage, Containers::ArrayView<const void>>> data, Containers::StringView filename);

/**
* @brief Link shader files together to a file
Expand All @@ -914,10 +921,10 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* @see @ref features(), @ref linkFilesToData(), @ref linkDataToFile(),
* @ref linkDataToData()
*/
bool linkFilesToFile(Containers::ArrayView<const std::pair<Stage, Containers::StringView>> from, Containers::StringView to);
bool linkFilesToFile(Containers::ArrayView<const Containers::Pair<Stage, Containers::StringView>> from, Containers::StringView to);

/** @overload */
bool linkFilesToFile(std::initializer_list<std::pair<Stage, Containers::StringView>> from, Containers::StringView to);
bool linkFilesToFile(std::initializer_list<Containers::Pair<Stage, Containers::StringView>> from, Containers::StringView to);

/**
* @brief Link shader files together to a data
Expand All @@ -935,15 +942,15 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
#else
Implementation::OptionalButAlsoArray<char>
#endif
linkFilesToData(Containers::ArrayView<const std::pair<Stage, Containers::StringView>> filenames);
linkFilesToData(Containers::ArrayView<const Containers::Pair<Stage, Containers::StringView>> filenames);

/** @overload */
#if !defined(MAGNUM_BUILD_DEPRECATED) || defined(DOXYGEN_GENERATING_OUTPUT)
Containers::Optional<Containers::Array<char>>
#else
Implementation::OptionalButAlsoArray<char>
#endif
linkFilesToData(std::initializer_list<std::pair<Stage, Containers::StringView>> filenames);
linkFilesToData(std::initializer_list<Containers::Pair<Stage, Containers::StringView>> filenames);

protected:
/**
Expand All @@ -961,7 +968,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* is not supported --- instead, file is loaded though the callback and
* data passed through to @ref doValidateData().
*/
virtual std::pair<bool, Containers::String> doValidateFile(Stage stage, Containers::StringView filename);
virtual Containers::Pair<bool, Containers::String> doValidateFile(Stage stage, Containers::StringView filename);

/**
* @brief Implementation for @ref convertFileToFile()
Expand Down Expand Up @@ -1012,7 +1019,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* is not supported --- instead, file is loaded though the callback and
* data passed through to @ref doLinkDataToData().
*/
virtual bool doLinkFilesToFile(Containers::ArrayView<const std::pair<Stage, Containers::StringView>> from, Containers::StringView to);
virtual bool doLinkFilesToFile(Containers::ArrayView<const Containers::Pair<Stage, Containers::StringView>> from, Containers::StringView to);

/**
* @brief Implementation for @ref linkFilesToData()
Expand All @@ -1029,7 +1036,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* is not supported --- instead, file is loaded though the callback and
* data passed through to @ref doConvertDataToData().
*/
virtual Containers::Optional<Containers::Array<char>> doLinkFilesToData(Containers::ArrayView<const std::pair<Stage, Containers::StringView>> filenames);
virtual Containers::Optional<Containers::Array<char>> doLinkFilesToData(Containers::ArrayView<const Containers::Pair<Stage, Containers::StringView>> filenames);

private:
/**
Expand Down Expand Up @@ -1100,7 +1107,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* Has to be implemented if @ref ConverterFeature::Preprocess is
* supported. This function isn't expected to fail.
*/
virtual void doSetDefinitions(Containers::ArrayView<const std::pair<Containers::StringView, Containers::StringView>> definitions);
virtual void doSetDefinitions(Containers::ArrayView<const Containers::Pair<Containers::StringView, Containers::StringView>> definitions);

/**
* @brief Implementation for @ref setOptimizationLevel()
Expand Down Expand Up @@ -1136,7 +1143,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* order to accept any type, this function gets it cast to
* @cpp char @ce for more convenience.
*/
virtual std::pair<bool, Containers::String> doValidateData(Stage stage, Containers::ArrayView<const char> data);
virtual Containers::Pair<bool, Containers::String> doValidateData(Stage stage, Containers::ArrayView<const char> data);

/* Used by convertFileToFile(), doConvertFileToFile(),
convertFileToData() and doConvertFileToData() */
Expand All @@ -1154,7 +1161,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac

/* Used by linkFilesToFile(), doLinkFilesToFile(), linkFilesToData()
and doLinkFilesToData() */
MAGNUM_SHADERTOOLS_LOCAL Containers::Optional<Containers::Array<char>> linkDataToDataUsingInputFileCallbacks(const char* prefix, Containers::ArrayView<const std::pair<Stage, Containers::StringView>> filenames);
MAGNUM_SHADERTOOLS_LOCAL Containers::Optional<Containers::Array<char>> linkDataToDataUsingInputFileCallbacks(const char* prefix, Containers::ArrayView<const Containers::Pair<Stage, Containers::StringView>> filenames);

/**
* @brief Implementation for @ref linkDataToData()
Expand All @@ -1164,7 +1171,7 @@ class MAGNUM_SHADERTOOLS_EXPORT AbstractConverter: public PluginManager::Abstrac
* order to accept any type, this function gets it cast to
* @cpp char @ce for more convenience.
*/
virtual Containers::Optional<Containers::Array<char>> doLinkDataToData(Containers::ArrayView<const std::pair<Stage, Containers::ArrayView<const char>>> data);
virtual Containers::Optional<Containers::Array<char>> doLinkDataToData(Containers::ArrayView<const Containers::Pair<Stage, Containers::ArrayView<const char>>> data);

ConverterFlags _flags;

Expand Down
Loading

0 comments on commit 7f4500d

Please sign in to comment.