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

Change const char * to std::string in BaseException #101

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

IceMage144
Copy link
Member

Signed off by: @victordomiciano
Signed off by: @Kazuo256
Signed off by: @josealvim

Signed off by: @victordomiciano
Signed off by: @Kazuo256
Signed off by: @josealvim
@Kazuo256
Copy link
Member

It seems va_start on Windows cannot receive reference-type arguments. We can separate the code of the constructor in a helper function on system/exceptions.cc that actually receives a const char*, and the constructor just delegates the string formatting to it.

@Kazuo256 Kazuo256 changed the title Changed const char * to std::string Changed const char * to std::string in BaseException Aug 20, 2017
@Kazuo256 Kazuo256 changed the title Changed const char * to std::string in BaseException Changed const char * to std::string in BaseException Aug 20, 2017
@Kazuo256 Kazuo256 changed the title Changed const char * to std::string in BaseException Change const char * to std::string in BaseException Aug 20, 2017
@henriquegemignani
Copy link
Member

I'd suggest keeping a version that receives a const char*: compile time strings such as "foo" are of the type const char[4], so converting to a std::string and convert back seems like a waste.

@Kazuo256
Copy link
Member

I was hoping to change the formatting code to variadic templates eventually, meaning this change would only be temporary and we'd soon be rid of the forth-and-back conversion.

@Kazuo256 Kazuo256 added this to the Release v0.5.0 milestone Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants