This is an archive of the discontinued LLVM Phabricator instance.

Fix issue 5385: fixit for missing built-in includes
Needs ReviewPublic

Authored by citisolo on Oct 14 2014, 1:40 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

When a built-in function was used without including the header a warning and the note 'please include the header' was generated without any fixit information . I have created a patch that adds the fixit advice.

Diff Detail

Event Timeline

citisolo updated this revision to Diff 14820.Oct 14 2014, 1:40 AM
citisolo retitled this revision from to Fix issue 5385: fixit for missing built-in includes .
citisolo updated this object.
citisolo edited the test plan for this revision. (Show Details)
citisolo added a subscriber: Unknown Object (MLST).
rsmith added a subscriber: rsmith.Oct 24 2014, 12:43 PM

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).

lib/Sema/SemaDecl.cpp
1686

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 //.

1687–1689

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.

1688

The * should be on the right, not on the left, in Clang code.

1692

Please insert a space after each comma here.

1694

Indent by 2 spaces, not 4.

1698

You should also check Tok.isAtStartOfLine() here.

1702

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.)

1707

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.

1708

Please stay wrap to an 80 column line limit.

1709–1711

No braces around a single-line if body.

citisolo updated this revision to Diff 15605.EditedOct 31 2014, 1:22 AM

made all the discussed changes to format as well as the below changes;
Lexer.h/.cpp : Added two new methods , findLastDirective and getLineAfterToken
SemaDecl.cpp: modified the code to use new methods
implicit-builtin-decl.c: minor modification to make test pass

citisolo updated this revision to Diff 15606.Oct 31 2014, 1:34 AM

tidied up some leftover comments

rsmith added inline comments.Nov 12 2014, 5:40 PM
include/clang/Lex/Lexer.h
444 ↗(On Diff #15606)

Remove the "return" here: "\returns false if ..."

451–453 ↗(On Diff #15606)

Please start comments with a capital letter and end them with a full stop.

lib/Lex/Lexer.cpp
1039 ↗(On Diff #15606)

I'm not sure it makes sense to check for a specific directive here. If a file has a mix of #include, #include_next, and #import, we want to find the location after all of them. I think you should make this function less general; specialize it to looking for the "right place to put an include", and just look for the location after the last include-like directive.

Generalizing this to any kind of directive actually makes it less useful, since it prevents us from adding smart heuristics (to deal with #ifs, to try to keep #includes in sorted order, and so on) that only make sense for includes.

1074–1081 ↗(On Diff #15606)

This doesn't cover all the cases. If a line ends in a \ (or ??/), or within a /*...*/ comment, we should keep scanning. One way to handle this would be to build a Lexer with whitespace retention enabled, and lex tokens until you find a token at the start of a line.

Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 3:55 PM