Skip to content

Enhance UNICODE support #192

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

Closed
wants to merge 4 commits into from
Closed

Enhance UNICODE support #192

wants to merge 4 commits into from

Conversation

malikm
Copy link

@malikm malikm commented Dec 12, 2023

Also to fix #191

Copy link
Contributor

@JeffersonMontgomery-Intel JeffersonMontgomery-Intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank-you for the contribution!

There is one change that is needed (see inline comment). I probably would have gone with wchar_t vs WCHAR, but don't bother changing that now.

I will pull, test, and accept this sometime this week. I can fix that one event then so you don't need to, this is just FYI.

@@ -2218,7 +2215,7 @@ void PMTraceConsumer::HandleProcessEvent(EVENT_RECORD* pEventRecord)
};
mMetadata.GetEventData(pEventRecord, desc, _countof(desc));
event.ProcessId = desc[0].GetData<uint32_t>();
event.ImageFileName = desc[1].GetData<std::string>();
event.ImageFileName = desc[1].GetData<std::wstring>();
event.IsStartEvent = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the underlying data for this event is always char, and GetData() doesn't do any conversion, it just accesses the event data as if it were a T. So, we need to leave it as GetData() and then convert here.

In debug build GetData() should be asserting with this change, though you'll only hit this event when processing from an ETL file (--etl_file).

@JeffersonMontgomery-Intel JeffersonMontgomery-Intel added bug enhancement PresentMon-ConsoleApplication Relates to the console application (PresentMon/) PresentData Relates to the collection/analysis library (PresentData/) labels Dec 12, 2023
@malikm
Copy link
Author

malikm commented Dec 12, 2023

Thanks for the review. I did all changes requested, hope it's OK now :)

@JeffersonMontgomery-Intel
Copy link
Contributor

JeffersonMontgomery-Intel commented Dec 14, 2023

Thanks! Testing now and everything looks good. I plumbed it through to the CSV too so should be supported throughout now:

image

Should land in main branch today or tomorrow next week.

JeffersonMontgomery-Intel added a commit that referenced this pull request Dec 28, 2023
See PR #192 and issue #191.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement PresentData Relates to the collection/analysis library (PresentData/) PresentMon-ConsoleApplication Relates to the console application (PresentMon/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when launching an application with non-ANSI characters in filename
2 participants