<div dir="ltr">Is it right to only change the behavior of this caller? Presumably other callers (like getSpellingColumnNumber, getExpansionColumnNumber, etc) probably want the same handling? Do any callers /not/ want this behavior?<br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 1, 2017 at 3:14 AM Erik Verbruggen via Phabricator via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">erikjv created this revision.<br>
<br>
Previously, the column number in a diagnostic would be the byte position<br>
in the line. This results in incorrect column numbers when a multi-byte<br>
UTF-8 character would be present in the input. By ignoring all bytes<br>
starting with 0b10.... the correct column number is created.<br>
<br>
This fixes PR21144.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D33765" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33765</a><br>
<br>
Files:<br>
  include/clang/Basic/SourceManager.h<br>
  lib/Basic/SourceManager.cpp<br>
  test/Misc/diag-utf8.cpp<br>
<br>
<br>
Index: test/Misc/diag-utf8.cpp<br>
===================================================================<br>
--- /dev/null<br>
+++ test/Misc/diag-utf8.cpp<br>
@@ -0,0 +1,11 @@<br>
+// RUN: not %clang_cc1 -fsyntax-only %s 2>&1 | FileCheck %s<br>
+<br>
+struct Foo { int member; };<br>
+<br>
+void f(Foo foo)<br>
+{<br>
+    "ideeen" << foo; // CHECK: {{.*[/\\]}}diag-utf8.cpp:7:14: error: invalid operands to binary expression ('const char *' and 'Foo')<br>
+    "ideëen" << foo; // CHECK: {{.*[/\\]}}diag-utf8.cpp:8:14: error: invalid operands to binary expression ('const char *' and 'Foo')<br>
+}<br>
+<br>
+<br>
Index: lib/Basic/SourceManager.cpp<br>
===================================================================<br>
--- lib/Basic/SourceManager.cpp<br>
+++ lib/Basic/SourceManager.cpp<br>
@@ -1055,11 +1055,22 @@<br>
   return Buffer->getBufferStart() + (CharDataInvalid? 0 : LocInfo.second);<br>
 }<br>
<br>
+static unsigned correctForMultiByteChars(const char *Buf, unsigned LineStart,<br>
+                                         unsigned Column)<br>
+{<br>
+    unsigned CorrectedColumn = Column;<br>
+    for (unsigned I = 0; I < Column; ++I) {<br>
+        if ((Buf[LineStart + I] & 0xc0) == 0x80)<br>
+            --CorrectedColumn;<br>
+    }<br>
+    return CorrectedColumn;<br>
+}<br>
<br>
 /// getColumnNumber - Return the column # for the specified file position.<br>
 /// this is significantly cheaper to compute than the line number.<br>
 unsigned SourceManager::getColumnNumber(FileID FID, unsigned FilePos,<br>
-                                        bool *Invalid) const {<br>
+                                        bool *Invalid,<br>
+                                        bool BytePosition) const {<br>
   bool MyInvalid = false;<br>
   llvm::MemoryBuffer *MemBuf = getBuffer(FID, &MyInvalid);<br>
   if (Invalid)<br>
@@ -1093,14 +1104,18 @@<br>
         if (Buf[FilePos - 1] == '\r' || Buf[FilePos - 1] == '\n')<br>
           --FilePos;<br>
       }<br>
-      return FilePos - LineStart + 1;<br>
+      unsigned Column = FilePos - LineStart + 1;<br>
+      return BytePosition ? Column : correctForMultiByteChars(Buf, LineStart,<br>
+                                                              Column);<br>
     }<br>
   }<br>
<br>
   unsigned LineStart = FilePos;<br>
   while (LineStart && Buf[LineStart-1] != '\n' && Buf[LineStart-1] != '\r')<br>
     --LineStart;<br>
-  return FilePos-LineStart+1;<br>
+  unsigned Column = FilePos - LineStart + 1;<br>
+  return BytePosition ? Column : correctForMultiByteChars(Buf, LineStart,<br>
+                                                          Column);<br>
 }<br>
<br>
 // isInvalid - Return the result of calling loc.isInvalid(), and<br>
@@ -1425,7 +1440,8 @@<br>
   unsigned LineNo = getLineNumber(LocInfo.first, LocInfo.second, &Invalid);<br>
   if (Invalid)<br>
     return PresumedLoc();<br>
-  unsigned ColNo  = getColumnNumber(LocInfo.first, LocInfo.second, &Invalid);<br>
+  unsigned ColNo  = getColumnNumber(LocInfo.first, LocInfo.second, &Invalid,<br>
+                                    false);<br>
   if (Invalid)<br>
     return PresumedLoc();<br>
<br>
Index: include/clang/Basic/SourceManager.h<br>
===================================================================<br>
--- include/clang/Basic/SourceManager.h<br>
+++ include/clang/Basic/SourceManager.h<br>
@@ -1275,7 +1275,8 @@<br>
   /// on a file sloc, so you must choose a spelling or expansion location<br>
   /// before calling this method.<br>
   unsigned getColumnNumber(FileID FID, unsigned FilePos,<br>
-                           bool *Invalid = nullptr) const;<br>
+                           bool *Invalid = nullptr,<br>
+                           bool BytePosition = true) const;<br>
   unsigned getSpellingColumnNumber(SourceLocation Loc,<br>
                                    bool *Invalid = nullptr) const;<br>
   unsigned getExpansionColumnNumber(SourceLocation Loc,<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>