[PATCH] Fix issue 5385: fixit for missing built-in includes

[Richard Smith]

Thank you for working on this, and sorry it took so long for you to get review feedback.

Most of my comments relate to coding style; please review our style rules at http://llvm.org/docs/CodingStandards.html. You can address many of these comments automatically by using clang-format.

Please only do the work of figuring out the right location for a new `#include` in the case where we've already decided to emit a diagnostic with a fixit, since it's not especially cheap. Please also factor this out into a separate function, for clarity and because we have other cases where we want to do the same thing (missing module imports, for instance).

================
Comment at: lib/Sema/SemaDecl.cpp:1686
@@ -1685,1 +1685,3 @@
 
+  //get the file buffer
+  FileID FileID = SourceMgr.getFileID(Loc);
----------------
In the Clang coding style, comments should start with a capital letter and end in a full stop, and there should be a space after the leading `//`.

================
Comment at: lib/Sema/SemaDecl.cpp:1687-1689
@@ +1686,5 @@
+  //get the file buffer
+  FileID FileID = SourceMgr.getFileID(Loc);
+  llvm::MemoryBuffer* FileBuf = SourceMgr.getBuffer(FileID);
+  SourceLocation LastIncludeLoc = SourceMgr.getLocForStartOfFile(FileID);
+
----------------
I don't think this will do the right thing if the use is within a macro; you will most likely need to pass `Loc` through `SourceMgr.getFileLoc` first.

================
Comment at: lib/Sema/SemaDecl.cpp:1688
@@ +1687,3 @@
+  FileID FileID = SourceMgr.getFileID(Loc);
+  llvm::MemoryBuffer* FileBuf = SourceMgr.getBuffer(FileID);
+  SourceLocation LastIncludeLoc = SourceMgr.getLocForStartOfFile(FileID);
----------------
The `*` should be on the right, not on the left, in Clang code.

================
Comment at: lib/Sema/SemaDecl.cpp:1692
@@ +1691,3 @@
+  //Find the Location of the last #include directive in the file
+  Lexer IncludeLexer(FileID,FileBuf,SourceMgr,LangOpts);
+  while (1) {
----------------
Please insert a space after each comma here.

================
Comment at: lib/Sema/SemaDecl.cpp:1694
@@ +1693,3 @@
+  while (1) {
+      Token Tok;
+      IncludeLexer.LexFromRawLexer(Tok);
----------------
Indent by 2 spaces, not 4.

================
Comment at: lib/Sema/SemaDecl.cpp:1698
@@ +1697,3 @@
+          break;
+      if (Tok.is(tok::hash)){
+          IncludeLexer.LexFromRawLexer(Tok);
----------------
You should also check `Tok.isAtStartOfLine()` here.

================
Comment at: lib/Sema/SemaDecl.cpp:1702
@@ +1701,3 @@
+                LastIncludeLoc = Tok.getLocation();
+      }
+  }
----------------
If you hit a token that isn't part of a preprocessor directive, you should stop scanning. (A `#include` half way through a file is probably a `.def` file or similar, and not a normal include, and in any case, your search will be more efficient.)

================
Comment at: lib/Sema/SemaDecl.cpp:1707
@@ +1706,3 @@
+  //Get the location of the line after the last #include directive
+  PresumedLoc PLoc = SourceMgr.getPresumedLoc(LastIncludeLoc);
+  SourceLocation FixitLoc = SourceMgr.translateLineCol(FileID,PLoc.getLine()+1,PLoc.getColumn());
----------------
You shouldn't use a presumed location here; the `FixitLoc` should refer to a position within the original source buffer, not into the presumed line numbers space.

================
Comment at: lib/Sema/SemaDecl.cpp:1708
@@ +1707,3 @@
+  PresumedLoc PLoc = SourceMgr.getPresumedLoc(LastIncludeLoc);
+  SourceLocation FixitLoc = SourceMgr.translateLineCol(FileID,PLoc.getLine()+1,PLoc.getColumn());
+  if (FixitLoc.isInvalid()){
----------------
Please stay wrap to an 80 column line limit.

================
Comment at: lib/Sema/SemaDecl.cpp:1709-1711
@@ +1708,5 @@
+  SourceLocation FixitLoc = SourceMgr.translateLineCol(FileID,PLoc.getLine()+1,PLoc.getColumn());
+  if (FixitLoc.isInvalid()){
+      FixitLoc = LastIncludeLoc;
+  }
+
----------------
No braces around a single-line `if` body.

http://reviews.llvm.org/D5770