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

Miscellaneous patches #149

Merged
merged 8 commits into from Jan 5, 2019

Conversation

Projects
None yet
3 participants
@slipher
Copy link
Contributor

slipher commented Jan 1, 2019

No description provided.

slipher added some commits Dec 25, 2018

Avoid calling close() when fd = -1
The Windows debug CRT 'helpfully' blows up if you do this.
Cut down image name buffer to MAX_QPATH
It was 1024 with a comment explaining that the extra space is used to
accommodate some sort of function call syntax which can compose multiple
images. But I didn't find any code handling such syntax. (I tried
googling it and got something like "quake 4 mods for dummies".)
@@ -377,7 +377,12 @@ namespace Cmd {
}

const std::string& Args::Argv(int argNum) const {
return args[argNum];
static std::string empty;

This comment has been minimized.

@DolceTriade

DolceTriade Jan 1, 2019

Contributor

I don't think we should mask programming errors like this. I think out of bounds accesses should crash. There is no reason that a programmer should attempt to access an out of bounds item.

This comment has been minimized.

@slipher

slipher Jan 1, 2019

Contributor

Well there is no guarantee it crashes, you can get garbage data instead which is a lot less nice. I don't want to have the undefined behavior here since it would be easy to miss when porting an old-style command. I added an assert so it will crash in debug mode.

This comment has been minimized.

@Kangz

Kangz Jan 1, 2019

Contributor

I think @DolceTriade means that the ASSERT should be enough.

This comment has been minimized.

@DolceTriade

DolceTriade Jan 4, 2019

Contributor

ya

This comment has been minimized.

@slipher

slipher Jan 4, 2019

Contributor

You want it to silently return garbage in release mode?

This comment has been minimized.

@Kangz

Kangz Jan 4, 2019

Contributor

Yes

Show resolved Hide resolved src/engine/renderer/tr_init.cpp Outdated
const ssFormat_t format;
const std::string fileExtension;
public:
ScreenshotCmd(std::string cmdName, ssFormat_t format, std::string ext)

This comment has been minimized.

@DolceTriade

DolceTriade Jan 1, 2019

Contributor

why are we passing these by value?

This comment has been minimized.

@slipher

slipher Jan 1, 2019

Contributor

It is rarely executed code with small strings, so I don't find the textual noise added by std::move worthwhile. While if you used const&, there wouldn't actually be less copies.

@slipher slipher force-pushed the slipher:misc2019 branch from c912be5 to e47baa0 Jan 1, 2019

@slipher slipher force-pushed the slipher:misc2019 branch from e47baa0 to 049a11d Jan 4, 2019

@slipher slipher force-pushed the slipher:misc2019 branch from 049a11d to 7eda4b7 Jan 4, 2019

@slipher slipher merged commit ea00e7d into DaemonEngine:master Jan 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@slipher slipher deleted the slipher:misc2019 branch Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment