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

Add support for wchar_t on Windows #111

Merged
merged 8 commits into from
Jul 26, 2015
Merged

Conversation

Kentzo
Copy link
Contributor

@Kentzo Kentzo commented Jul 13, 2015

No description provided.

@gabime
Copy link
Owner

gabime commented Jul 14, 2015

Thanks for the pull. Two problems:
1.the build fails in travis
2. I dont want to impose wchar on windows. It should be configurable

@Kentzo
Copy link
Contributor Author

Kentzo commented Jul 14, 2015

  1. We will fix that, @a-martynovich Please take a look.
  2. Do you have some concept in mind to make it configurable?

@gabime
Copy link
Owner

gabime commented Jul 14, 2015

Not sure. Maybe simple ifdef will do. Most of them are defined in tweakme.h

@Kentzo
Copy link
Contributor Author

Kentzo commented Jul 18, 2015

@gabime Could you put some explanation behind making it configurable? The majority of windows desktop applications will use wchar_t (either right away or after they realize their app crash for a lot of users).

@gabime
Copy link
Owner

gabime commented Jul 18, 2015

@Kentzo add '''#define use_whcar''' to tweakme.h

@gabime
Copy link
Owner

gabime commented Jul 21, 2015

Seems the tests fails because it gets filesize of 2048 but expects 1024.. Makes sense for wchar

@a-martynovich
Copy link
Contributor

@gabime Fixed, please take a look.

@gabime
Copy link
Owner

gabime commented Jul 21, 2015

Thanks. Looks good.
Please remove the use ofSPDLOG_NO_WCHAR from the code -It is redundant
#define SPDLOG_USE_WCHAR is enough (and should commented out by default in tweakme.h)

@Kentzo
Copy link
Contributor Author

Kentzo commented Jul 26, 2015

@gabime Done but tests failed. Not sure if it's due to our changes.

gabime added a commit that referenced this pull request Jul 26, 2015
Add support for wchar_t on Windows
@gabime gabime merged commit 3632d01 into gabime:master Jul 26, 2015
gabime added a commit that referenced this pull request Aug 7, 2015
@gabime
Copy link
Owner

gabime commented Aug 7, 2015

I reverted this - it causes problems - global defines that conflicts with use code. We need to find a cleaner solution. Maybe a dedicated file sink

@Kentzo
Copy link
Contributor Author

Kentzo commented Aug 29, 2015

Current version is completely unusable without that change on Windows (except developer's environment I guess)...

Do you have a set of changes in mind to be made?

@Kentzo
Copy link
Contributor Author

Kentzo commented Sep 6, 2015

@gabime I'm willing to contribute so we can use spdlog's upstream in our product. Please think of steps this change requires to make it happen.

@gabime
Copy link
Owner

gabime commented Sep 7, 2015

Main issue was that it broke user code because it introdeced global defines..

@Kentzo
Copy link
Contributor Author

Kentzo commented Sep 7, 2015

@gabime We could use templates

@gabime
Copy link
Owner

gabime commented Sep 8, 2015

Another option would be defining a new sink class which supports wchar. This would be the less intrusive option i think.

@VikingExplorer
Copy link

What's the status of this? I'm getting

        auto log = spdlog::get( "debug" );
        log->debug( fmt.value().c_str(), value1 );
        auto msg = fmt::format( fmt.value(), value1 );

1>Log.cpp(75): error C2664: 'spdlog::details::line_logger spdlog::logger::debug<wchar_t>(const char *,const wchar_t &)': cannot convert argument 1 from 'const wchar_t *' to 'const char *'

I have already integrated with CppFormat, so I know it supports wchar_t.

What am I doing wrong? I searched the code for SPDLOG_USE_WCHAR and couldn't find it.

@Kentzo
Copy link
Contributor Author

Kentzo commented Mar 4, 2016

@VikingExplorer Change was reverted in the mainstream. We're using our fork for know. Feel free to improve this PR.

@Kentzo
Copy link
Contributor Author

Kentzo commented Apr 1, 2016

@gabime Is it all right to have the SPDLOG_USE_WCHAR macro though?

@Kentzo
Copy link
Contributor Author

Kentzo commented Apr 1, 2016

I don't think supporting 2 sink is a good solution: you would end up with 2 identical version of the code… Maybe I put the S macro under the SPDLOG_ namespace?

@gabime
Copy link
Owner

gabime commented Apr 2, 2016

I don't know.. The code got quite ugly with all those "tstrings" everywhere.. Not sure it worth it

@Kentzo
Copy link
Contributor Author

Kentzo commented Apr 3, 2016

@gabime I tried to do it with templates, it becomes even uglier: you cannot easily get around with char and wchar_t string literals.

Current implementation is terribly unusable on Windows though: you cannot have a path with Chinese symbols for example.

Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 3, 2016
Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 3, 2016
Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 3, 2016
Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 3, 2016
Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 3, 2016
@Kentzo
Copy link
Contributor Author

Kentzo commented Apr 3, 2016

@gabime Made another PR that attempts to solve this problem.

@gabime
Copy link
Owner

gabime commented Apr 3, 2016

@Kentzo it fails the tests. included file not found..

Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 4, 2016
Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 4, 2016
@Kentzo
Copy link
Contributor Author

Kentzo commented Apr 4, 2016

@gabime Fixed.

Kentzo added a commit to GreatFruitOmsk/spdlog that referenced this pull request Apr 8, 2016
gabime added a commit that referenced this pull request Apr 9, 2016
Add the SPDLOG_USE_WCHAR tweak to enable support for Unicode names on Windows. Refs #111
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.

None yet

4 participants