[PATCH] D59977: [Lexer] Fix an off-by-one bug in Lexer::getAsCharRange() - NFC.

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 22 15:55:59 PDT 2019


NoQ updated this revision to Diff 196145.
NoQ added a comment.
Herald added subscribers: kadircet, arphaman, jkorous.

In D59977#1458051 <https://reviews.llvm.org/D59977#1458051>, @alexfh wrote:

> It looks like there's a number of users of this function beyond what you've mentioned:
>  clang-tidy/readability/IsolateDeclarationCheck.cpp:210


This one uses only char-ranges, not token-ranges, so it's unaffected by the change.

In D59977#1458051 <https://reviews.llvm.org/D59977#1458051>, @alexfh wrote:

> clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:43


They're using this function for counting the number of `::`s in `a::b::c::...::l`, and for them an off-by-one doesn't affect the answer.

In D59977#1458051 <https://reviews.llvm.org/D59977#1458051>, @alexfh wrote:

> clang-tidy/readability/NamespaceCommentCheck.cpp:106


Yup, that's one of the failing tests. They were expecting their `CharSourceRange` for `x::y {` (which starts with token `x` and ends with token `{`) to not include token `{`. I had to manually `rtrim()` it from the string. It sounds as if `CharSourceRange` is indeed supposed to be "inclusive" (include its last token), so the new behavior is correct:

  229 /// The underlying SourceRange can either specify the starting/ending character
  230 /// of the range, or it can specify the start of the range and the start of the
  231 /// last token of the range (a "token range").  In the token range case, the
  232 /// size of the last token must be measured to determine the actual end of the
  233 /// range.
  234 class CharSourceRange {



In D59977#1458051 <https://reviews.llvm.org/D59977#1458051>, @alexfh wrote:

> clang/lib/ARCMigrate/PlistReporter.cpp:110
>  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:192


These are addressed all together by the hack in `PlistSupport.h`.

---------

Also fixed `SelectionTests`; they were artificially adding `1` to the resulting range and i removed it.

I believe it should all work now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59977/new/

https://reviews.llvm.org/D59977

Files:
  clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
  clang-tools-extra/unittests/clangd/SelectionTests.cpp
  clang/include/clang/Basic/PlistSupport.h
  clang/include/clang/Lex/Lexer.h
  clang/unittests/Lex/LexerTest.cpp


Index: clang/unittests/Lex/LexerTest.cpp
===================================================================
--- clang/unittests/Lex/LexerTest.cpp
+++ clang/unittests/Lex/LexerTest.cpp
@@ -513,4 +513,23 @@
   EXPECT_EQ(String6, R"(a\\\n\n\n    \\\\b)");
 }
 
+TEST_F(LexerTest, CharRangeOffByOne) {
+  std::vector<Token> toks = Lex(R"(#define MOO 1
+    void foo() { MOO; })");
+  const Token &moo = toks[5];
+
+  EXPECT_EQ(getSourceText(moo, moo), "MOO");
+
+  SourceRange R{moo.getLocation(), moo.getLocation()};
+
+  EXPECT_TRUE(
+      Lexer::isAtStartOfMacroExpansion(R.getBegin(), SourceMgr, LangOpts));
+  EXPECT_TRUE(
+      Lexer::isAtEndOfMacroExpansion(R.getEnd(), SourceMgr, LangOpts));
+
+  CharSourceRange CR = Lexer::getAsCharRange(R, SourceMgr, LangOpts);
+
+  EXPECT_EQ(Lexer::getSourceText(CR, SourceMgr, LangOpts), "MOO"); // Was "MO".
+}
+
 } // anonymous namespace
Index: clang/include/clang/Lex/Lexer.h
===================================================================
--- clang/include/clang/Lex/Lexer.h
+++ clang/include/clang/Lex/Lexer.h
@@ -382,7 +382,7 @@
     SourceLocation End = getLocForEndOfToken(Range.getEnd(), 0, SM, LangOpts);
     return End.isInvalid() ? CharSourceRange()
                            : CharSourceRange::getCharRange(
-                                 Range.getBegin(), End.getLocWithOffset(-1));
+                                 Range.getBegin(), End);
   }
   static CharSourceRange getAsCharRange(CharSourceRange Range,
                                         const SourceManager &SM,
Index: clang/include/clang/Basic/PlistSupport.h
===================================================================
--- clang/include/clang/Basic/PlistSupport.h
+++ clang/include/clang/Basic/PlistSupport.h
@@ -127,7 +127,11 @@
   assert(R.isCharRange() && "cannot handle a token range");
   Indent(o, indent) << "<array>\n";
   EmitLocation(o, SM, R.getBegin(), FM, indent + 1);
-  EmitLocation(o, SM, R.getEnd(), FM, indent + 1);
+
+  // The ".getLocWithOffset(-1)" emulates the behavior of an off-by-one bug
+  // in Lexer that is already fixed. It is here for backwards compatibility
+  // even though it is incorrect.
+  EmitLocation(o, SM, R.getEnd().getLocWithOffset(-1), FM, indent + 1);
   Indent(o, indent) << "</array>\n";
 }
 
Index: clang-tools-extra/unittests/clangd/SelectionTests.cpp
===================================================================
--- clang-tools-extra/unittests/clangd/SelectionTests.cpp
+++ clang-tools-extra/unittests/clangd/SelectionTests.cpp
@@ -45,7 +45,7 @@
   CharSourceRange R =
       Lexer::getAsCharRange(SR, SM, AST.getASTContext().getLangOpts());
   return Range{offsetToPosition(Buffer, SM.getFileOffset(R.getBegin())),
-               offsetToPosition(Buffer, SM.getFileOffset(R.getEnd()) + 1)};
+               offsetToPosition(Buffer, SM.getFileOffset(R.getEnd()))};
 }
 
 std::string nodeKind(const SelectionTree::Node *N) {
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -106,7 +106,9 @@
       Lexer::getAsCharRange(SourceRange(NestedNamespaceBegin, LBracketLocation),
                             Sources, getLangOpts());
   StringRef NestedNamespaceName =
-      Lexer::getSourceText(TextRange, Sources, getLangOpts()).rtrim();
+      Lexer::getSourceText(TextRange, Sources, getLangOpts())
+          .rtrim('{') // Drop the { itself.
+          .rtrim();   // Drop any whitespace before it.
   bool IsNested = NestedNamespaceName.contains(':');
 
   if (IsNested)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59977.196145.patch
Type: text/x-patch
Size: 3711 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190422/5f38f5ca/attachment-0001.bin>


More information about the cfe-commits mailing list