Skip to content

Added MidlSwitches for #1428.#1432

Merged
mikebattista merged 4 commits intomainfrom
mikebattista/midlswitches
Jan 30, 2023
Merged

Added MidlSwitches for #1428.#1432
mikebattista merged 4 commits intomainfrom
mikebattista/midlswitches

Conversation

@mikebattista
Copy link
Copy Markdown
Contributor

@mikebattista mikebattista commented Jan 18, 2023

I followed the existing pattern for AdditionalIncludes to add MidlSwitches for #1428.

@mikebattista
Copy link
Copy Markdown
Contributor Author

@benkuhn if we wanted to set /notlb as the default, I believe we would make that change in commonMidlArgs.

abolade
abolade previously approved these changes Jan 19, 2023
Copy link
Copy Markdown

@abolade abolade left a comment

Choose a reason for hiding this comment

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

This looks fine for simply appending switches to the midl.exe command line.

If the command line needs to be replaced, a different approach might be needed: check if MidlSwitches is null or empty and, if so, use the defaults currently set in commonMidlArgs; otherwise, use the contents of MidlSwitches instead of the defaults currently set in commonMidlArgs.

@mikebattista
Copy link
Copy Markdown
Contributor Author

Thanks. For now, I'd like to start with being able to append flags that are missing.

Unless we want to make a change to set /notld by default per @benkuhn's comments, then we can set that switch in the default arguments.

Are there other MIDL parameters that would need to be appended in this way besides /notld? If we made /notld the default, would we still want to have this ability to append additional arguments?

chenss3
chenss3 previously approved these changes Jan 19, 2023
@riverar
Copy link
Copy Markdown
Collaborator

riverar commented Jan 20, 2023

I'd just hardcode /notld and not allow for the configurability. Otherwise it may give others the impression they can pass in other parameters (that may break win32metadata).

But am okay with the changes as they stand right now.

@mikebattista
Copy link
Copy Markdown
Contributor Author

If we want to keep this, I believe the below would also need to be updated to pass through the user-defined switches to the CompileIdls Task.

<CompileIdls
Idls="@(Idls)"
ObjDir="$(ObjDir)"
CompiledHeadersDir="$(CompiledHeadersDir)"
ScriptsDir="$(ScriptsDir)"
SdkIncRoot="$(SdkIncRoot)"
SdkBinDir="$(WindowsSDKVersionedBinRoot)"
AdditionalIncludes="$(AdditionalIncludes)"
MSBuildProjectDirectory="$(MSBuildProjectDirectory)"/>
</Target>

BenJKuhn
BenJKuhn previously approved these changes Jan 20, 2023
@mikebattista mikebattista dismissed stale reviews from BenJKuhn, chenss3, and abolade via 44206e3 January 30, 2023 18:21
@mikebattista
Copy link
Copy Markdown
Contributor Author

mikebattista commented Jan 30, 2023

What are the implications of setting /notlb by default? Is that expected to work in all scenarios?

@kennykerr
Copy link
Copy Markdown
Contributor

That should be safe. Since you aren't consuming tlb files, you can safely avoid generating them.

@mikebattista
Copy link
Copy Markdown
Contributor Author

Ok. I will set it by default and leave the custom additional flags support in. It's optional so there's no harm in leaving it there as an experiment.

@mikebattista mikebattista merged commit ad9264e into main Jan 30, 2023
@mikebattista mikebattista deleted the mikebattista/midlswitches branch January 30, 2023 21:43
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.

6 participants