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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
