[Clang][NFC] Clarify some SourceManager related code

[shafik]

Static analysis flagged the columns - 1 code, it was correct but the assumption was not obvious. I document the assumption w/ assertions.

While digging through related code I found getColumnNumber that looks wrong at first inspection and adding parentheses makes it clearer.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Shafik Yaghmour (shafik)

Changes

Static analysis flagged the columns - 1 code, it was correct but the assumption was not obvious. I document the assumption w/ assertions.

While digging through related code I found getColumnNumber that looks wrong at first inspection and adding parentheses makes it clearer.

---

Full diff: https://github.com/llvm/llvm-project/pull/153527.diff

2 Files Affected:

• (modified) clang/lib/Basic/SourceManager.cpp (+2-2)
• (modified) clang/lib/Frontend/TextDiagnostic.cpp (+2)

diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index 343c26e17236d..d8ec837f0f7b9 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -1171,14 +1171,14 @@ unsigned SourceManager::getColumnNumber(FileID FID, unsigned FilePos,
         if (Buf[FilePos - 1] == '\r' || Buf[FilePos - 1] == '\n')
           --FilePos;
       }
-      return FilePos - LineStart + 1;
+      return (FilePos - LineStart) + 1;
     }
   }
 
   unsigned LineStart = FilePos;
   while (LineStart && Buf[LineStart-1] != '\n' && Buf[LineStart-1] != '\r')
     --LineStart;
-  return FilePos-LineStart+1;
+  return (FilePos - LineStart) + 1;
 }
 
 // isInvalid - Return the result of calling loc.isInvalid(), and
diff --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp
index ccdd59da89bd1..cb35257003441 100644
--- a/clang/lib/Frontend/TextDiagnostic.cpp
+++ b/clang/lib/Frontend/TextDiagnostic.cpp
@@ -1095,6 +1095,8 @@ prepareAndFilterRanges(const SmallVectorImpl<CharSourceRange> &Ranges,
 
     unsigned StartColumn = SM.getExpansionColumnNumber(Begin);
     unsigned EndColumn = SM.getExpansionColumnNumber(End);
+    assert(StartColumn && "StartColumn has a value of 0");
+    assert(EndColumn && "EndColumn has a value of 0");
     if (R.isTokenRange())
       EndColumn += Lexer::MeasureTokenLength(End, SM, LangOpts);
 

[AaronBallman]

LGTM modulo Mariya's request for expanded assert explanations.

[Fznamznon]

as someone dealing with c/c++ I don't really get why 0 column number is invalid, but that is already clearer, thanks

[AaronBallman]

as someone dealing with c/c++ I don't really get why 0 column number is invalid, but that is already clearer, thanks

Lines and columns are one-based, but now I wonder if this change is correct because we do accept zero-based line numbers as a GNU extension. So it is possible to get a zero-based value, and we currently mess up with it: https://godbolt.org/z/8KMY3j7T9

Note how the second warning says <source>:4294967295:8:

@shafik does that code now cause an assertion?

[AaronBallman]

Requesting changes so we don't accidentally land; a new question came up.

[shafik]

as someone dealing with c/c++ I don't really get why 0 column number is invalid, but that is already clearer, thanks

Lines and columns are one-based, but now I wonder if this change is correct because we do accept zero-based line numbers as a GNU extension. So it is possible to get a zero-based value, and we currently mess up with it: https://godbolt.org/z/8KMY3j7T9

Note how the second warning says <source>:4294967295:8:

@shafik does that code now cause an assertion?

It does not assert, it would have failed testing we have several lines in tests that test that diagnostic "directive with zero argument is a GNU extension"

[shafik]

as someone dealing with c/c++ I don't really get why 0 column number is invalid, but that is already clearer, thanks

It looks like they are just using zero as invalid and then you need to subtract 1, we can see SourceManager::getSpellingColumnNumber, SourceManager::getExpansionColumnNumber and `SourceManager::getPresumedColumnNumber1 all return zero on invalid.

SourceManager also says they return zero for invalid:

llvm-project/clang/include/clang/Basic/SourceManager.h

Line 1424 in 7f20c6c

/// returns zero if the column number isn't known. This may only be called

I think in modern code we would use optional instead.

[AaronBallman]

LGTM again!