[PATCH] D11862: Edits to CMake.rst for clarity's sake

[Brian R. Gaeke via llvm-commits]

gaeke marked 7 inline comments as done.
gaeke added a comment.

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


================
Comment at: docs/CMake.rst:72
@@ -71,3 +71,3 @@
 
-   The underlying build tool can be invoked directly either of course, but
+   The underlying build tool can be invoked directly, of course, but
    the ``--build`` option is portable.
----------------
chapuni wrote:
> 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)
I will leave it for now. cmake --build works well for me so far.

================
Comment at: docs/CMake.rst:126
@@ -119,3 +125,3 @@
 
   $ cmake -G "Visual Studio 11" path/to/llvm/source/root
 
----------------
chapuni wrote:
> We dropped "Visual Studio 11", aka msc17, in trunk.
> We might upgrade it, regardless of this review.
OK. I changed it to "Visual Studio 12".

================
Comment at: docs/CMake.rst:199
@@ -190,3 +198,3 @@
   Flag indicating if shared libraries will be built. Its default value is
-  OFF. Shared libraries are not supported on Windows and not recommended on the
-  other OSes.
+  OFF. Shared libraries are not supported on Windows and not recommended on
+  other operating systems.
----------------
chapuni wrote:
> BUILD_SHARED_LIBS is unsupported by MS toolchain, CL.EXE and LINK.EXE.
> 
> Mingw32(including mingw-w64) supports BUILD_SHARED_LIBS.
OK. I revised this to state that shared libraries are supported if you use MinGW or mingw-w64.

================
Comment at: docs/CMake.rst:235
@@ -226,2 +234,3 @@
   *UnitTestNameTests* (where at this time *UnitTestName* can be ADT, Analysis,
-  ExecutionEngine, JIT, Support, Transform, VMCore; see the subdirectories of
+  AsmParser, Bitcode, CodeGen, DebugInfoDWARF, DebugInfoPDB, ExecutionEngine,
+  IPO, IR, LineEditor, Linker, MCJIT, MC, Option, OrcJIT, ProfileData, Support,
----------------
chapuni wrote:
> 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".
I agree. I have replaced this list with a small subset.

================
Comment at: docs/CMake.rst:312
@@ -301,2 +311,3 @@
 
 **LLVM_LIT_TOOLS_DIR**:PATH
+  The path to GnuWin32 tools for tests. Valid on Windows host.  Defaults to
----------------
chapuni wrote:
> I introduced this section.
> 
> I took %PATH%, rather than PATH or $PATH to clarify this aims Windows platform.
I changed both PATH occurrences back to %PATH%.

================
Comment at: docs/CMake.rst:330
@@ +329,3 @@
+  LLVM projects Clang, lld, and Polly, respectively, relative to the
+  top-level source directory. Defaults to ``tools/clang``, ``tools/lld``, and
+  ``tools/polly``, respectively.  If the variable for an external project
----------------
chapuni wrote:
> 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.
I reworded this to incorporate the points you stated here.

================
Comment at: docs/CMake.rst:444
@@ -427,3 +443,3 @@
 
   $ make check
 
----------------
chapuni wrote:
> IIRC, we have a plan to let "check" obsoleted.
> I suggest to use "check-llvm" or "check-all" here.
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D11862