This is an archive of the discontinued LLVM Phabricator instance.

[docs] Use literalinclude for most code in the Kaleidoscope tutorial to ensure the code always compiles and is up to date
AcceptedPublic

Authored by Voxel on Feb 20 2018, 3:12 AM.

Details

Reviewers
Wilfred
lhames
Summary

Hi

This is my first patch submission to this list, so you'll have to tell me if I do anything wrong somewhere.

I originally submitted this to llvm-commits directly a few days ago, but I later figured out how to use Differential to submit a patch, so it seemed reasonable to resubmit it here.

The code in the kaleidoscope tutorial is currently inlined in the documentation pages and is thus never compiled to ensure it still works. Over time this code has diverged from the full code listing in small and subtle ways (such as the FunctionPassManager now being in the legacy namespace) which makes it harder for anyone following the tutorial to actually get something working.

I have replaced almost all of the code blocks in the tutorial with literalinclude blocks which include code directly from the source files based on markers. This ensures that the code in the tutorial actually compiles and is up to date, even when future changes are made and reduces the maintenance burden since changes only have to be done in a single place.

I have also uploaded a visual html diff here: https://arongranberg.com/temp/llvm-diff/
with the changed files having URLs on the format https://arongranberg.com/temp/llvm-diff/LangImpl0[1-8].html
Note that the html diff library gets a little confused when some lines have been emphasized and it may emphasize more lines than are actually emphasized or may add lots of additional spacing.

The downside of this change is that some markers are required in the source files, however they are relatively few so hopefully it is not too big of a problem.

I added as reviewers two people who seem to have done large parts of the tutorial. Hopefully that's alright.

Diff Detail

Repository
rL LLVM

Event Timeline

Voxel created this revision.Feb 20 2018, 3:12 AM
Wilfred accepted this revision.Feb 21 2018, 12:31 AM

Thanks. I think this is a big improvement: it ensures the docs stay in sync with the code rather than discovering that they rely on removed APIs.

I like the token approach you've taken.

This revision is now accepted and ready to land.Feb 21 2018, 12:31 AM
Voxel added a comment.Feb 21 2018, 2:13 AM

Thanks. I think this is a big improvement: it ensures the docs stay in sync with the code rather than discovering that they rely on removed APIs.

I like the token approach you've taken.

Nice!
Since I don't have write access to the repository, how do we go about applying the patch?

Love this!

Though the ":lines: X, Y" doesn't seem to do the right thing for me (using sphinx-build v1.5.6 on my Ubuntu system). Any of the code samples that use the 'lines:' property end up empty for me.

Any ideas?

Though honestly, I'm thinking maybe it'd be better to not 'lines:' property anyway. Perhaps it's better to have the start/end comments in the source so when code is refactored, line wrapped, etc, the line counts don't end up becoming out of date?

Nitpick: Might be good to keep any whitespace that was present before (in some cases an empty line has been replaced with two comments (the end of one region and the start of the next) - perhaps keep the empty line between those two comments, for readibility and to reduce the noise in this diff)?

Another note/thought: It'd be great if, when including the whole file, there was some way to omit the snippet marker comments - but, on looking around at the Sphinx docs I don't see anything immediately obvious & figure there's nothing specifically to support that (I guess if someone cared a lot they could add new build rules that would strip the comments & produce some temporary files that could then be included when the whole source listing was desired).

Voxel added a comment.Feb 21 2018, 2:42 PM

Though the ":lines: X, Y" doesn't seem to do the right thing for me (using sphinx-build v1.5.6 on my Ubuntu system). Any of the code samples that use the 'lines:' property end up empty for me.

Looking at sphinx docs it seems like :lines: together with :start-after: only works properly in v1.6 or higher.

Though honestly, I'm thinking maybe it'd be better to not 'lines:' property anyway. Perhaps it's better to have the start/end comments in the source so when code is refactored, line wrapped, etc, the line counts don't end up becoming out of date?

I agree that line counts are not ideal, unfortunately (also related to your other comment) it does not seem possible to strip the tags using sphinx, and in several places in the tutorial there are overlapping code blocks which would mean that tags would show up inside those code blocks, and that's not ideal either.

Nitpick: Might be good to keep any whitespace that was present before (in some cases an empty line has been replaced with two comments (the end of one region and the start of the next) - perhaps keep the empty line between those two comments, for readibility and to reduce the noise in this diff)?

Ah, true. I tried to keep it, but it looks like I missed a few places. I'll see if I can tweak the diff a bit.

Voxel updated this revision to Diff 135381.Feb 22 2018, 2:38 AM

Fixed some newline noise in the diff.

Anyone who knows which version of Sphinx that the server that builds the documentation uses? Or anyone who knows how to look that up?

Anyone who knows which version of Sphinx that the server that builds the documentation uses? Or anyone who knows how to look that up?

Looks like maybe the closest to guidance on that we have is: https://llvm.org/docs/GettingStarted.html#local-llvm-configuration which says "Sphinx version 1.5 or later recommended.".

Voxel added a comment.Feb 27 2018, 2:13 AM

I found that there is apparently a config file that can be used to set the minimum sphinx version that is required.

It can be found in the llvm/docs/conf.py file:

# If your documentation needs a minimal Sphinx version, state it here.
#needs_sphinx = '1.0'

Currently it is not set, but I suppose one option would be to set that to 1.6 to at least ensure that anyone who tries to build the docs with an outdated version of sphinx will get an error message.

Hey Aaron (Ballman) - do you happen to know who maintains sphinx bots, etc?
Wondering if we can reasonably require a certain version (looks like
there's a feature in 1.6 that could be handy).

Hey Aaron (Ballman) - do you happen to know who maintains sphinx bots, etc?
Wondering if we can reasonably require a certain version (looks like
there's a feature in 1.6 that could be handy).

Tanya Lattner is the one currently maintaining the automatic docs builder for attributes (and I believe compiler flags as well) and I think Dmitri Gribenko maintains the actual bots (clang-sphinx-docs, llvm-sphinx-docs, etc).

I'm not certain what version we're building with currently -- I couldn't find the information in any of my emails. I think 1.6 was released in May 2017 which seems a bit recent for it being a minimum requirement. That said, I don't have Opinions on whether to bump the min version or not.

@tonic and @gribozavr, do you have any opinions or info about this?