[PATCH] D142323: [Kaleidoscope] Update code snippets in text to match full code listings

[Shivam Gupta via Phabricator via llvm-commits]

xgupta added inline comments.


================
Comment at: llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl04.rst:300
 
+    static ExitOnError ExitOnErr;
+    ...
----------------
0xdc03 wrote:
> xgupta wrote:
> > we may remove this line.
> I thought it would be more useful to know what the type of `ExitOnErr` was, as this variable has never been referenced before, and it would help not having to scroll to the very end for that
Fine, I agree.


================
Comment at: llvm/docs/tutorial/MyFirstLanguageFrontend/LangImpl07.rst:583
+        // Note: If LLVM is built without RTTI (the default), a static_cast
+        //       must be used here.
         VariableExprAST *LHSE = dynamic_cast<VariableExprAST*>(LHS.get());
----------------
0xdc03 wrote:
> xgupta wrote:
> > Not sure about this, why not use static_cast then by default? @kazu might better review this.
> Yeah, I was a little confused at this point so I decided to stick with what the tutorial had before and mention how the code listing did it
I think the code listing is using static_cast, you should change dynamic_cast here to static_cast imo.


================
Comment at: llvm/examples/Kaleidoscope/Chapter9/toy.cpp:803
     // Make an anonymous proto.
-    auto Proto = std::make_unique<PrototypeAST>(FnLoc, "__anon_expr",
+    auto Proto = std::make_unique<PrototypeAST>(FnLoc, "main",
                                                  std::vector<std::string>());
----------------
0xdc03 wrote:
> xgupta wrote:
> >   // Make an anonymous proto.
> > This comment is better suited for  __anon_expr. we should keep it.
> Oh, I completely missed this comment! This was also one of the points where I wasn't sure what to do. Given that the start of the chapter explicitly says
> 
> > First we make our anonymous function that contains our top level statement be our “main”:
> 
> under section 9.3, I feel it'd be better to update the comment to say something like `// Make the top level expression be our "main" function`
Yeah, then it sounds reasonable to update it to main.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142323/new/

https://reviews.llvm.org/D142323