Cleanup COM interfaces in jump list code - fix PreserveSig attributes #9899
Conversation
|
Close and reopen the PR to force Windows CI build working properly. |
|
@bergmeister Can you please review this PR? |
There was a problem hiding this comment.
I cannot comment much on the code change itself as I am not a COM expert (maybe you guys could ask someone from the .Net team?), so I cannot say if it makes it better or worse, we could trust the author but the PS team has to decide on the risk of changing something when there hasn't been reported issues (since we fixed the original problem by using an STA thread, I haven't heard about problems with the JumpLIst since then). I appreciate the author's good intentions but there is a chance that this is a change that could cause a regression in edge cases or less common scenarios like old OSs, especially since we do not have test automation for this feature.
I have tested manually though that the functionality still works on Win 10 (1809) (one needs to make sure to build PowerShell from a folder where the JumpList has never been created before due to persistence).
I therefore leave the decision up to the PowerShell team and want to stay neutral on this.
|
Looking at the docs, this change seems fine. |
|
@anmenaga based on the docs, it would seem this is correct. Can we get the CodeFactor issue fixed? |
|
@SteveL-MSFT do I insert newlines for the whole file or only the line it complains about? there are plenty more methods without separating newlines in this file, I think it only complains on the lines I touched. |
|
@weltkante only the line it is complaining about. Thanks! (General style issues in the file should be a seperate PR, but I'm not asking for you to do that unless you want to :) |
b2b0d26 to
cba8116
Compare
cba8116 to
c478c06
Compare
|
Very sorry for the noise, got my repo settings wrong and had to amend. Build failure seems independent? |
|
@PoshChan please retry windows |
|
@SteveL-MSFT, successfully started retry of |
|
@weltkante the static analysis failures are understood and should not block this PR |
|
Looks like the general consensus is to merge this PR. |
|
🎉 Handy links: |
PR Summary
Correction of wrong/unnecessary COM interop attributes
PR Context
While reviewing the JumpList COM interop code for #9295 I noticed that several
[PreserveSig]attributes were wrong. The methods which got the attribute wrong were never called, so this is not a direct fix for the issue, its just to prevent future problems should they ever be called.Also there were some unnecessary
[MethodImpl]attributes which are not supposed to be there, these are cleaned up.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.