This is an archive of the discontinued LLVM Phabricator instance.

Edits to CMake.rst for clarity's sake
ClosedPublic

Authored by gaeke on Aug 8 2015, 1:08 AM.

Details

Summary

Having recently followed some of the instructions in CMake.rst I am
submitting this patch to try to improve it a little. Any comments
are appreciated.

Highlights:

  • Fix some grammatical and typographical errors.
  • Try to improve upon some awkward/nonstandard phrasings.
  • Expand slightly the treatment of how you specify arguments to cmake.
  • Update the list of possible LLVM_BUILD_TESTS and state where to find the definitive list.
  • Correct the name of llvm-tblgen.
  • Expand slightly the treatment of several build options, including LLVM_LIT_TOOLS_DIR, LLVM_ENABLE_FFI, and LLVM_EXTERNAL_project_SOURCE_DIR.

Diff Detail

Repository
rL LLVM

Event Timeline

gaeke updated this revision to Diff 31575.Aug 8 2015, 1:08 AM
gaeke retitled this revision from to Edits to CMake.rst for clarity's sake.
gaeke updated this object.
gaeke added a subscriber: llvm-commits.
gaeke updated this revision to Diff 32793.Aug 20 2015, 9:22 PM

Edits to CMake.rst for clarity's sake [updated]

Rebased the patch against current trunk.

vsk added a subscriber: vsk.Aug 22 2015, 12:01 AM

Thanks for your work on this! Comments inline.

docs/CMake.rst
34 ↗(On Diff #32793)

"Create a build directory", perhaps?

106 ↗(On Diff #32793)

Maybe just: "CMake requires that you specify a build tool (e.g ..."

113 ↗(On Diff #32793)

The text-wrapping changes here for some reason. Could you maintain the original width?

118 ↗(On Diff #32793)

I suggest dropping the plural possessive and just going for "This will list the generator names".

144 ↗(On Diff #32793)

Does this need to be capitalized?

171 ↗(On Diff #32793)

Please omit this.

443 ↗(On Diff #32793)

"while at" --> "in"

gaeke marked 6 inline comments as done.Aug 22 2015, 1:09 AM

Thanks for your feedback. I will submit a new patch shortly.

docs/CMake.rst
106 ↗(On Diff #32793)

I am going to go with "allows you to specify", as we go on immediately to state that CMake tries to figure out which tool you want to use if you don't specify it.

113 ↗(On Diff #32793)

I have tried to do this in my new version. Sorry about that. There were a lot of changes in this paragraph.

gaeke updated this revision to Diff 32900.Aug 22 2015, 1:11 AM
gaeke marked 2 inline comments as done.

This new version of the patch incorporates changes based on vsk's feedback.

gaeke set the repository for this revision to rL LLVM.Aug 22 2015, 1:15 AM

LGTM, but I won't approve it since I am not native English speaker.
Thanks for you work!

docs/CMake.rst
72 ↗(On Diff #32900)

One nitpicking. I suppose almost all guys wouldn't use "cmake --build".
We might refine around there later. (Don't care in this review, though)

126 ↗(On Diff #32900)

We dropped "Visual Studio 11", aka msc17, in trunk.
We might upgrade it, regardless of this review.

199 ↗(On Diff #32900)

BUILD_SHARED_LIBS is unsupported by MS toolchain, CL.EXE and LINK.EXE.

Mingw32(including mingw-w64) supports BUILD_SHARED_LIBS.

312 ↗(On Diff #32900)

I introduced this section.

I took %PATH%, rather than PATH or $PATH to clarify this aims Windows platform.

330 ↗(On Diff #32900)

They don't have default value any more.

  • It isn't referred if in-tree subdirectory, for example llvm/tools/clang &c. exists.
  • It should be set explicitlly if corresponding subproject is out of tree.

For now, I suggest not to mention default behavior.
See also changelog in llvm/cmake/modules/AddLLVM.cmake.

444 ↗(On Diff #32900)

IIRC, we have a plan to let "check" obsoleted.
I suggest to use "check-llvm" or "check-all" here.

chapuni added inline comments.Aug 22 2015, 5:55 AM
docs/CMake.rst
235 ↗(On Diff #32900)

Are you really willing to maintain this list whenever an unittest were added or removed?
I suggest here might be generalized presentation, for example "ADTTests, IRTests, etc. in llvm/unittests".

gaeke marked 7 inline comments as done.Aug 22 2015, 12:52 PM

Thanks for your comments. I will post an updated patch soon.

docs/CMake.rst
72 ↗(On Diff #32900)

I will leave it for now. cmake --build works well for me so far.

126 ↗(On Diff #32900)

OK. I changed it to "Visual Studio 12".

199 ↗(On Diff #32900)

OK. I revised this to state that shared libraries are supported if you use MinGW or mingw-w64.

235 ↗(On Diff #32900)

I agree. I have replaced this list with a small subset.

312 ↗(On Diff #32900)

I changed both PATH occurrences back to %PATH%.

330 ↗(On Diff #32900)

I reworded this to incorporate the points you stated here.

444 ↗(On Diff #32900)

OK. Should check-all be specified for Visual Studio as well? For now, I have changed the first two occurrences, and left the sentence about Visual Studio as-is.

gaeke updated this revision to Diff 32904.Aug 22 2015, 12:53 PM
gaeke marked 7 inline comments as done.

This new version of the patch incorporates changes based on chapuni's feedback.

chapuni added inline comments.Aug 22 2015, 4:46 PM
docs/CMake.rst
199–201 ↗(On Diff #32904)

I wonder it might misunderstand like
"BUILD_SHARED_LIBS is recommended on mingw."

IMO, BUILD_SHARED_LIBS is an option for developers.

443 ↗(On Diff #32904)

Yes. in trunk, "check" is an alias of "check-llvm".
For consistency in CMake scripts, "check-*" is the primary target and "check" is compatible stuff for autoconf Make.

You may also mention "check-*" to run tests for individual subproject, here.

445 ↗(On Diff #32904)

Although I am not a user of Xcode, I guess it might be generic for IDE Generators.

gaeke marked an inline comment as done.Aug 22 2015, 7:39 PM

Thanks for your comments. I will submit another patch soon.

docs/CMake.rst
199–201 ↗(On Diff #32904)

OK, I revised this to warn people that it is not for general use.

443 ↗(On Diff #32904)

OK, I changed the third occurrence to "check-all" as well. I don't think it makes a lot of sense to go too deeply into testing procedures here. It seems like people should consult the TestingGuide for that.

445 ↗(On Diff #32904)

I'm not a user of Xcode either. If other people want to edit this to explain what the situation is for Xcode, or for other IDEs, I will leave that up to them.

gaeke updated this revision to Diff 32911.Aug 22 2015, 7:42 PM
gaeke marked an inline comment as done.

This new version of the patch incorporates changes based on chapuni's 2nd set of comments.

This revision was automatically updated to reflect the committed changes.