Skip to content

Fix invalid inlining#34234

Merged
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:bad_inlining
Jun 14, 2016
Merged

Fix invalid inlining#34234
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:bad_inlining

Conversation

@GuillaumeGomez

Copy link
Copy Markdown
Member

r? @steveklabnik

So to put a context. @nox found an issue on the generated doc:

screenshot from 2016-06-11 19-53-38

So as you can see, the two variants are on the same where they shouldn't. I found out that the issue is also on structs:

screenshot from 2016-06-11 19-53-31

And so such is the result of the PR:

screenshot from 2016-06-12 01-15-21
screenshot from 2016-06-12 01-15-24

@steveklabnik

Copy link
Copy Markdown
Contributor

Do we know what change caused this regression? I'd rather undo whatever broke it then add more stuff on top.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

I have no idea. But it seems strange that it worked.

@est31

est31 commented Jun 12, 2016

Copy link
Copy Markdown
Member

@GuillaumeGomez see my comment here

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

@est31: It matches what I found, so that's why I think just fixing it like this might be better.

@sanxiyn

sanxiyn commented Jun 13, 2016

Copy link
Copy Markdown
Contributor

I believe this was caused by #33867, cc @oli-obk. Looking at diff, struct field rendering was changed from table (which does linebreaking) to span (which does not).

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

@sanxiyn: In this PR, it's still a table.

@sanxiyn

sanxiyn commented Jun 13, 2016

Copy link
Copy Markdown
Contributor

@GuillaumeGomez I don't understand what you are saying. I see no <table> in diff here, and I see <table> in diff in #33867.

@GuillaumeGomez

GuillaumeGomez commented Jun 13, 2016

Copy link
Copy Markdown
Member Author

Oh sorry. I thought you said that #33867 removed the <table>.

Anyway, I don't understand why we'd need a <table> in there. It's better with <span>.

@sanxiyn

sanxiyn commented Jun 13, 2016

Copy link
Copy Markdown
Contributor

@steveklabnik asked what change caused this regression, so I answered it is #33867. Yes, I am saying that #33867 removed <table> in struct field case.

@nox

nox commented Jun 13, 2016

Copy link
Copy Markdown
Contributor

It should probably be a <ul> IMO.

Edit: s/ol/ul/

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

@nox: It'll be part of a big HTML/CSS refactor.

@steveklabnik

Copy link
Copy Markdown
Contributor

Okay. I think, given all this, adding a small hack is better, given that the regression has already landed. This isn't the worst thing in the world.

@bors: r+

@bors

bors commented Jun 14, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 7cd8912 has been approved by steveklabnik

@bors

bors commented Jun 14, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 7cd8912 with merge be8bd82...

bors added a commit that referenced this pull request Jun 14, 2016
Fix invalid inlining

r? @steveklabnik

So to put a context. @nox found an issue on the generated doc:

![screenshot from 2016-06-11 19-53-38](https://cloud.githubusercontent.com/assets/3050060/15987898/f7341de0-303b-11e6-9cd7-f2a6df423ee7.png)

So as you can see, the two variants are on the same where they shouldn't. I found out that the issue is also on structs:

![screenshot from 2016-06-11 19-53-31](https://cloud.githubusercontent.com/assets/3050060/15987900/0f66c5de-303c-11e6-90fc-5e49d11b6903.png)

And so such is the result of the PR:

![screenshot from 2016-06-12 01-15-21](https://cloud.githubusercontent.com/assets/3050060/15987904/19d9183c-303c-11e6-91c1-7c3f1163fbb0.png)
![screenshot from 2016-06-12 01-15-24](https://cloud.githubusercontent.com/assets/3050060/15987905/1b5d2db0-303c-11e6-8f43-9a8ad2371007.png)
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