Skip to content

feat: add name de-mangling for C++ symbols in the Test Explorer#4340

Merged
gcampbell-msft merged 8 commits intomicrosoft:mainfrom
rjaegers:feature/add-cpp-name-demangling
May 13, 2025
Merged

feat: add name de-mangling for C++ symbols in the Test Explorer#4340
gcampbell-msft merged 8 commits intomicrosoft:mainfrom
rjaegers:feature/add-cpp-name-demangling

Conversation

@rjaegers
Copy link
Copy Markdown
Contributor

@rjaegers rjaegers commented Mar 8, 2025

This changes visible behavior

The following changes are proposed:

  • Add name de-mangling for C++ symbols in the Test Explorer view

The purpose of this change

Currently when coverage is measured during test execution the mangled name is shown in case of C++ symbols. This PR adds name de-mangling for C++ symbols. When the symbol is not a mangled C++ name, the input is left un-touched.

@rjaegers
Copy link
Copy Markdown
Contributor Author

rjaegers commented Mar 8, 2025

@gcampbell-msft this is the PR that adds de-mangling. I have added a package to do the actual de-mangling, and I think I should not have pushed the yarn.lock file. I will revert the changes in that file and hope you and your team can do the magic to get the added package onboard.

Copy link
Copy Markdown
Collaborator

@gcampbell-msft gcampbell-msft left a comment

Choose a reason for hiding this comment

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

Overall, I don't have any concerns necessarily, but being somewhat unfamiliar with the packages available in node for demangling, I wanted to ask, why did you pick this specific one?

I see that there are a couple of npm packages for it: https://www.npmjs.com/search?q=demangler, and there are a couple that have more downloads (not that more downloads = better quality), and I was curious if there was any reason specifically?

Thanks

@rjaegers
Copy link
Copy Markdown
Contributor Author

I picked the package that:

  1. had tests
  2. I could review the code of
  3. satisfied my API requirements (i.e. either demangle the mangled name and return that, or return the input untouched)

That left the demangler-js package as most viable option in my opinion.

@rjaegers
Copy link
Copy Markdown
Contributor Author

@gcampbell-msft is there any further action required from my side to drive this home?

@gcampbell-msft
Copy link
Copy Markdown
Collaborator

@rjaegers No, nothing from your side, I need to get around to updating the package-lock.json on our side. Thanks for the ping.

@gcampbell-msft
Copy link
Copy Markdown
Collaborator

@rjaegers Update, I've updated the yarn.lock file and kicked off the builds, we will review and hopefully merge this PR today or next week.

@gcampbell-msft gcampbell-msft enabled auto-merge (squash) April 11, 2025 16:12
gcampbell-msft
gcampbell-msft previously approved these changes Apr 11, 2025
snehara99
snehara99 previously approved these changes Apr 11, 2025
paulmaybee
paulmaybee previously approved these changes Apr 11, 2025
@gcampbell-msft gcampbell-msft dismissed stale reviews from paulmaybee, snehara99, and themself via 1b51bb0 April 16, 2025 12:07
@gcampbell-msft gcampbell-msft added this to the 1.21 milestone Apr 25, 2025
@gcampbell-msft gcampbell-msft merged commit b084cc3 into microsoft:main May 13, 2025
4 checks passed
fajkomix1990 pushed a commit to fajkomix1990/mati---glowny that referenced this pull request Jul 2, 2025
…osoft#4340)

* feat: add name demangling for C++ symbols

* docs: update CHANGELOG

* chore: revert changes in yarn.lock as per CONTRIBUTING guide

* updating yarn.lock

---------

Co-authored-by: Garrett Campbell <[email protected]>
TSonono added a commit to TSonono/vscode-cmake-tools that referenced this pull request Jan 20, 2026
hanniavalera pushed a commit that referenced this pull request Jan 20, 2026
…er" (#4658)

* Revert "feat: add name de-mangling for C++ symbols in the Test Explorer (#4340)"

This reverts commit b084cc3.

* Update CHANGELOG.md
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