Skip to content

Conversation

@KAndQ
Copy link

@KAndQ KAndQ commented Oct 14, 2019

统一所有平台的日志输出, 不会有多一行换行的问题出现.

统一所有平台的日志输出, 不会有多一行换行的问题出现.
@minggo
Copy link
Contributor

minggo commented Oct 15, 2019

@KAndQ thanks for your contribution, but please use english in future. Thanks.

@minggo minggo requested a review from PatriceJiang October 15, 2019 01:31
@minggo minggo merged commit 0d41ade into cocos2d:v3 Oct 30, 2019
@minggo minggo added this to the next milestone Oct 30, 2019
@rh101
Copy link
Contributor

rh101 commented Nov 21, 2019

What is the reason behind doing this? Without line breaks, it makes it very hard to scan the logs, both visually and with automated tools, because it is literally attaching unrelated log output lines together.

EDIT: There is a bug introduced in this PR. The fix is in PR #20357

@rh101
Copy link
Contributor

rh101 commented Nov 21, 2019

@minggo There are 2 issues with the code:

In void log(const char * format, ...):

#if CC_TARGET_PLATFORM != CC_PLATFORM_WIN32
    buf[nret] = '\n';
#endif
    buf[++nret] = '\0';

The nret should not be incremented, so the correct code would be:

#if CC_TARGET_PLATFORM != CC_PLATFORM_WIN32
    buf[nret] = '\n';
    buf[++nret] = '\0';
#else
    buf[nret] = '\0';
#endif

Also, the main cause of the Windows Visual Studio debugger output problems:

        MultiByteToWideChar(CP_UTF8, 0, tempBuf, -1, wszBuf, sizeof(wszBuf));
        OutputDebugStringW(wszBuf);
        WideCharToMultiByte(CP_ACP, 0, wszBuf, -1, tempBuf, sizeof(tempBuf), nullptr, FALSE);

Should be:

        MultiByteToWideChar(CP_UTF8, 0, tempBuf, -1, wszBuf, sizeof(wszBuf));
        OutputDebugStringW(wszBuf);
        OutputDebugStringW(L"\n");
        WideCharToMultiByte(CP_ACP, 0, wszBuf, -1, tempBuf, sizeof(tempBuf), nullptr, FALSE);

The reason for this is that the \n is no longer added to the line (in the code before it), so it is what's causing the bad output in the Visual Studio console window.

@minggo
Copy link
Contributor

minggo commented Nov 22, 2019

@PatriceJiang please take a look.

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.

4 participants