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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to