This is an archive of the discontinued LLVM Phabricator instance.

Fix recording preamble's conditional stack in skipped PP branches.
ClosedPublic

Authored by ilya-biryukov on Sep 11 2017, 10:37 AM.

Details

Summary

This fixes PR34547.
Lexer::LexEndOfFile handles recording of ConditionalStack for
preamble and reporting errors about unmatched conditionalal PP
directives.
However, SkipExcludedConditionalBlock contianed duplicated logic for
reporting errors and clearing ConditionalStack, but not for preamble
recording.

This fix removes error reporting logic from
SkipExcludedConditionalBlock, unmatched PP conditionals are now
reported inside Lexer::LexEndOfFile.

Event Timeline

ilya-biryukov created this revision.Sep 11 2017, 10:37 AM

Fixed description of the change.

nik added a subscriber: nik.Sep 12 2017, 12:45 AM

Fixes the reported issue, thanks!

I still run into another case that is not yet properly covered. The reported issue was extracted from the following case. If you prefer, I'll create a separate report for the remaining issues.

The follow code outlines the problems in the comments:

#ifdef MYCPLUSPLUS
extern "C" { // On parse this is skipped as expected. On reparse, this is surprisingly not skipped anymore.
#endif

#ifdef MYCPLUSPLUS
}
#endif

int main()
{
    return 0;
} // On reparse a diagnostic is added here: "expected '}' to match this '{' (line 2)"

Here are some c-index-test invocations for this to make this testable/reproducible ($FILE does not contain the above comments):

Do a parse and save the tokens:

% ./c-index-test -test-annotate-tokens=$FILE:1:1:7:1 $FILE > /tmp/1

No diagnostics are emitted as expected.

Do a parse and reparse and save also the tokens:

% CINDEXTEST_EDITING=1 ./c-index-test -test-annotate-tokens=$FILE:1:1:7:1 $FILE > /tmp/2
$FILE:12:2: error: expected '}'
Number FIX-ITs = 0
$FILE:2:12: note: to match this '{'
Number FIX-ITs = 0

Note that a new diagnostic is emitted on reparse at the bottom of the file (issue1).
Also, the first skipped source range is not skipped anymore on reparse (issue2):

% diff -u /tmp/{1,2}
--- /tmp/1	2017-09-12 09:33:42.210204021 +0200
+++ /tmp/2	2017-09-12 09:33:51.458334870 +0200
@@ -1,4 +1,3 @@
-Skipping: [1:2 - 3:7]
 Skipping: [5:2 - 7:7]
 Punctuation: "#" [1:1 - 1:2] preprocessing directive=
 Identifier: "ifdef" [1:2 - 1:7] preprocessing directive=
zsh: exit 1     colordiff -u /tmp/{1,2}
%
erikjv accepted this revision.Sep 12 2017, 1:02 AM
This revision is now accepted and ready to land.Sep 12 2017, 1:02 AM

I'd put/fix Nik's issue in a separate patch.

This revision was automatically updated to reflect the committed changes.

I'd put/fix Nik's issue in a separate patch.

Totally agree. It seems like a separate issue, though maybe related.

@nik, could you file a separate bug so that we won't forget about it?

nik added a comment.Sep 12 2017, 2:15 AM

@nik, could you file a separate bug so that we won't forget about it?

Done, it's https://bugs.llvm.org/show_bug.cgi?id=34570 .