Rename /P to /Po; make /P match cl.exe behavior#8165
Conversation
There was a problem hiding this comment.
Pull request overview
This PR renames the old FXC-style /P preprocessing flag to /Po (with deprecation warnings) and makes /P behave like cl.exe, which preprocesses to <inputname>.i by default. The change maintains backward compatibility through /Po while aligning DXC's behavior with the widely-used Microsoft C/C++ compiler.
Changes:
- Added new
/Poflag preserving FXC-style preprocessing behavior (deprecated) - Updated
/Pto matchcl.exe: defaults to<input>.i, supports/Fito override - Maintained support for old
/Po <filename>positional syntax with deprecation warnings
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| include/dxc/Support/HLSLOptions.td | Adds /Po flag definition for backward compatibility |
| lib/DxcSupport/HLSLOptions.cpp | Implements separate logic for /P (cl.exe-style) and /Po (FXC-style with deprecation) |
| tools/clang/unittests/HLSL/OptionsTest.cpp | Adds comprehensive unit tests for both /P and /Po behaviors |
| tools/clang/test/DXC/preprocess.test | Adds integration tests verifying preprocessing with /P and /Po |
| docs/ReleaseNotes.md | Documents the command-line interface changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Gah, I merged the wrong main into this branch. I'm afraid I'm going to have to do a rebase + force push. |
Rename the old FXC-style /P flag to /Po (deprecated) and make /P behave like cl.exe: preprocess to <input>.i by default, with /Fi to specify the output filename. The old /Po <filename> positional syntax is preserved with a deprecation warning directing users to /P /Fi. Fixes microsoft#4611 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Verify /Po deprecation warning is emitted in preprocess.test - Clarify ReleaseNotes.md: positional syntax is deprecated and moved to /Po, not removed entirely Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update test to use -P -Fi instead of the old -P <filename> syntax, which was changed to -Po in the previous commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add test cases verifying that /P with extra positional arguments (e.g. -P out.pp input.hlsl) does not emit the deprecation warning that is specific to /Po. These cases confirm the /P and /Po code paths are fully independent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the deprecation warning to fire for every /Po usage, not just the positional-filename case. The warning adapts its suggestion: - /Po with default output: 'please use /P instead' - /Po with /Fi or positional filename: 'please use /P /Fi <file> instead' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace bare /P and /Po tests that wrote smoke.i next to the source file with /P /Fi %t variants that write to the output directory. The default filename logic is already covered by unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the /P and /Po preprocess option handling into a standalone static helper function, reducing nesting in ReadDxcOpts to a single line: opts.Preprocess = getPreprocessOutput(Args, Errors); No behavioral change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
I think this looks good, and I checked the API wrapper, and it looks like it will be ok, but that wrapper does not use the expected default name.
That would probably be the one improvement I'd recommend, to update this hard-coded name to be based on the pSourceName (<inputname>.i) instead:
I suppose you could consider this is a separate change, but it did seem related to cleaning up the old behavior. |
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
I think this looks fine. Regarding my previous comment, it's unlikely anyone will be seeing or depending on the hard coded file name, it was just something that looked inconsistent with the current default file name for the output. I'm ok with leaving it as-is.
Rename the old FXC-style /P flag to /Po (deprecated) and make /P behave like cl.exe: preprocess to .i by default, with /Fi to specify the output filename. The old /Po positional syntax is preserved with a deprecation warning directing users to /P /Fi.
Fixes #4611