ilya-biryukov updated this revision to Diff 184588. ilya-biryukov marked 8 inline comments as done. ilya-biryukov added a comment. Herald added a project: clang. Herald added a subscriber: llvm-commits.
- Remove Dummy.cpp - Add halfOpenRangeTouches - Add a comment about file vs expansion locations - Move range manipulations with else and then to apply() - Remove test fixture, turn test member functions into free functions - Add checkTransform - Replace a null check for getCond() with an isValid() check for the corresponding location. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56611/new/ https://reviews.llvm.org/D56611 Files: clang-tools-extra/clangd/SourceCode.cpp clang-tools-extra/clangd/SourceCode.h clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt clang-tools-extra/clangd/refactor/tweaks/Dummy.cpp clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp clang-tools-extra/unittests/clangd/CMakeLists.txt clang-tools-extra/unittests/clangd/TweakTests.cpp
Index: clang-tools-extra/unittests/clangd/TweakTests.cpp =================================================================== --- /dev/null +++ clang-tools-extra/unittests/clangd/TweakTests.cpp @@ -0,0 +1,158 @@ +//===-- TweakTests.cpp ------------------------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "Annotations.h" +#include "SourceCode.h" +#include "TestTU.h" +#include "refactor/Tweak.h" +#include "clang/AST/Expr.h" +#include "clang/Rewrite/Core/Rewriter.h" +#include "clang/Tooling/Core/Replacement.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" +#include "llvm/Testing/Support/Error.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include <cassert> + +using llvm::Failed; +using llvm::HasValue; +using llvm::Succeeded; +using ::testing::IsEmpty; +using ::testing::Not; + +namespace clang { +namespace clangd { +namespace { + +std::string markRange(llvm::StringRef Code, Range R) { + size_t Begin = llvm::cantFail(positionToOffset(Code, R.start)); + size_t End = llvm::cantFail(positionToOffset(Code, R.end)); + assert(Begin <= End); + if (Begin == End) // Mark a single point. + return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str(); + // Mark a range. + return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) + + "]]" + Code.substr(End)) + .str(); +} + +void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) { + Annotations Code(Input); + ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size()) + << "no points of interest specified"; + TestTU TU; + TU.Filename = "foo.cpp"; + TU.Code = Code.code(); + + ParsedAST AST = TU.build(); + + auto CheckOver = [&](Range Selection) { + auto CursorLoc = llvm::cantFail(sourceLocationInMainFile( + AST.getASTContext().getSourceManager(), Selection.start)); + auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc}); + if (Available) + EXPECT_THAT_EXPECTED(T, Succeeded()) + << "code is " << markRange(Code.code(), Selection); + else + EXPECT_THAT_EXPECTED(T, Failed()) + << "code is " << markRange(Code.code(), Selection); + }; + for (auto P : Code.points()) + CheckOver(Range{P, P}); + for (auto R : Code.ranges()) + CheckOver(R); +} + +/// Checks action is available at every point and range marked in \p Input. +void checkAvailable(TweakID ID, llvm::StringRef Input) { + return checkAvailable(ID, Input, /*Available=*/true); +} + +/// Same as checkAvailable, but checks the action is not available. +void checkNotAvailable(TweakID ID, llvm::StringRef Input) { + return checkAvailable(ID, Input, /*Available=*/false); +} +llvm::Expected<std::string> apply(TweakID ID, llvm::StringRef Input) { + Annotations Code(Input); + Range SelectionRng; + if (Code.points().size() != 0) { + assert(Code.ranges().size() == 0 && + "both a cursor point and a selection range were specified"); + SelectionRng = Range{Code.point(), Code.point()}; + } else { + SelectionRng = Code.range(); + } + TestTU TU; + TU.Filename = "foo.cpp"; + TU.Code = Code.code(); + + ParsedAST AST = TU.build(); + auto CursorLoc = llvm::cantFail(sourceLocationInMainFile( + AST.getASTContext().getSourceManager(), SelectionRng.start)); + Tweak::Selection S = {Code.code(), AST, CursorLoc}; + + auto T = prepareTweak(ID, S); + if (!T) + return T.takeError(); + auto Replacements = (*T)->apply(S); + if (!Replacements) + return Replacements.takeError(); + return applyAllReplacements(Code.code(), *Replacements); +} + +void checkTransform(llvm::StringRef ID, llvm::StringRef Input, + llvm::StringRef Output) { + EXPECT_THAT_EXPECTED(apply(ID, Input), HasValue(Output)) + << "action id is" << ID; +} + +TEST(TweakTest, SwapIfBranches) { + llvm::StringLiteral ID = "SwapIfBranches"; + + checkAvailable(ID, R"cpp( + void test() { + ^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; } + } + )cpp"); + + checkNotAvailable(ID, R"cpp( + void test() { + if (true) {^return ^100;^ } else { ^continue^;^ } + } + )cpp"); + + llvm::StringLiteral Input = R"cpp( + void test() { + ^if (true) { return 100; } else { continue; } + } + )cpp"; + llvm::StringLiteral Output = R"cpp( + void test() { + if (true) { continue; } else { return 100; } + } + )cpp"; + checkTransform(ID, Input, Output); + + Input = R"cpp( + void test() { + ^if () { return 100; } else { continue; } + } + )cpp"; + Output = R"cpp( + void test() { + if () { continue; } else { return 100; } + } + )cpp"; + checkTransform(ID, Input, Output); +} + +} // namespace +} // namespace clangd +} // namespace clang Index: clang-tools-extra/unittests/clangd/CMakeLists.txt =================================================================== --- clang-tools-extra/unittests/clangd/CMakeLists.txt +++ clang-tools-extra/unittests/clangd/CMakeLists.txt @@ -45,8 +45,11 @@ TestTU.cpp ThreadingTests.cpp TraceTests.cpp + TweakTests.cpp URITests.cpp XRefsTests.cpp + + $<TARGET_OBJECTS:obj.clangDaemonTweaks> ) target_link_libraries(ClangdTests Index: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp @@ -0,0 +1,132 @@ +//===--- SwapIfBranches.cpp --------------------------------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +#include "ClangdUnit.h" +#include "Logger.h" +#include "SourceCode.h" +#include "refactor/Tweak.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Stmt.h" +#include "clang/Basic/LangOptions.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/SourceManager.h" +#include "clang/Lex/Lexer.h" +#include "clang/Tooling/Core/Replacement.h" +#include "llvm/ADT/None.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/Casting.h" +#include "llvm/Support/Error.h" + +namespace clang { +namespace clangd { +namespace { +/// Swaps the 'then' and 'else' branch of the if statement. +/// Before: +/// if (foo) { return 10; } else { continue; } +/// ^^^^^^^ ^^^^ +/// After: +/// if (foo) { continue; } else { return 10; } +class SwapIfBranches : public Tweak { +public: + TweakID id() const override final; + + bool prepare(const Selection &Inputs) override; + Expected<tooling::Replacements> apply(const Selection &Inputs) override; + std::string title() const override; + +private: + IfStmt *If = nullptr; +}; + +REGISTER_TWEAK(SwapIfBranches); + +class FindIfUnderCursor : public RecursiveASTVisitor<FindIfUnderCursor> { +public: + FindIfUnderCursor(ASTContext &Ctx, SourceLocation CursorLoc, IfStmt *&Result) + : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {} + + bool VisitIfStmt(IfStmt *If) { + // Check if the cursor is in the range of 'if (cond)'. + // FIXME: this does not contain the closing paren, add it too. + auto R = toHalfOpenFileRange( + Ctx.getSourceManager(), Ctx.getLangOpts(), + SourceRange(If->getIfLoc(), If->getCond()->getEndLoc().isValid() + ? If->getCond()->getEndLoc() + : If->getIfLoc())); + if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) { + Result = If; + return false; + } + // Check the range of 'else'. + R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(), + SourceRange(If->getElseLoc())); + if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) { + Result = If; + return false; + } + + return true; + } + +private: + ASTContext &Ctx; + SourceLocation CursorLoc; + IfStmt *&Result; +}; +} // namespace + +bool SwapIfBranches::prepare(const Selection &Inputs) { + auto &Ctx = Inputs.AST.getASTContext(); + FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx); + if (!If) + return false; + + // avoid dealing with single-statement brances, they require careful handling + // to avoid changing semantics of the code (i.e. dangling else). + if (!If->getThen() || !llvm::isa<CompoundStmt>(If->getThen()) || + !If->getElse() || !llvm::isa<CompoundStmt>(If->getElse())) + return false; + return true; +} + +Expected<tooling::Replacements> SwapIfBranches::apply(const Selection &Inputs) { + auto &Ctx = Inputs.AST.getASTContext(); + auto &SrcMgr = Ctx.getSourceManager(); + + auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(), + If->getThen()->getSourceRange()); + if (!ThenRng) + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Could not obtain range of the 'then' branch. Macros?"); + auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(), + If->getElse()->getSourceRange()); + if (!ElseRng) + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Could not obtain range of the 'else' branch. Macros?"); + + auto ThenCode = toSourceCode(SrcMgr, *ThenRng); + auto ElseCode = toSourceCode(SrcMgr, *ElseRng); + + tooling::Replacements Result; + if (auto Err = Result.add(tooling::Replacement(Ctx.getSourceManager(), + ThenRng->getBegin(), + ThenCode.size(), ElseCode))) + return std::move(Err); + if (auto Err = Result.add(tooling::Replacement(Ctx.getSourceManager(), + ElseRng->getBegin(), + ElseCode.size(), ThenCode))) + return std::move(Err); + return Result; +} + +std::string SwapIfBranches::title() const { return "Swap if branches"; } +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/refactor/tweaks/Dummy.cpp =================================================================== --- clang-tools-extra/clangd/refactor/tweaks/Dummy.cpp +++ /dev/null @@ -1,9 +0,0 @@ -//===--- Dummy.cpp -----------------------------------------------*- C++-*-===// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// -// Does nothing, only here to avoid cmake errors for empty libraries. \ No newline at end of file Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt +++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt @@ -8,6 +8,5 @@ # $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see # clangd/tool/CMakeLists.txt for an example. add_clang_library(clangDaemonTweaks OBJECT - Dummy.cpp # FIXME: to avoid CMake errors due to empty inputs, remove when a - # first tweak lands. + SwapIfBranches.cpp ) Index: clang-tools-extra/clangd/SourceCode.h =================================================================== --- clang-tools-extra/clangd/SourceCode.h +++ clang-tools-extra/clangd/SourceCode.h @@ -14,6 +14,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H #include "Protocol.h" #include "clang/Basic/Diagnostic.h" +#include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Format/Format.h" @@ -61,6 +62,45 @@ llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM, Position P); +/// Turns a token range into a half-open range and checks its correctness. +/// The resulting range will have only valid source location on both sides, both +/// of which are file locations. +/// +/// File locations always point to a particular offset in a file, i.e. they +/// never refer to a location inside a macro expansion. Turning locations from +/// macro expansions into file locations is ambiguous, this function calls +/// SourceManager::getFileLoc on both ends of \p R. +/// +/// User input (e.g. cursor position) is expressed as a file location, so this +/// function can be viewed as a way to normalize the ranges used in the clang +/// AST so that they are comparable with ranges coming from the user input. +llvm::Optional<SourceRange> toHalfOpenFileRange(const SourceManager &Mgr, + const LangOptions &LangOpts, + SourceRange R); + +/// Returns true iff all of the following conditions hold: +/// - start and end locations are valid, +/// - start and end locations are file locations from the same file +/// (i.e. expansion locations are not taken into account). +/// - start offset <= end offset. +/// FIXME: introduce a type for source range with this invariant. +bool isValidFileRange(const SourceManager &Mgr, SourceRange R); + +/// Returns true iff \p L is contained in \p R. +/// EXPECTS: isValidFileRange(R) == true, L is a file location. +bool halfOpenRangeContains(const SourceManager &Mgr, SourceRange R, + SourceLocation L); + +/// Returns true iff \p L is contained in \p R or \p L is equal to the end point +/// of \p R. +/// EXPECTS: isValidFileRange(R) == true, L is a file location. +bool halfOpenRangeTouches(const SourceManager &Mgr, SourceRange R, + SourceLocation L); + +/// Returns the source code covered by the source range. +/// EXPECTS: isValidFileRange(R) == true. +llvm::StringRef toSourceCode(const SourceManager &SM, SourceRange R); + // Converts a half-open clang source range to an LSP range. // Note that clang also uses closed source ranges, which this can't handle! Range halfOpenToRange(const SourceManager &SM, CharSourceRange R); Index: clang-tools-extra/clangd/SourceCode.cpp =================================================================== --- clang-tools-extra/clangd/SourceCode.cpp +++ clang-tools-extra/clangd/SourceCode.cpp @@ -11,6 +11,8 @@ #include "clang/AST/ASTContext.h" #include "clang/Basic/SourceManager.h" #include "clang/Lex/Lexer.h" +#include "llvm/ADT/None.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/Errc.h" #include "llvm/Support/Error.h" #include "llvm/Support/Path.h" @@ -141,6 +143,69 @@ return P; } +bool isValidFileRange(const SourceManager &Mgr, SourceRange R) { + if (!R.getBegin().isValid() || !R.getEnd().isValid()) + return false; + + FileID BeginFID; + size_t BeginOffset = 0; + std::tie(BeginFID, BeginOffset) = Mgr.getDecomposedLoc(R.getBegin()); + + FileID EndFID; + size_t EndOffset = 0; + std::tie(EndFID, EndOffset) = Mgr.getDecomposedLoc(R.getEnd()); + + return BeginFID.isValid() && BeginFID == EndFID && BeginOffset <= EndOffset; +} + +bool halfOpenRangeContains(const SourceManager &Mgr, SourceRange R, + SourceLocation L) { + assert(isValidFileRange(Mgr, R)); + + FileID BeginFID; + size_t BeginOffset = 0; + std::tie(BeginFID, BeginOffset) = Mgr.getDecomposedLoc(R.getBegin()); + size_t EndOffset = Mgr.getFileOffset(R.getEnd()); + + FileID LFid; + size_t LOffset; + std::tie(LFid, LOffset) = Mgr.getDecomposedLoc(L); + return BeginFID == LFid && BeginOffset <= LOffset && LOffset < EndOffset; +} + +bool halfOpenRangeTouches(const SourceManager &Mgr, SourceRange R, + SourceLocation L) { + return L == R.getEnd() || halfOpenRangeContains(Mgr, R, L); +} + +llvm::Optional<SourceRange> toHalfOpenFileRange(const SourceManager &Mgr, + const LangOptions &LangOpts, + SourceRange R) { + auto Begin = Mgr.getFileLoc(R.getBegin()); + if (Begin.isInvalid()) + return llvm::None; + auto End = Mgr.getFileLoc(R.getEnd()); + if (End.isInvalid()) + return llvm::None; + End = Lexer::getLocForEndOfToken(End, 0, Mgr, LangOpts); + + SourceRange Result(Begin, End); + if (!isValidFileRange(Mgr, Result)) + return llvm::None; + return Result; +} + +llvm::StringRef toSourceCode(const SourceManager &SM, SourceRange R) { + assert(isValidFileRange(SM, R)); + bool Invalid = false; + auto *Buf = SM.getBuffer(SM.getFileID(R.getBegin()), &Invalid); + assert(!Invalid); + + size_t BeginOffset = SM.getFileOffset(R.getBegin()); + size_t EndOffset = SM.getFileOffset(R.getEnd()); + return Buf->getBuffer().substr(BeginOffset, EndOffset - BeginOffset); +} + llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM, Position P) { llvm::StringRef Code = SM.getBuffer(SM.getMainFileID())->getBuffer(); @@ -169,8 +234,7 @@ return {Lines + 1, Offset - StartOfLine + 1}; } -std::pair<llvm::StringRef, llvm::StringRef> -splitQualifiedName(llvm::StringRef QName) { +std::pair<StringRef, StringRef> splitQualifiedName(StringRef QName) { size_t Pos = QName.rfind("::"); if (Pos == llvm::StringRef::npos) return {llvm::StringRef(), QName};
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits