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/FramebufferShaderGen: Minor clean up #8509

Merged
merged 6 commits into from Dec 8, 2019

Conversation

@lioncash
Copy link
Member

lioncash commented Dec 5, 2019

Cleans up the framebuffer shader generator code in various ways, mostly by collapsing unnecessary repeated string literal insertions into string streams; making for a nicer transition over to fmt.

While in the same area, we can also:

  • Make use of std::string_view to enforce the use of valid strings.
  • Add missing initial source file comments.
  • Remove an unused Config struct.

This isn't intended to perform any behavioral changes.

lioncash added 6 commits Dec 5, 2019
No behavioral change. This is intended to make the transition to fmt
less noisy in subsequent changes by combining insertions of multiple
string literals into one where applicable.
…ments

Makes the source files consistent with the rest of the VideoCommon code.
…plicable

Places all internal helpers and types within an anonymous namespace.
This isn't used anywhere within the codebase, so it can be removed
entirely.
…applicable

Prevents the use of the null pointer as an input to any functions.
…rnally

We only use these string streams to output into a final std::string
instance, we don't read into types with them. Because of this, we can
just make use of std::ostringstream, rather than the fully-fledged
std::stringstream.
@lioncash lioncash force-pushed the lioncash:shader-str branch from c46b53d to f297309 Dec 5, 2019
ss << " float4 position : SV_Position;\n"
"};\n";

ss << "struct GS_OUTPUT\n"

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Dec 5, 2019

Contributor

Why is this using ss << instead of making it one long string like you do above?

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 5, 2019

Author Member

Because the string above it is related to a completely different struct declaration. For visual clarity I've kept them apart from one another.

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Dec 5, 2019

Contributor

Sure, but keeping a blank line between those would still keep the separation clear, still without the need to have a separate <<. IMO

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 5, 2019

Author Member

There is no difference here other than preference. The use of << itself isn't an issue, especially given it's going to be moved over to fmt in a follow-up change, and I'd still prefer to keep the struct declarations separate from one another in that context as well.

@Helios747 Helios747 merged commit 9ef50a1 into dolphin-emu:master Dec 8, 2019
10 checks passed
10 checks passed
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@lioncash lioncash deleted the lioncash:shader-str branch Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.