Skip to content
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.

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

rustdoc: Remove 'need_backline' field from DocFragment #92095

Merged
merged 2 commits into from
Dec 21, 2021
Merged

rustdoc: Remove 'need_backline' field from DocFragment #92095

merged 2 commits into from
Dec 21, 2021

Conversation

vacuus
Copy link
Contributor

@vacuus vacuus commented Dec 19, 2021

Fixes #92084

@rust-highfive
Copy link
Collaborator

Some changes occurred in clean/types.rs.

cc @camelid

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 19, 2021
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2021
@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

Just having noticed #92084, this PR may become obsolete.

To clarify, lines 945-950 and 960-962 would be deleted, so the only change in the end would be to unconditionally push a newline. I suppose that "mostly obsolete" would've fit better.

@vacuus vacuus changed the title rustdoc: Omit conditional check of successive line in loop rustdoc: Omit conditional check of successive line in loop & don't use 'need_backline' (tentative) Dec 19, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 19, 2021

@vacuus here's a guide on fixing conflicts: https://rustc-dev-guide.rust-lang.org/git.html#rebasing-and-conflicts

I'm not sure what you mean by obsolete - this is exactly the sort of change I was hoping to see when I opened the issue :)

Copy link
Member

@jyn514 jyn514 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 great! Could you now remove the need_backline field everywhere in types.rs? That should give a big memory improvement :)

// multiple lines in case of `#[doc = ""]`).
// * Adding backlines between `DocFragment`s and adding an extra one if required (stored in the
// `need_backline` field).
// Note: remove the trailing newline where appropriate
Copy link
Member

Choose a reason for hiding this comment

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

While you're rewriting these comments, can you make them doc-comments so they show up on doc.rust-lang.org/rustdoc?

@jyn514 jyn514 assigned jyn514 and unassigned GuillaumeGomez Dec 19, 2021
@jyn514 jyn514 added I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2021
@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

I confess that I'm stumped on the rebase. Right after cloning my fork, I did git remote add upstream https://github.com/rust-lang/rust.git and then git pull upstream master. However, git rebase master just says "current branch master is up to date". Doing git config pull.rebase true instead of git config pull.rebase false changes nothing. Would it be better to close this PR and open a new one?

@jyn514
Copy link
Member

jyn514 commented Dec 19, 2021

@vacuus try git fetch upstream and then git rebase upstream/master

@vacuus vacuus changed the title rustdoc: Omit conditional check of successive line in loop & don't use 'need_backline' (tentative) rustdoc: Remove 'need_backline' field from DocFragment Dec 19, 2021
@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

Sorry to bother you more, but do I just allow the changes I made? For example, this is one of the conflicts: since the goal is to remove 'need_backline`, the if let would be deleted. There shouldn't be any problem with me deleting the whole snippet and considering that conflict resolved, right?

<<<<<<< master
=======
                if let Some(prev) = doc_strings.last_mut() {
                    prev.need_backline = true;
                }

>>>>>>> master

@jyn514
Copy link
Member

jyn514 commented Dec 19, 2021

Yup! That whole block shouldn't be needed any more.

@camelid
Copy link
Member

camelid commented Dec 19, 2021

All you need to do is delete the conflict marker arrows and made sure the code that is left behind is what you want. So if you want to delete the section that has conflicts, just delete it.

@camelid
Copy link
Member

camelid commented Dec 19, 2021

Should this be marked as closing #92084? (Just add the phrase "Fixes #92084" to the PR description body.)

@rust-log-analyzer

This comment has been minimized.

@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

The size assertion at line 923 (rustc_data_structures::static_assert_size!(DocFragment, 32);) should be updated to 31 bytes, right?

@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

There seem to be quite a few compilation errors due to a String being returned from one or two of the functions instead of an Option. I'll try to fix those. The macro-related ones are probably beyond my expertise.

@jyn514
Copy link
Member

jyn514 commented Dec 19, 2021

There seem to be quite a few compilation errors due to a String being returned from one or two of the functions instead of an Option. I'll try to fix those. The macro-related ones are probably beyond my expertise.

Wait, why did you change that in this PR? I thought this was only changing need_backline.

@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

I guess I thought that #92078 was already merged and I should update this accordingly. My bad. I'll revert those changes.

@rust-log-analyzer

This comment has been minimized.

@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

I suppose that, for alignment reasons, the size of DocFragment is still 32 bytes and not 31.

@rust-log-analyzer

This comment has been minimized.

@vacuus
Copy link
Contributor Author

vacuus commented Dec 19, 2021

That seems to be a formatting issue, yet running rustfmt on the file doesn't raise any complaints.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 20, 2021
@bors
Copy link
Contributor

bors commented Dec 20, 2021

⌛ Testing commit 386ab1e with merge 824ed5019ca407ae81afe5bc7163faee796aae22...

@bors
Copy link
Contributor

bors commented Dec 20, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 20, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@vacuus
Copy link
Contributor Author

vacuus commented Dec 20, 2021

Well, that's a doozy.

@matthiaskrgr

This comment has been minimized.

@matthiaskrgr
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2021
@bors
Copy link
Contributor

bors commented Dec 21, 2021

⌛ Testing commit 386ab1e with merge 9f6776cf4b2bda47d9bfa3dfc00b365b90cac7b4...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-2 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
9 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
10 


The actual run.stderr differed from the expected run.stderr.
Actual run.stderr saved to D:\a\rust\rust\build\x86_64-pc-windows-msvc\test\ui\panics\panic-short-backtrace-windows-x86_64\panic-short-backtrace-windows-x86_64.run.stderr
error: 1 errors occurred comparing run output.
status: exit code: 101
status: exit code: 101
command: PATH="D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage2\lib\rustlib\x86_64-pc-windows-msvc\lib;C:\Program Files (x86)\Windows Kits\10\bin\x64;C:\Program Files (x86)\Windows Kits\10\bin\10.0.22000.0\x64;C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64;C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64;D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage0-bootstrap-tools\x86_64-pc-windows-msvc\release\deps;D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage0\bin;D:\a\rust\rust\ninja;D:\a\rust\rust\msys2\mingw64\bin;C:\hostedtoolcache\windows\Python\3.10.0\x64\Scripts;C:\hostedtoolcache\windows\Python\3.10.0\x64;C:\msys64\usr\bin;D:\a\rust\rust\sccache;C:\Program Files\MongoDB\Server\5.0\bin;C:\aliyun-cli;C:\vcpkg;C:\cf-cli;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\2.7.3\x64;C:\cabal\bin;C:\ghcup\bin;C:\tools\ghc-9.2.1\bin;C:\Program Files\dotnet;C:\mysql\bin;C:\Program Files\R\R-4.1.2\bin\x64;C:\SeleniumWebDrivers\GeckoDriver;C:\Program Files (x86)\sbt\bin;C:\Rust\.cargo\bin;C:\Program Files (x86)\GitHub CLI;C:\Program Files\Git\bin;C:\Program Files (x86)\pipx_bin;C:\hostedtoolcache\windows\go\1.15.15\x64\bin;C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts;C:\hostedtoolcache\windows\Python\3.7.9\x64;C:\hostedtoolcache\windows\Ruby\2.5.9\x64\bin;C:\tools\kotlinc\bin;C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.312-7\x64\bin;C:\npm\prefix;C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\wbin;C:\ProgramData\kind;C:\Program Files\Eclipse Foundation\jdk-8.0.302.8-hotspot\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\ProgramData\Chocolatey\bin;C:\Program Files\Docker;C:\Program Files\PowerShell\7;C:\Program Files\Microsoft\Web Platform Installer;C:\Program Files\dotnet;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit;C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\150\DTS\Binn;C:\Program Files\nodejs;C:\Program Files\OpenSSL\bin;C:\Strawberry\c\bin;C:\Strawberry\perl\site\bin;C:\Strawberry\perl\bin;C:\ProgramData\chocolatey\lib\pulumi\tools\Pulumi\bin;C:\Program Files\TortoiseSVN\bin;C:\Program Files\CMake\bin;C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.4\bin;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files\Git\cmd;C:\Program Files\Git\mingw64\bin;C:\Program Files\Git\usr\bin;C:\tools\php;C:\Program Files (x86)\sbt\bin;C:\SeleniumWebDrivers\ChromeDriver;C:\SeleniumWebDrivers\EdgeDriver;C:\Program Files\Amazon\AWSCLIV2;C:\Program Files\Amazon\SessionManagerPlugin\bin;C:\Program Files\Amazon\AWSSAMCLI\bin;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\Program Files (x86)\Microsoft BizTalk Server;C:\Program Files\LLVM\bin;C:\Users\runneradmin\.dotnet\tools;C:\Users\runneradmin\AppData\Local\Microsoft\WindowsApps" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\ui\\panics\\panic-short-backtrace-windows-x86_64\\a.exe"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
thread 'main' panicked at 'd was called', D:\a\rust\rust\src/test\ui\panics\panic-short-backtrace-windows-x86_64.rs:48:5
   0: std::panicking::begin_panic
   1: d
   2: c
   3: b
---

Some tests failed in compiletest suite=ui mode=ui host=x86_64-pc-windows-msvc target=x86_64-pc-windows-msvc


command did not execute successfully: "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0-tools-bin\\compiletest.exe" "--compile-lib-path" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2\\bin" "--run-lib-path" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "--rustc-path" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2\\bin\\rustc.exe" "--src-base" "D:\\a\\rust\\rust\\src/test\\ui" "--build-base" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\ui" "--stage-id" "stage2-x86_64-pc-windows-msvc" "--suite" "ui" "--mode" "ui" "--target" "x86_64-pc-windows-msvc" "--host" "x86_64-pc-windows-msvc" "--llvm-filecheck" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\llvm\\build\\bin\\FileCheck.exe" "--nodejs" "C:\\Program Files\\nodejs\\node" "--npm" "C:\\Program Files\\nodejs\\npm" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\native\\rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\native\\rust-test-helpers" "--docck-python" "C:\\hostedtoolcache\\windows\\Python\\3.10.0\\x64\\python3.exe" "--lldb-python" "C:\\hostedtoolcache\\windows\\Python\\3.10.0\\x64\\python3.exe" "--gdb" "C:\\msys64\\usr\\bin\\gdb" "--llvm-version" "13.0.0-rust-1.59.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker dwp engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink libdriver lineeditor linker lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo webassemblyutils windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:42:07
Build completed unsuccessfully in 0:42:07
make: *** [Makefile:74: ci-subset-2] Error 1

@bors
Copy link
Contributor

bors commented Dec 21, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2021
@matthiaskrgr
Copy link
Member

@bors retry #92000

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2021
@bors
Copy link
Contributor

bors commented Dec 21, 2021

⌛ Testing commit 386ab1e with merge e100ec5...

@bors
Copy link
Contributor

bors commented Dec 21, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing e100ec5 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e100ec5): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate removing need_backline from rustdoc::clean::DocFragment
10 participants