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.

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

VideoCommon/ShaderGenCommon: Convert helper functions over to fmt where applicable #8532

Open
wants to merge 2 commits into
base: master
from

Conversation

@lioncash
Copy link
Member

lioncash commented Dec 22, 2019

These helper functions are fairly trivial to convert over: no fancy formatting, just simple renaming Write over to WriteFmt and converting formatting specifiers over to {}. This'll make it more straightforward when converting over the shader generators, since std::string_view can now be used with these functions without needing to call .data().

While we're in the same area, we can also convert several template functions into normal functions, given the type of their argument is always the same type (ShaderCode&). This allows hiding details within the translation unit, allowing changes to the implementation of the functions without needing to recompile every single shader generator translation unit. This also uncovered quite a few indirect inclusions in the process, which have also been amended.

I've separated the formatting changes and the changes that migrate the template functions to regular functions to make reviewing the two changes nicer on the reader.

@lioncash lioncash force-pushed the lioncash:shader-common-fmt branch from 9743c99 to 90bd825 Dec 22, 2019
Copy link
Member

BhaaLseN left a comment

Code seems good in general, untested. Ctor remark is ignorable (since I'm pretty sure you know better)


std::unique_ptr<VideoCommon::ShaderCache> g_shader_cache;

namespace VideoCommon
{
ShaderCache::ShaderCache() = default;
ShaderCache::ShaderCache() : m_api_type{APIType::Nothing}

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Dec 22, 2019

Member

Feels a bit weird to go from direct member initialization and default ctor to one that's typed out just to have a forward-declared enum class in the header instead. Is this really worth it?

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 22, 2019

Author Member

I don't particularly see anything unusual about it in this case, given if anyone was concerned about the final state of the internal member variables after construction, then one would need to see what the constructor is doing anyways. I can revert this portion of the change if it's not desirable.

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Dec 23, 2019

Member

No, was mainly wondering since for the most part, the suggestion is to do the reverse (get rid of the ctor and move it to the members themselves; so it isn't forgotten if another ctor is added).
Just wanting to wrap my head around it.

@lioncash lioncash force-pushed the lioncash:shader-common-fmt branch from 90bd825 to e43f9a0 Dec 22, 2019
return "sample";
}
}
const char* GetInterpolationQualifier(bool msaa, bool ssaa, bool in_glsl_interface_block = false,

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Dec 23, 2019

Contributor
Suggested change
const char* GetInterpolationQualifier(bool msaa, bool ssaa, bool in_glsl_interface_block = false,
std::string_view GetInterpolationQualifier(bool msaa, bool ssaa, bool in_glsl_interface_block = false,

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Dec 23, 2019

Contributor

It's probably nearly irrelevant but I noticed that result of this function is passed to a string_view argument, so using a string view here may tidy things up a bit.

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 23, 2019

Author Member

I'm waiting until all relevant usage cases with the ShaderCode generators are converted over to WriteFmt, so that we're using an API that safely handles custom types like string_view before making this change.

@lioncash

This comment has been minimized.

Copy link
Member Author

lioncash commented Jan 24, 2020

@stenzek Whenever you have the time, can you look this over?

lioncash added 2 commits Dec 22, 2019
A very trivial conversion, this simply converts calls to Write over to
WriteFmt and adjusts the formatting specifiers as necessary.

This also allows the const char* parameters to become std::string_view
instances, allowing for ease of use with other string types.
These are only ever used with ShaderCode instances and nothing else.
Given that, we can convert these helper functions to expect that type of
object as an argument and remove the need for templates, improving
compiler throughput a marginal amount, as the template instantiation
process doesn't need to be performed.

We can also move the definitions of these functions into the cpp file,
which allows us to remove a few inclusions from the ShaderGenCommon
header. This uncovered a few instances of indirect inclusions being
relied upon in other source files.

One other benefit is this allows changes to be made to the definitions
of the functions without needing to recompile all translation units that
make use of these functions, making change testing a little quicker.

Moving the definitions into the cpp file also allows us to completely
hide DefineOutputMember() from external view, given it's only ever used
inside of GenerateVSOutputMembers().
@lioncash lioncash force-pushed the lioncash:shader-common-fmt branch from e43f9a0 to c682805 Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.