[PATCH] D105495: [clang] Make negative getLocWithOffset widening-safe.

[Tomas Matheson via Phabricator via cfe-commits]

tmatheson added a comment.

I'm not a huge fan of this as an API; it is not obvious what the function and parameters do without reading the comment or implementation (e.g. whether `Offset` is ignored if `NegOffset` is given, and what it means to pass offsets to both). It moves the subtraction logic from the call site, where it is meaningful, to inside the function where it is not. I also think it's the caller's responsibility to make sure what they pass in is correctly signed. I've put suggestions for how each call site could be updated without casting while keeping `getLocWithOffset` unchanged.



================
Comment at: clang/lib/AST/SelectorLocationsKind.cpp:41
     ++Len;
-  return ArgLoc.getLocWithOffset(-Len);
 }
----------------
These are probably fine as they were


================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:236
     if (Loc.isMacroID())
-      return Loc.getLocWithOffset(-SM.getFileOffset(Loc));
+      return Loc.getLocWithOffset(0, SM.getFileOffset(Loc));
     return SM.getLocForStartOfFile(SM.getFileID(Loc));
----------------
Seems like getFileOffset should now return ui64.


================
Comment at: clang/lib/Format/FormatTokenLexer.cpp:835
   SourceLocation WhitespaceStart =
-      FormatTok->Tok.getLocation().getLocWithOffset(-TrailingWhitespace);
+      FormatTok->Tok.getLocation().getLocWithOffset(0, TrailingWhitespace);
   FormatTok->IsFirst = IsFirstToken;
----------------
This probably would need a cast; widening `TrailingWhitespace` seems overkill.


================
Comment at: clang/lib/Lex/Lexer.cpp:530
   // Create a lexer starting at the beginning of this token.
-  SourceLocation LexerStartLoc = Loc.getLocWithOffset(-LocInfo.second);
+  SourceLocation LexerStartLoc = Loc.getLocWithOffset(0, LocInfo.second);
   Lexer TheLexer(LexerStartLoc, LangOpts, Buffer.data(), LexStart,
----------------
ditto `getDecomposedLoc`


================
Comment at: clang/lib/Lex/Lexer.cpp:568-570
   std::pair<FileID, unsigned> FileLocInfo = SM.getDecomposedLoc(FileLoc);
   std::pair<FileID, unsigned> BeginFileLocInfo =
       SM.getDecomposedLoc(BeginFileLoc);
----------------
Shouldn't the return type of `getDecomposedLoc` need updating at some stage? If so that would take care of this case


================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:174-179
   unsigned Offset = SMLoc.getPointer() - LBuf->getBufferStart();
 
   // Figure out which token that offset points into.
   const unsigned *TokOffsetPtr = llvm::lower_bound(AsmTokOffsets, Offset);
   unsigned TokIndex = TokOffsetPtr - AsmTokOffsets.begin();
   unsigned TokOffset = *TokOffsetPtr;
----------------



================
Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:1073-1074
   // Compute the column number of the end.
   unsigned EndColNo = SM.getExpansionColumnNumber(InstantiationEnd);
   unsigned OldEndColNo = EndColNo;
 
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105495