diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -1007,14 +1007,30 @@ template static SourceLocation getEndCharLoc(const NodeTy *Node, const SourceManager &SM, const LangOptions &LangOpts) { - return Lexer::getLocForEndOfToken(Node->getEndLoc(), 1, SM, LangOpts); + unsigned TkLen = Lexer::MeasureTokenLength(Node->getEndLoc(), SM, LangOpts); + SourceLocation Loc = Node->getEndLoc().getLocWithOffset(TkLen - 1); + + // We expect `Loc` to be valid. The location is obtained by moving forward + // from the beginning of the token 'len(token)-1' characters. The file ID of + // the locations within a token must be consistent. + assert(Loc.isValid() && "Expected the source location of the last" + "character of an AST Node to be always valid"); + return Loc; } // Return the source location just past the last character of the AST `Node`. template static SourceLocation getPastLoc(const NodeTy *Node, const SourceManager &SM, const LangOptions &LangOpts) { - return Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts); + SourceLocation Loc = + Lexer::getLocForEndOfToken(Node->getEndLoc(), 0, SM, LangOpts); + + // We expect `Loc` to be valid as it is either associated to a file ID or + // it must be the end of a macro expansion. (see + // `Lexer::getLocForEndOfToken`) + assert(Loc.isValid() && "Expected the source location just past the last " + "character of an AST Node to be always valid"); + return Loc; } // Return text representation of an `Expr`. @@ -1179,9 +1195,25 @@ static FixItList populateInitializerFixItWithSpan(const Expr *Init, const ASTContext &Ctx, const StringRef UserFillPlaceHolder) { - FixItList FixIts{}; const SourceManager &SM = Ctx.getSourceManager(); const LangOptions &LangOpts = Ctx.getLangOpts(); + + // If `Init` has a constant value that is (or equivalent to) a + // NULL pointer, we use the default constructor to initialize the span + // object, i.e., a `std:span` variable declaration with no initializer. + // So the fix-it is just to remove the initializer. + if (Init->isNullPointerConstant( + std::remove_const_t(Ctx), + // FIXME: Why does this function not ask for `const ASTContext + // &`? It should. Maybe worth an NFC patch later. + Expr::NullPointerConstantValueDependence:: + NPC_ValueDependentIsNotNull)) { + SourceRange SR(Init->getBeginLoc(), getEndCharLoc(Init, SM, LangOpts)); + + return {FixItHint::CreateRemoval(SR)}; + } + + FixItList FixIts{}; std::string ExtentText = UserFillPlaceHolder.data(); StringRef One = "1"; @@ -1254,8 +1286,8 @@ FixItList InitFixIts = populateInitializerFixItWithSpan(Init, Ctx, UserFillPlaceHolder); - if (InitFixIts.empty()) - return {}; // Something wrong with fixing initializer, give up + assert(!InitFixIts.empty() && + "Unable to fix initializer of unsafe variable declaration"); // The loc right before the initializer: ReplacementLastLoc = Init->getBeginLoc().getLocWithOffset(-1); FixIts.insert(FixIts.end(), std::make_move_iterator(InitFixIts.begin()), @@ -1318,8 +1350,8 @@ // FIXME: For now we only check if the range (or the first token) is (part of) // a macro expansion. Ideally, we want to check for all tokens in the range. return llvm::any_of(FixIts, [](const FixItHint &Hint) { - auto BeginLoc = Hint.RemoveRange.getBegin(); - if (BeginLoc.isMacroID()) + auto Range = Hint.RemoveRange; + if (Range.getBegin().isMacroID() || Range.getEnd().isMacroID()) // If the range (or the first token) is (part of) a macro expansion: return true; return false; diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp @@ -130,6 +130,38 @@ tmp = (int) s[5]; } +void null_init() { +#define NULL 0 + int tmp; + int * my_null = 0; + int * p = 0; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span p" + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}} + int * g = NULL; // cannot handle fix-its involving macros for now + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]: + int * f = nullptr; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span f" + // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:{{^3}} + + // In case of value dependencies, we give up + int * q = my_null; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span q" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:20-[[@LINE-3]]:20}:", <# placeholder #>}" + int * r = my_null + 0; + // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::span r" + // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"{" + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:24-[[@LINE-3]]:24}:", <# placeholder #>}" + + tmp = p[5]; // `p[5]` will cause crash after `p` being transformed to be a `std::span` + tmp = q[5]; // Similar for the rests. + tmp = r[5]; + tmp = g[5]; + tmp = f[5]; +#undef NULL +} + + void unsupported_multi_decl(int * x) { int * p = x, * q = new int[10]; // CHECK-NOT: fix-it:"{{.*}}":[[@LINE-1]]