Page MenuHomePhabricator

[Lexer] Report more precise skipped regions (PR34166)
ClosedPublic

Authored by vsk on Aug 11 2017, 7:41 PM.

Details

Summary

This patch teaches the preprocessor to report more precise source ranges for code that is skipped due to conditional directives.

The new behavior includes the '#' from the opening directive and the full text of the line containing the closing directive in the skipped area. This matches up clang's behavior (we don't IRGen the code between the closing "endif" and the end of a line).

This also affects the code coverage implementation. See llvm.org/PR34166 (this also happens to be rdar://problem/23224058).

Diff Detail

Event Timeline

vsk created this revision.Aug 11 2017, 7:41 PM
efriedma edited edge metadata.Aug 14 2017, 1:20 PM

I'm not sure this produces the right locations in general. Consider the following slightly evil testcase:

#\
if 0
#elif \
11\

int a;
#endif
vsk updated this revision to Diff 111113.Aug 14 2017, 7:41 PM

Thanks for the review. I've updated the patch so that we do better with "#\" directives.

efriedma added inline comments.Aug 15 2017, 12:20 PM
lib/Lex/PPDirectives.cpp
570

This doesn't really handle backslash-escaped newlines correctly. (Not likely to matter, I guess, but better to get it right while we're messing with it.)

Could we use CurPPLexer->getSourceLocation() or something like that, instead of trying to scan the line ourselves?

vsk updated this revision to Diff 111596.Aug 17 2017, 6:20 PM
vsk marked an inline comment as done.
  • Address Eli's comment.
lib/Lex/PPDirectives.cpp
570

Thanks again for the catch! CurPPLexer->getSourceLocation() does the job. I couldn't find an efficient solution which places the end of the skipped range at the end of the line containing the #endif. With getSourceLocation(), we always set the end location to the first column of the line after the #endif. I think this is OK (at least, it shouldn't affect coverage rendering).

I'd like to see someone more familiar with indexing comment on the effect there; otherwise LGTM.

arphaman added inline comments.
lib/Lex/PPDirectives.cpp
579

You'd have to update the pp-tests in clang-tools-extra too

test/Index/skipped-ranges.c
23–25

Some editor clients might want to grey out the skipped PP ranges. The new end location is not ideal as we wouldn't want to grey out the trailing comments after the #endif.

vsk added inline comments.Sep 8 2017, 1:33 PM
test/Index/skipped-ranges.c
23–25

It seems like the end location we want should be different for the coverage client and the indexer. I'll modify the callback so we pass along both end locations.

vsk updated this revision to Diff 114433.Sep 8 2017, 2:38 PM
  • Add an 'EndifLoc' parameter to the SourceRangeSkipped callback so that indexing clients can preserve their existing behavior.
  • I'll submit a follow-up patch which updates the pp-trace tests.
arphaman accepted this revision.Sep 11 2017, 3:15 AM

Thanks, LGTM

This revision is now accepted and ready to land.Sep 11 2017, 3:15 AM
This revision was automatically updated to reflect the committed changes.