Skip to content

Remove excessive .reserve calls, remove unexplained check#18630

Merged
hrydgard merged 2 commits into
masterfrom
more-feedback
Dec 29, 2023
Merged

Remove excessive .reserve calls, remove unexplained check#18630
hrydgard merged 2 commits into
masterfrom
more-feedback

Conversation

@hrydgard

Copy link
Copy Markdown
Owner

I don't see a path where that check could reasonably be hit, so removing it until we figure it out, due to its perf critical location, as commented by Unknown.

Also, remove a bunch of excessive .reserve calls from #18557

See comments on #18543

I don't see how we ever ran out of bounds there.
@hrydgard hrydgard added this to the v1.17.0 milestone Dec 29, 2023
@hrydgard hrydgard merged commit 9e82b12 into master Dec 29, 2023
@hrydgard hrydgard deleted the more-feedback branch December 29, 2023 09:47
@hrydgard hrydgard added the Code Cleanup Cleanup to make future work easier. Needs to be done sometimes. label Dec 29, 2023
@anr2me

anr2me commented Dec 29, 2023

Copy link
Copy Markdown
Collaborator

Regarding reserve, i also experiencing a strange issue where i need to reserve at least 1 element when reusing a vector after clearing it while i was working on sceHttp implementation (as commented at #18003 (comment)), where begin() returning null instead of an iterator/object, and i couldn't find any documentation that stated that begin() can returns a null anywhere (which is why it's a strange case as there are no documentation that mentions that this can happen), for example this documentation (which is quite detailed) https://en.cppreference.com/w/cpp/iterator/begin

Resulting any attempt to use the [supposed to be] object in normal way (ie. assuming it's an object per documentation) could triggered an exception/crash.

PS: i haven't tested the issue on linux build, in case it was a compiler bug that only occurred on windows.

@hrydgard

Copy link
Copy Markdown
Owner Author

Huh, that is quite surprising!

@unknownbrackets

Copy link
Copy Markdown
Collaborator

Well, that happens because you can't take a pointer to an empty (capacity) vector. It's kinda dangerous anyway.

For begin, "If the container is empty, the returned iterator value shall not be dereferenced." Basically, you shouldn't use it. I don't see why null wouldn't be a valid return value in that case.

It's really an implementation detail that reserve() makes begin() return something dereferenceable for a zero-length vector. It might change based on optimization settings or compiler or libc++ variant or whatever. It's definitely not a safe thing to do.

-[Unknown]

@hrydgard

Copy link
Copy Markdown
Owner Author

Yeah, indeed - reserve really doesn't mean you are allowed to call begin - that's a bug. still, surprising that the implementation is like that.

@anr2me

anr2me commented Dec 30, 2023

Copy link
Copy Markdown
Collaborator

Of course, begin() of an empty vector shouldn't be dereferenced, but it should still be usable for insert for example, meanwhile if begin() is null insert would failed, or even triggered an access violation if insert internally tried to access memory address 0x0 because of it.

Anyway, just now i tried to reproduce the issue by removing the reserve, and apparently the issue didn't happened anymore O.o may be because i'm using a newer version of VS2022 than the last time i debug it, it used to be consistently crashed due to trying to access memory address 0x0.

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

Labels

Code Cleanup Cleanup to make future work easier. Needs to be done sometimes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants