[PATCH] D21047: [clang] Documentation fixes for LibASTMatchersTutorial

2016-06-06 Thread Simon Whittaker via cfe-commits
simon.f.whittaker created this revision.
simon.f.whittaker added reviewers: silvas, bogner.
simon.f.whittaker added a subscriber: cfe-commits.

This patch fixes Bug 25583 as well as cleaning up some other problems in the 
LibASTMatchersTutorial.

I've updated the tutorial to use:

'./configure.py —bootstrap' rather than 'bootstrap.py' in the ninja buildstep
'ninja check-llvm' and 'ninja check-clang' rather than 'ninja check' and 'ninja 
clang-test' respectively
Clarified that no output will be produced with the first iteration of the 
program (Step 1: Create a ClangTool)
Cleaned up some English usage in the section on hasCondition(binaryOperator
Fixed the problem in the original bug report:
'Context->getSourceManager().isFromMainFile(FS->getForLoc()' should be 
'Context->getSourceManager().isInMainFile(FS->getForLoc()'

The only change I'm unsure about is changing:

'ninja install' to 'sudo ninja install' in the installation step. If I follow 
the instructions step-by-step then no special install directory is set and the 
default is /usr/local on OS X which requires sudo. As we've used sudo in 
similar circumstances above this then it makes sense to me.

This is my first contribution of any kind to Clang / LLVM (or even open source) 
so hope I have everything correct.

Thanks,

Simon

http://reviews.llvm.org/D21047

Files:
  docs/LibASTMatchersTutorial.rst

Index: docs/LibASTMatchersTutorial.rst
===
--- docs/LibASTMatchersTutorial.rst
+++ docs/LibASTMatchersTutorial.rst
@@ -40,7 +40,7 @@
   git clone https://github.com/martine/ninja.git
   cd ninja
   git checkout release
-  ./bootstrap.py
+  ./configure.py —bootstrap
   sudo cp ninja /usr/bin/
 
   cd ~/clang-llvm
@@ -59,9 +59,9 @@
   mkdir build && cd build
   cmake -G Ninja ../llvm -DLLVM_BUILD_TESTS=ON  # Enable tests; default is 
off.
   ninja
-  ninja check   # Test LLVM only.
-  ninja clang-test  # Test Clang only.
-  ninja install
+  ninja check-llvm   # Test LLVM only.
+  ninja check-clang  # Test Clang only.
+  sudo ninja install
 
 And we're live.
 
@@ -176,6 +176,8 @@
 them from a compilation database - there just aren't any options needed
 right now.
 
+As there are no syntax errors in the example no output will be produced.
+
 Intermezzo: Learn AST matcher basics
 
 
@@ -404,10 +406,10 @@
 hasLHS(declRefExpr(to(varDecl(hasType(isInteger()),
 hasRHS(expr(hasType(isInteger())
 
-Why? Because it doesn't work. Of the three loops provided in
+However this doesn't work, of the three loops provided in
 ``test-files/simple.cpp``, zero of them have a matching condition. A
 quick look at the AST dump of the first for loop, produced by the
-previous iteration of loop-convert, shows us the answer:
+previous iteration of loop-convert, shows us why:
 
 ::
 
@@ -498,7 +500,7 @@
 ASTContext *Context = Result.Context;
 const ForStmt *FS = Result.Nodes.getStmtAs("forLoop");
 // We do not want to convert header files!
-if (!FS || 
!Context->getSourceManager().isFromMainFile(FS->getForLoc()))
+if (!FS || !Context->getSourceManager().isInMainFile(FS->getForLoc()))
   return;
 const VarDecl *IncVar = Result.Nodes.getNodeAs("incVarName");
 const VarDecl *CondVar = 
Result.Nodes.getNodeAs("condVarName");


Index: docs/LibASTMatchersTutorial.rst
===
--- docs/LibASTMatchersTutorial.rst
+++ docs/LibASTMatchersTutorial.rst
@@ -40,7 +40,7 @@
   git clone https://github.com/martine/ninja.git
   cd ninja
   git checkout release
-  ./bootstrap.py
+  ./configure.py —bootstrap
   sudo cp ninja /usr/bin/
 
   cd ~/clang-llvm
@@ -59,9 +59,9 @@
   mkdir build && cd build
   cmake -G Ninja ../llvm -DLLVM_BUILD_TESTS=ON  # Enable tests; default is off.
   ninja
-  ninja check   # Test LLVM only.
-  ninja clang-test  # Test Clang only.
-  ninja install
+  ninja check-llvm   # Test LLVM only.
+  ninja check-clang  # Test Clang only.
+  sudo ninja install
 
 And we're live.
 
@@ -176,6 +176,8 @@
 them from a compilation database - there just aren't any options needed
 right now.
 
+As there are no syntax errors in the example no output will be produced.
+
 Intermezzo: Learn AST matcher basics
 
 
@@ -404,10 +406,10 @@
 hasLHS(declRefExpr(to(varDecl(hasType(isInteger()),
 hasRHS(expr(hasType(isInteger())
 
-Why? Because it doesn't work. Of the three loops provided in
+However this doesn't work, of the three loops provided in
 ``test-files/simple.cpp``, zero of them have a matching condition. A
 quick look at the AST dump of the first for loop, produced by the
-previous iteration of loop-convert, shows us the answer:
+previous iteration of loop-c

Re: [PATCH] D21047: [clang] Documentation fixes for LibASTMatchersTutorial

2016-06-06 Thread Simon Whittaker via cfe-commits
simon.f.whittaker added a reviewer: klimek.
simon.f.whittaker added a comment.

Added Manuel Klimek at Sean's suggestion.


http://reviews.llvm.org/D21047



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits