[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

This revision was automatically updated to reflect the committed changes. Closed by commit rG6669dc09a441: [clang][NFC] Refactor printableTextForNextCharacter (authored by tbaeder). Changed prior to commit: https://reviews.llvm.org/D150843?vs=529170=530160#toc Repository: rG LLVM Github

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. > They are NFC, it's just not 100% only a refactoring, since it adds the new > ASCII-only case. Ok, thanks. Changes look good. I noticed one comment that appears to be incorrect;

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder updated this revision to Diff 529170. tbaeder marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150843/new/ https://reviews.llvm.org/D150843 Files: clang/lib/Frontend/TextDiagnostic.cpp Index: clang/lib/Frontend/TextDiagnostic.cpp

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added a comment. In D150843#4401189 , @tahonermann wrote: > The summary states that the changes are not quite NFC. In that case, we would > ideally have a test that demonstrates the changed behavior. Would adding such > a test be challenging?

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tahonermann added a comment. The changes look good to me. I suggested two minor edits to align with parameter name changes. The summary states that the changes are not quite NFC. In that case, we would ideally have a test that demonstrates the changed behavior. Would adding such a test be

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added a comment. Ping @tahonermann CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150843/new/ https://reviews.llvm.org/D150843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150843/new/ https://reviews.llvm.org/D150843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

cor3ntin added a comment. Give some time for @tahonermann to have an opportunity to review again, but otherwise this looks good to me. Thanks! Comment at: clang/lib/Frontend/TextDiagnostic.cpp:124-128 + if (CharSize == 1 && llvm::isLegalUTF8Sequence(Begin, End) && +

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder updated this revision to Diff 525094. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150843/new/ https://reviews.llvm.org/D150843 Files: clang/lib/Frontend/TextDiagnostic.cpp Index: clang/lib/Frontend/TextDiagnostic.cpp

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added inline comments. Comment at: clang/lib/Frontend/TextDiagnostic.cpp:124-128 + if (CharSize == 1 && llvm::isLegalUTF8Sequence(Begin, End) && + llvm::sys::locale::isPrint(*Begin)) { +++(*I); +return std::make_pair(SmallString<16>(Begin, End), true); + }

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added inline comments. Comment at: clang/lib/Frontend/TextDiagnostic.cpp:124-128 + if (CharSize == 1 && llvm::isLegalUTF8Sequence(Begin, End) && + llvm::sys::locale::isPrint(*Begin)) { +++(*I); +return std::make_pair(SmallString<16>(Begin, End), true); + }

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

cor3ntin added inline comments. Comment at: clang/lib/Frontend/TextDiagnostic.cpp:124-128 + if (CharSize == 1 && llvm::isLegalUTF8Sequence(Begin, End) && + llvm::sys::locale::isPrint(*Begin)) { +++(*I); +return std::make_pair(SmallString<16>(Begin, End), true); +

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added inline comments. Comment at: clang/lib/Frontend/TextDiagnostic.cpp:124-128 + if (CharSize == 1 && llvm::isLegalUTF8Sequence(Begin, End) && + llvm::sys::locale::isPrint(*Begin)) { +++(*I); +return std::make_pair(SmallString<16>(Begin, End), true); + }

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder updated this revision to Diff 525010. tbaeder marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150843/new/ https://reviews.llvm.org/D150843 Files: clang/lib/Frontend/TextDiagnostic.cpp Index: clang/lib/Frontend/TextDiagnostic.cpp

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added inline comments. Comment at: clang/lib/Frontend/TextDiagnostic.cpp:121 + unsigned CharSize = llvm::getNumBytesForUTF8(*Begin); + const unsigned char *End = Begin + CharSize; tahonermann wrote: > This could assign `End` to a location past the

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. Splitting this into two patches (one to do the renames, another to perform the other changes) would simplify review. Comment at:

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

cor3ntin added a comment. This generally looks good to me but i have a couple remarks Comment at: clang/lib/Frontend/TextDiagnostic.cpp:124-128 + if (CharSize == 1 && llvm::isLegalUTF8Sequence(Begin, End) && + llvm::sys::locale::isPrint(*Begin)) { +++(*I); +

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder added inline comments. Comment at: clang/lib/Frontend/TextDiagnostic.cpp:120 - begin = reinterpret_cast(&*(SourceLine.begin() + *i)); - end = begin + (SourceLine.size() - *i); - I don't know what this computation of `end` means, but from the debug

[PATCH] D150843: [clang][Diagnostics] Refactor printableTextForNextCharacter

tbaeder created this revision. tbaeder added reviewers: aaron.ballman, tahonermann. Herald added a project: All. tbaeder requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Rename parameters and local variables to start with an uppercase