dexonsmith added inline comments.
================
Comment at: clang/lib/Lex/Lexer.cpp:2026
+
+ if (C == 0) {
NulCharacter = CurPtr-1;
----------------
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?
================
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
----------------
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.
https://reviews.llvm.org/D41423
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
