vsapsai marked 2 inline comments as done.
vsapsai added inline comments.
================
Comment at: clang/lib/Lex/Lexer.cpp:2026
+
+ if (C == 0) {
NulCharacter = CurPtr-1;
----------------
dexonsmith wrote:
> vsapsai wrote:
> > dexonsmith wrote:
> > > Should this check still be skipped (in an `else if` of the `C == '\\'`
> > > check)?
> > I agree it is behaviour change. `NulCharacter` is used to warn if you have
> > null character in the string and I think it is worth warning even if it is
> > preceded by the backslash. Therefore I think this check shouldn't be
> > skipped after `C == '\\'` check. In practice I don't know how you can
> > create a file with null character in its name and use it in inclusion
> > directive.
> Can you add a test for the behaviour change then?
Added clang/test/Lexer/null-character-in-literal.c but struggling with
Phabricator to display it as text file, not as binary. Guess those null
characters can be confusing for some tools.
================
Comment at: clang/unittests/Lex/LexerTest.cpp:477
TEST_F(LexerTest, AvoidPastEndOfStringDereference) {
- std::vector<Token> LexedTokens = Lex(" // \\\n");
- EXPECT_TRUE(LexedTokens.empty());
+ EXPECT_TRUE(Lex(" // \\\n").empty());
+ // rdar://problem/35572754
----------------
dexonsmith wrote:
> vsapsai wrote:
> > dexonsmith wrote:
> > > To minimize the diff, please separate this change into an NFC commit
> > > ahead of time.
> > I plan to nominate this fix for inclusion in 6.0.0 release and having one
> > commit will be easier. It is not necessary to include NFC change in 6.0.0
> > release but it creates discrepancy that increases a chance of merge
> > conflicts.
> >
> > But I don't have strong opinion, just pointing out potential downsides with
> > merging the change to other branches.
> I think it's worth separating the NFC changes from behaviour changes, even if
> it means having to cherry-pick extra patches.
OK, done.
https://reviews.llvm.org/D41423
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
