Skip to content

[COMGR] Fix API usage per upstream changes #356

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 23, 2025
Merged

[COMGR] Fix API usage per upstream changes #356

merged 2 commits into from
Oct 23, 2025

Conversation

@chinmaydd
Copy link

No description provided.

jansvoboda11 and others added 2 commits October 22, 2025 22:42
@jansvoboda11 @chinmaydd
[clang] Don't silently inherit the VFS from FileManager (llvm#164323)
92fee18 
Since llvm#158381 the
`CompilerInstance` is aware of the VFS and co-owns it. To reduce scope
of that PR, the VFS was being inherited from the `FileManager` during
`setFileManager()` if it wasn't configured before. However, the
implementation of that setter was buggy. This PR fixes the bug, and
moves us closer to the long-term goal of `CompilerInstance` requiring
the VFS to be configured explicitly and owned by the instance.
@chinmaydd
[COMGR] Fix API usage of VFS per upstream
8541b1f
@chinmaydd chinmaydd requested a review from lamb-j October 23, 2025 03:12
@skganesan008
Copy link
Collaborator

PSDB Link: https://compiler-ci.amd.com/job/compiler-psdb-amd-staging/2395

@chinmaydd
Copy link
Author

chinmaydd commented Oct 23, 2025 •
edited
Loading

This is my first time raising a PR to public staging. Curious to see if the PSDB displays comgr test runs. Also, not sure what the strategy is for handling reverted PRs.

@ronlieb ronlieb self-requested a review October 23, 2025 03:18
ronlieb
ronlieb requested changes Oct 23, 2025
View reviewed changes
Copy link
Collaborator

@ronlieb ronlieb left a comment

Choose a reason for hiding this comment

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

LGTM, I will land it tomorrow after current merge lands

thx

@lamb-j
Copy link
Collaborator

lamb-j commented Oct 23, 2025 •
edited
Loading

LGTM, I will land it tomorrow after current merge lands

thx

Should we also reapply the reverted patch here? Or just land this patch and reapply separately (which I'm guessing you will handle)

Never mind, I now see this PR has both commits

@ronlieb
Copy link
Collaborator

ronlieb commented Oct 23, 2025

i typically clean up the revert_patches.txt later. generally let the npsdb run past lits, then kill it, and land it

@ronlieb ronlieb self-requested a review October 23, 2025 05:44
ronlieb
ronlieb approved these changes Oct 23, 2025
View reviewed changes
Copy link
Collaborator

@ronlieb ronlieb left a comment

Choose a reason for hiding this comment

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

ok to land

@ronlieb ronlieb merged commit 0216fcf into amd-staging Oct 23, 2025
6 checks passed
@ronlieb ronlieb deleted the amd/dev/chdeshpa/comgr-fix branch October 23, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronlieb ronlieb ronlieb approved these changes

@lamb-j lamb-j lamb-j approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chinmaydd @skganesan008 @lamb-j @ronlieb @jansvoboda11