[PATCH] Frontend: Fix SourceColumnMap assertion failure on non-ascii characters.

Richard Smith richard at metafoo.co.uk
Mon Jan 5 07:18:15 PST 2015


LGTM, thank you! Sorry for the delay.


================
Comment at: test/Frontend/source-col-map.c:5-16
@@ +4,14 @@
+
+// There are two problems to be tested:
+//
+// 1. The assertion in startOfNextColumn() and startOfPreviousColumn()
+//    should not be raised when the byte index is greater than the column index
+//    since the non-ascii characters may use more than one bytes to store a
+//    character in a column.
+//
+// 2. The length of the caret line should be equal to the number of columns of
+//    source line, instead of the length of the source line.  Otherwise, the
+//    assertion in selectInterestingSourceRegion will be raised because the
+//    removed columns plus the kept columns are not greater than the max column,
+//    which means that we should not remove any column at all.
+
----------------
The comment here should explain what we're testing, not talk about the structure of the code under test -- that code might change, but this comment should not need updating when it does. This is an excellent writeup for the commit message, though.

http://reviews.llvm.org/D6746

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list