<div dir="auto">Doh, that's a missing 'const' on RangeLess::operator(), which libstdc++ doesn't mind.<div dir="auto"><br></div><div dir="auto">I don't have my laptop with me, so feel free to revert. Though if it's easy for you, adding the const would be great :-)</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Dec 3, 2019, 8:55 PM Voss, Matthew <<a href="mailto:Matthew.Voss@sony.com">Matthew.Voss@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Sam,<br>
<br>
This commit fails to build on the PS4 windows bot. I'd like to revert this to unclog our CI and the buildbots. Could you take a look at the failure?<br>
<br>
<a href="http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/29554/steps/build-unified-tree/logs/stdio" rel="noreferrer noreferrer" target="_blank">http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/29554/steps/build-unified-tree/logs/stdio</a><br>
<br>
Thanks,<br>
Matthew<br>
<br>
> -----Original Message-----<br>
> From: cfe-commits <<a href="mailto:cfe-commits-bounces@lists.llvm.org" target="_blank" rel="noreferrer">cfe-commits-bounces@lists.llvm.org</a>> On Behalf Of Sam<br>
> McCall via cfe-commits<br>
> Sent: Tuesday, December 3, 2019 8:54 AM<br>
> To: <a href="mailto:cfe-commits@lists.llvm.org" target="_blank" rel="noreferrer">cfe-commits@lists.llvm.org</a><br>
> Subject: [clang] c9c714c - Reland [clangd] Rethink how SelectionTree deals<br>
> with macros and #includes.<br>
> <br>
> <br>
> Author: Sam McCall<br>
> Date: 2019-12-03T17:53:43+01:00<br>
> New Revision: c9c714c7054d555398c767cb39d7d97600b3d9d1<br>
> <br>
> URL: <a href="https://github.com/llvm/llvm-" rel="noreferrer noreferrer" target="_blank">https://github.com/llvm/llvm-</a><br>
> project/commit/c9c714c7054d555398c767cb39d7d97600b3d9d1<br>
> DIFF: <a href="https://github.com/llvm/llvm-" rel="noreferrer noreferrer" target="_blank">https://github.com/llvm/llvm-</a><br>
> project/commit/c9c714c7054d555398c767cb39d7d97600b3d9d1.diff<br>
> <br>
> LOG: Reland [clangd] Rethink how SelectionTree deals with macros and<br>
> #includes.<br>
> <br>
> This reverts commit 905b002c139f039a32ab9bf1fad63d745d12423f.<br>
> <br>
> Avoid tricky (and invalid) comparator for std::set.<br>
> <br>
> Added:<br>
> <br>
> <br>
> Modified:<br>
> clang-tools-extra/clangd/Selection.cpp<br>
> clang-tools-extra/clangd/Selection.h<br>
> clang-tools-extra/clangd/unittests/SelectionTests.cpp<br>
> clang-tools-extra/clangd/unittests/TweakTests.cpp<br>
> clang/include/clang/Tooling/Syntax/Tokens.h<br>
> clang/lib/Tooling/Syntax/Tokens.cpp<br>
> clang/unittests/Tooling/Syntax/TokensTest.cpp<br>
> <br>
> Removed:<br>
> <br>
> <br>
> <br>
> ##########################################################################<br>
> ######<br>
> diff --git a/clang-tools-extra/clangd/Selection.cpp b/clang-tools-<br>
> extra/clangd/Selection.cpp<br>
> index 5b29b916b33c..6ff5eabe566c 100644<br>
> --- a/clang-tools-extra/clangd/Selection.cpp<br>
> +++ b/clang-tools-extra/clangd/Selection.cpp<br>
> @@ -34,95 +34,289 @@ namespace {<br>
> using Node = SelectionTree::Node;<br>
> using ast_type_traits::DynTypedNode;<br>
> <br>
> -// Identifies which tokens are selected, and evaluates claims of source<br>
> ranges -// by AST nodes. Tokens may be claimed only once: first-come,<br>
> first-served.<br>
> -class SelectedTokens {<br>
> +// An IntervalSet maintains a set of disjoint subranges of an array.<br>
> +//<br>
> +// Initially, it contains the entire array.<br>
> +// [-----------------------------------------------------------<br>
> ]<br>
> +//<br>
> +// When a range is erased(), it will typically split the array in two.<br>
> +// Claim: [--------------------]<br>
> +// after: [----------------] [-------------------<br>
> ]<br>
> +//<br>
> +// erase() returns the segments actually erased. Given the state above:<br>
> +// Claim: [---------------------------------------]<br>
> +// Out: [---------] [------]<br>
> +// After: [-----] [-----------<br>
> ]<br>
> +//<br>
> +// It is used to track (expanded) tokens not yet associated with an AST<br>
> node.<br>
> +// On traversing an AST node, its token range is erased from the<br>
> unclaimed set.<br>
> +// The tokens actually removed are associated with that node, and<br>
> +hit-tested // against the selection to determine whether the node is<br>
> selected.<br>
> +template <typename T><br>
> +class IntervalSet {<br>
> +public:<br>
> + IntervalSet(llvm::ArrayRef<T> Range) { UnclaimedRanges.insert(Range);<br>
> +}<br>
> +<br>
> + // Removes the elements of Claim from the set, modifying or removing<br>
> + ranges // that overlap it.<br>
> + // Returns the continuous subranges of Claim that were actually<br>
> removed.<br>
> + llvm::SmallVector<llvm::ArrayRef<T>, 4> erase(llvm::ArrayRef<T> Claim)<br>
> {<br>
> + llvm::SmallVector<llvm::ArrayRef<T>, 4> Out;<br>
> + if (Claim.empty())<br>
> + return Out;<br>
> +<br>
> + // General case:<br>
> + // Claim: [-----------------]<br>
> + // UnclaimedRanges: [-A-] [-B-] [-C-] [-D-] [-E-] [-F-] [-G-]<br>
> + // Overlap: ^first ^second<br>
> + // Ranges C and D are fully included. Ranges B and E must be trimmed.<br>
> + auto Overlap = std::make_pair(<br>
> + UnclaimedRanges.lower_bound({Claim.begin(), Claim.begin()}), // C<br>
> + UnclaimedRanges.lower_bound({Claim.end(), Claim.end()})); // F<br>
> + // Rewind to cover B.<br>
> + if (Overlap.first != UnclaimedRanges.begin()) {<br>
> + --Overlap.first;<br>
> + // ...unless B isn't selected at all.<br>
> + if (Overlap.first->end() <= Claim.begin())<br>
> + ++Overlap.first;<br>
> + }<br>
> + if (Overlap.first == Overlap.second)<br>
> + return Out;<br>
> +<br>
> + // First, copy all overlapping ranges into the output.<br>
> + auto OutFirst = Out.insert(Out.end(), Overlap.first, Overlap.second);<br>
> + // If any of the overlapping ranges were sliced by the claim, split<br>
> them:<br>
> + // - restrict the returned range to the claimed part<br>
> + // - save the unclaimed part so it can be reinserted<br>
> + llvm::ArrayRef<T> RemainingHead, RemainingTail;<br>
> + if (Claim.begin() > OutFirst->begin()) {<br>
> + RemainingHead = {OutFirst->begin(), Claim.begin()};<br>
> + *OutFirst = {Claim.begin(), OutFirst->end()};<br>
> + }<br>
> + if (Claim.end() < Out.back().end()) {<br>
> + RemainingTail = {Claim.end(), Out.back().end()};<br>
> + Out.back() = {Out.back().begin(), Claim.end()};<br>
> + }<br>
> +<br>
> + // Erase all the overlapping ranges (invalidating all iterators).<br>
> + UnclaimedRanges.erase(Overlap.first, Overlap.second);<br>
> + // Reinsert ranges that were merely trimmed.<br>
> + if (!RemainingHead.empty())<br>
> + UnclaimedRanges.insert(RemainingHead);<br>
> + if (!RemainingTail.empty())<br>
> + UnclaimedRanges.insert(RemainingTail);<br>
> +<br>
> + return Out;<br>
> + }<br>
> +<br>
> +private:<br>
> + using TokenRange = llvm::ArrayRef<T>;<br>
> + struct RangeLess {<br>
> + bool operator()(llvm::ArrayRef<T> L, llvm::ArrayRef<T> R) {<br>
> + return L.begin() < R.begin();<br>
> + }<br>
> + };<br>
> +<br>
> + // Disjoint sorted unclaimed ranges of expanded tokens.<br>
> + std::set<llvm::ArrayRef<T>, RangeLess><br>
> + UnclaimedRanges;<br>
> +};<br>
> +<br>
> +// Sentinel value for the selectedness of a node where we've seen no<br>
> tokens yet.<br>
> +// This resolves to Unselected if no tokens are ever seen.<br>
> +// But Unselected + Complete -> Partial, while NoTokens + Complete --><br>
> Complete.<br>
> +// This value is never exposed publicly.<br>
> +constexpr SelectionTree::Selection NoTokens =<br>
> + static_cast<SelectionTree::Selection>(<br>
> + static_cast<unsigned char>(SelectionTree::Complete + 1));<br>
> +<br>
> +// Nodes start with NoTokens, and then use this function to aggregate<br>
> +the // selectedness as more tokens are found.<br>
> +void update(SelectionTree::Selection &Result, SelectionTree::Selection<br>
> +New) {<br>
> + if (New == NoTokens)<br>
> + return;<br>
> + if (Result == NoTokens)<br>
> + Result = New;<br>
> + else if (Result != New)<br>
> + // Can only be completely selected (or unselected) if all tokens are.<br>
> + Result = SelectionTree::Partial;<br>
> +}<br>
> +<br>
> +<br>
> +// SelectionTester can determine whether a range of tokens from the<br>
> +PP-expanded // stream (corresponding to an AST node) is considered<br>
> selected.<br>
> +//<br>
> +// When the tokens result from macro expansions, the appropriate tokens<br>
> +in the // main file are examined (macro invocation or args). Similarly<br>
> for #includes.<br>
> +//<br>
> +// It tests each token in the range (not just the endpoints) as<br>
> +contiguous // expanded tokens may not have contiguous spellings (with<br>
> macros).<br>
> +//<br>
> +// Non-token text, and tokens not modeled in the AST (comments,<br>
> +semicolons) // are ignored when determining selectedness.<br>
> +class SelectionTester {<br>
> public:<br>
> - SelectedTokens(llvm::ArrayRef<syntax::Token> Spelled, const<br>
> SourceManager &SM,<br>
> - unsigned SelBegin, unsigned SelEnd)<br>
> - : SelBegin(SelBegin), SelEnd(SelEnd) {<br>
> - // Extract bounds and selected-ness for all tokens spelled in the<br>
> file.<br>
> - Tokens.reserve(Spelled.size());<br>
> - for (const auto& Tok : Spelled) {<br>
> + // The selection is offsets [SelBegin, SelEnd) in SelFile.<br>
> + SelectionTester(const syntax::TokenBuffer &Buf, FileID SelFile,<br>
> + unsigned SelBegin, unsigned SelEnd, const SourceManager<br>
> &SM)<br>
> + : SelFile(SelFile), SM(SM) {<br>
> + // Find all tokens (partially) selected in the file.<br>
> + auto AllSpelledTokens = Buf.spelledTokens(SelFile);<br>
> + const syntax::Token *SelFirst =<br>
> + llvm::partition_point(AllSpelledTokens, [&](const syntax::Token<br>
> &Tok) {<br>
> + return SM.getFileOffset(Tok.endLocation()) <= SelBegin;<br>
> + });<br>
> + const syntax::Token *SelLimit = std::partition_point(<br>
> + SelFirst, AllSpelledTokens.end(), [&](const syntax::Token &Tok) {<br>
> + return SM.getFileOffset(Tok.location()) < SelEnd;<br>
> + });<br>
> + // Precompute selectedness and offset for selected spelled tokens.<br>
> + for (const syntax::Token *T = SelFirst; T < SelLimit; ++T) {<br>
> // As well as comments, don't count semicolons as real tokens.<br>
> // They're not properly claimed as expr-statement is missing from<br>
> the AST.<br>
> - if (Tok.kind() == tok::comment || Tok.kind() == tok::semi)<br>
> + if (T->kind() == tok::comment || T->kind() == tok::semi)<br>
> continue;<br>
> -<br>
> - Tokens.emplace_back();<br>
> - TokInfo &S = Tokens.back();<br>
> - S.StartOffset = SM.getFileOffset(Tok.location());<br>
> - S.EndOffset = S.StartOffset + Tok.length();<br>
> - if (S.StartOffset >= SelBegin && S.EndOffset <= SelEnd)<br>
> + SpelledTokens.emplace_back();<br>
> + Tok &S = SpelledTokens.back();<br>
> + S.Offset = SM.getFileOffset(T->location());<br>
> + if (S.Offset >= SelBegin && S.Offset + T->length() <= SelEnd)<br>
> S.Selected = SelectionTree::Complete;<br>
> - else if (S.EndOffset > SelBegin && S.StartOffset < SelEnd)<br>
> - S.Selected = SelectionTree::Partial;<br>
> else<br>
> - S.Selected = SelectionTree::Unselected;<br>
> - S.Claimed = false;<br>
> + S.Selected = SelectionTree::Partial;<br>
> }<br>
> }<br>
> <br>
> - // Associates any tokens overlapping [Begin, End) with an AST node.<br>
> - // Tokens that were already claimed by another AST node are not claimed<br>
> again.<br>
> - // Updates Result if the node is selected in the sense of<br>
> SelectionTree.<br>
> - void claim(unsigned Begin, unsigned End, SelectionTree::Selection<br>
> &Result) {<br>
> - assert(Begin <= End);<br>
> + // Test whether a consecutive range of tokens is selected.<br>
> + // The tokens are taken from the expanded token stream.<br>
> + SelectionTree::Selection<br>
> + test(llvm::ArrayRef<syntax::Token> ExpandedTokens) const {<br>
> + if (SpelledTokens.empty())<br>
> + return NoTokens;<br>
> + SelectionTree::Selection Result = NoTokens;<br>
> + while (!ExpandedTokens.empty()) {<br>
> + // Take consecutive tokens from the same context together for<br>
> efficiency.<br>
> + FileID FID = SM.getFileID(ExpandedTokens.front().location());<br>
> + auto Batch = ExpandedTokens.take_while([&](const syntax::Token &T)<br>
> {<br>
> + return SM.getFileID(T.location()) == FID;<br>
> + });<br>
> + assert(!Batch.empty());<br>
> + ExpandedTokens = ExpandedTokens.drop_front(Batch.size());<br>
> +<br>
> + update(Result, testChunk(FID, Batch));<br>
> + }<br>
> + return Result;<br>
> + }<br>
> <br>
> - // Fast-path for missing the selection entirely.<br>
> - if (Begin >= SelEnd || End <= SelBegin)<br>
> - return;<br>
> -<br>
> - // We will consider the range (at least partially) selected if it hit<br>
> any<br>
> - // selected and previously unclaimed token.<br>
> - bool ClaimedAnyToken = false;<br>
> - // The selection is (at most) partial if:<br>
> - // - any claimed token is partially selected<br>
> - // - any token in the range is unselected<br>
> - bool PartialSelection = false;<br>
> -<br>
> - // Find the first token that (maybe) overlaps the claimed range.<br>
> - auto Start = llvm::partition_point(Tokens, [&](const TokInfo &Tok) {<br>
> - return Tok.EndOffset <= Begin;<br>
> - });<br>
> - // Iterate over every token that overlaps the range.<br>
> - // Claim selected tokens, and update the two result flags.<br>
> - for (auto It = Start; It != Tokens.end() && It->StartOffset < End;<br>
> ++It) {<br>
> - if (It->Selected) {<br>
> - if (!It->Claimed) {<br>
> - // Token is selected, in the node's range, and unclaimed; claim<br>
> it.<br>
> - It->Claimed = true;<br>
> - ClaimedAnyToken = true;<br>
> - // If the token was only partially selected, so is the node.<br>
> - PartialSelection |= (It->Selected == SelectionTree::Partial);<br>
> - }<br>
> - } else {<br>
> - // If the node covers an unselected token, it's not completely<br>
> selected.<br>
> - PartialSelection = true;<br>
> + // Cheap check whether any of the tokens in R might be selected.<br>
> + // If it returns false, test() will return NoTokens or Unselected.<br>
> + // If it returns true, test() may return any value.<br>
> + bool mayHit(SourceRange R) const {<br>
> + if (SpelledTokens.empty())<br>
> + return false;<br>
> + auto B = SM.getDecomposedLoc(R.getBegin());<br>
> + auto E = SM.getDecomposedLoc(R.getEnd());<br>
> + if (B.first == SelFile && E.first == SelFile)<br>
> + if (E.second < SpelledTokens.front().Offset ||<br>
> + B.second > SpelledTokens.back().Offset)<br>
> + return false;<br>
> + return true;<br>
> + }<br>
> +<br>
> +private:<br>
> + // Hit-test a consecutive range of tokens from a single file ID.<br>
> + SelectionTree::Selection<br>
> + testChunk(FileID FID, llvm::ArrayRef<syntax::Token> Batch) const {<br>
> + assert(!Batch.empty());<br>
> + SourceLocation StartLoc = Batch.front().location();<br>
> + // There are several possible categories of FileID depending on how<br>
> the<br>
> + // preprocessor was used to generate these tokens:<br>
> + // main file, #included file, macro args, macro bodies.<br>
> + // We need to identify the main-file tokens that represent Batch, and<br>
> + // determine whether we want to exclusively claim them. Regular<br>
> tokens<br>
> + // represent one AST construct, but a macro invocation can represent<br>
> many.<br>
> +<br>
> + // Handle tokens written directly in the main file.<br>
> + if (FID == SelFile) {<br>
> + return testTokenRange(SM.getFileOffset(Batch.front().location()),<br>
> + SM.getFileOffset(Batch.back().location()));<br>
> + }<br>
> +<br>
> + // Handle tokens in another file #included into the main file.<br>
> + // Check if the #include is selected, but don't claim it exclusively.<br>
> + if (StartLoc.isFileID()) {<br>
> + for (SourceLocation Loc = Batch.front().location(); Loc.isValid();<br>
> + Loc = SM.getIncludeLoc(SM.getFileID(Loc))) {<br>
> + if (SM.getFileID(Loc) == SelFile)<br>
> + // FIXME: use whole #include directive, not just the filename<br>
> string.<br>
> + return testToken(SM.getFileOffset(Loc));<br>
> }<br>
> + return NoTokens;<br>
> }<br>
> <br>
> - // If some tokens were previously claimed (Result != Unselected), we<br>
> may<br>
> - // upgrade from Partial->Complete, even if no new tokens were<br>
> claimed.<br>
> - // Important for [[int a]].<br>
> - if (ClaimedAnyToken || Result) {<br>
> - Result = std::max(Result, PartialSelection ? SelectionTree::Partial<br>
> - :<br>
> SelectionTree::Complete);<br>
> + assert(StartLoc.isMacroID());<br>
> + // Handle tokens that were passed as a macro argument.<br>
> + SourceLocation ArgStart = SM.getTopMacroCallerLoc(StartLoc);<br>
> + if (SM.getFileID(ArgStart) == SelFile) {<br>
> + SourceLocation ArgEnd =<br>
> SM.getTopMacroCallerLoc(Batch.back().location());<br>
> + return testTokenRange(SM.getFileOffset(ArgStart),<br>
> + SM.getFileOffset(ArgEnd));<br>
> }<br>
> +<br>
> + // Handle tokens produced by non-argument macro expansion.<br>
> + // Check if the macro name is selected, don't claim it exclusively.<br>
> + auto Expansion = SM.getDecomposedExpansionLoc(StartLoc);<br>
> + if (Expansion.first == SelFile)<br>
> + // FIXME: also check ( and ) for function-like macros?<br>
> + return testToken(Expansion.second);<br>
> + else<br>
> + return NoTokens;<br>
> }<br>
> <br>
> -private:<br>
> - struct TokInfo {<br>
> - unsigned StartOffset;<br>
> - unsigned EndOffset;<br>
> + // Is the closed token range [Begin, End] selected?<br>
> + SelectionTree::Selection testTokenRange(unsigned Begin, unsigned End)<br>
> const {<br>
> + assert(Begin <= End);<br>
> + // Outside the selection entirely?<br>
> + if (End < SpelledTokens.front().Offset ||<br>
> + Begin > SpelledTokens.back().Offset)<br>
> + return SelectionTree::Unselected;<br>
> +<br>
> + // Compute range of tokens.<br>
> + auto B = llvm::partition_point(<br>
> + SpelledTokens, [&](const Tok &T) { return T.Offset < Begin; });<br>
> + auto E = std::partition_point(<br>
> + B, SpelledTokens.end(), [&](const Tok &T) { return T.Offset <=<br>
> + End; });<br>
> +<br>
> + // Aggregate selectedness of tokens in range.<br>
> + bool ExtendsOutsideSelection = Begin < SpelledTokens.front().Offset<br>
> ||<br>
> + End > SpelledTokens.back().Offset;<br>
> + SelectionTree::Selection Result =<br>
> + ExtendsOutsideSelection ? SelectionTree::Unselected : NoTokens;<br>
> + for (auto It = B; It != E; ++It)<br>
> + update(Result, It->Selected);<br>
> + return Result;<br>
> + }<br>
> +<br>
> + // Is the token at `Offset` selected?<br>
> + SelectionTree::Selection testToken(unsigned Offset) const {<br>
> + // Outside the selection entirely?<br>
> + if (Offset < SpelledTokens.front().Offset ||<br>
> + Offset > SpelledTokens.back().Offset)<br>
> + return SelectionTree::Unselected;<br>
> + // Find the token, if it exists.<br>
> + auto It = llvm::partition_point(<br>
> + SpelledTokens, [&](const Tok &T) { return T.Offset < Offset; });<br>
> + if (It != SpelledTokens.end() && It->Offset == Offset)<br>
> + return It->Selected;<br>
> + return NoTokens;<br>
> + }<br>
> +<br>
> + struct Tok {<br>
> + unsigned Offset;<br>
> SelectionTree::Selection Selected;<br>
> - bool Claimed;<br>
> - bool operator<(const TokInfo &Other) const {<br>
> - return StartOffset < Other.StartOffset;<br>
> - }<br>
> };<br>
> - std::vector<TokInfo> Tokens;<br>
> - unsigned SelBegin, SelEnd;<br>
> + std::vector<Tok> SpelledTokens;<br>
> + FileID SelFile;<br>
> + const SourceManager &SM;<br>
> };<br>
> <br>
> // Show the type of a node for debugging.<br>
> @@ -195,16 +389,6 @@ class SelectionVisitor : public<br>
> RecursiveASTVisitor<SelectionVisitor> {<br>
> V.TraverseAST(AST);<br>
> assert(V.Stack.size() == 1 && "Unpaired push/pop?");<br>
> assert(V.Stack.top() == &V.Nodes.front());<br>
> - // We selected TUDecl if tokens were unclaimed (or the file is<br>
> empty).<br>
> - SelectionTree::Selection UnclaimedTokens = SelectionTree::Unselected;<br>
> - V.Claimed.claim(Begin, End, UnclaimedTokens);<br>
> - if (UnclaimedTokens || V.Nodes.size() == 1) {<br>
> - StringRef FileContent = AST.getSourceManager().getBufferData(File);<br>
> - // Don't require the trailing newlines to be selected.<br>
> - bool SelectedAll = Begin == 0 && End >= FileContent.rtrim().size();<br>
> - V.Stack.top()->Selected =<br>
> - SelectedAll ? SelectionTree::Complete : SelectionTree::Partial;<br>
> - }<br>
> return std::move(V.Nodes);<br>
> }<br>
> <br>
> @@ -289,11 +473,8 @@ class SelectionVisitor : public<br>
> RecursiveASTVisitor<SelectionVisitor> { #ifndef NDEBUG<br>
> PrintPolicy(PP),<br>
> #endif<br>
> - Claimed(Tokens.spelledTokens(SelFile), SM, SelBegin, SelEnd),<br>
> - SelFile(SelFile),<br>
> - SelBeginTokenStart(SM.getFileOffset(Lexer::GetBeginningOfToken(<br>
> - SM.getComposedLoc(SelFile, SelBegin), SM, LangOpts))),<br>
> - SelEnd(SelEnd) {<br>
> + TokenBuf(Tokens), SelChecker(Tokens, SelFile, SelBegin, SelEnd,<br>
> SM),<br>
> + UnclaimedExpandedTokens(Tokens.expandedTokens()) {<br>
> // Ensure we have a node for the TU decl, regardless of traversal<br>
> scope.<br>
> Nodes.emplace_back();<br>
> Nodes.back().ASTNode =<br>
> DynTypedNode::create(*AST.getTranslationUnitDecl());<br>
> @@ -346,18 +527,12 @@ class SelectionVisitor : public<br>
> RecursiveASTVisitor<SelectionVisitor> {<br>
> // don't intersect the selection may be recursively skipped.<br>
> bool canSafelySkipNode(const DynTypedNode &N) {<br>
> SourceRange S = N.getSourceRange();<br>
> - auto B = SM.getDecomposedLoc(S.getBegin());<br>
> - auto E = SM.getDecomposedLoc(S.getEnd());<br>
> - // Node lies in a macro expansion?<br>
> - if (B.first != SelFile || E.first != SelFile)<br>
> - return false;<br>
> - // Node intersects selection tokens?<br>
> - if (B.second < SelEnd && E.second >= SelBeginTokenStart)<br>
> - return false;<br>
> - // Otherwise, allow skipping over the node.<br>
> - dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());<br>
> - dlog("{1}skipped range = {0}", S.printToString(SM), indent(1));<br>
> - return true;<br>
> + if (!SelChecker.mayHit(S)) {<br>
> + dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());<br>
> + dlog("{1}skipped range = {0}", S.printToString(SM), indent(1));<br>
> + return true;<br>
> + }<br>
> + return false;<br>
> }<br>
> <br>
> // There are certain nodes we want to treat as leaves in the<br>
> SelectionTree, @@ -377,11 +552,9 @@ class SelectionVisitor : public<br>
> RecursiveASTVisitor<SelectionVisitor> {<br>
> Nodes.emplace_back();<br>
> Nodes.back().ASTNode = std::move(Node);<br>
> Nodes.back().Parent = Stack.top();<br>
> + Nodes.back().Selected = NoTokens;<br>
> Stack.push(&Nodes.back());<br>
> claimRange(Early, Nodes.back().Selected);<br>
> - // Early hit detection never selects the whole node.<br>
> - if (Nodes.back().Selected)<br>
> - Nodes.back().Selected = SelectionTree::Partial;<br>
> }<br>
> <br>
> // Pops a node off the ancestor stack, and finalizes it. Pairs with<br>
> push().<br>
> @@ -390,6 +563,8 @@ class SelectionVisitor : public<br>
> RecursiveASTVisitor<SelectionVisitor> {<br>
> Node &N = *Stack.top();<br>
> dlog("{1}pop: {0}", printNodeToString(N.ASTNode, PrintPolicy),<br>
> indent(-1));<br>
> claimRange(N.ASTNode.getSourceRange(), N.Selected);<br>
> + if (N.Selected == NoTokens)<br>
> + N.Selected = SelectionTree::Unselected;<br>
> if (N.Selected || !N.Children.empty()) {<br>
> // Attach to the tree.<br>
> N.Parent->Children.push_back(&N); @@ -424,31 +599,12 @@ class<br>
> SelectionVisitor : public RecursiveASTVisitor<SelectionVisitor> {<br>
> // This is usually called from pop(), so we can take children into<br>
> account.<br>
> // The existing state of Result is relevant (early/late claims can<br>
> interact).<br>
> void claimRange(SourceRange S, SelectionTree::Selection &Result) {<br>
> - if (!S.isValid())<br>
> - return;<br>
> - // toHalfOpenFileRange() allows selection of constructs in macro<br>
> args. e.g:<br>
> - // #define LOOP_FOREVER(Body) for(;;) { Body }<br>
> - // void IncrementLots(int &x) {<br>
> - // LOOP_FOREVER( ++x; )<br>
> - // }<br>
> - // Selecting "++x" or "x" will do the right thing.<br>
> - auto Range = toHalfOpenFileRange(SM, LangOpts, S);<br>
> - assert(Range && "We should be able to get the File Range");<br>
> - dlog("{1}claimRange: {0}", Range->printToString(SM), indent());<br>
> - auto B = SM.getDecomposedLoc(Range->getBegin());<br>
> - auto E = SM.getDecomposedLoc(Range->getEnd());<br>
> - // Otherwise, nodes in macro expansions can't be selected.<br>
> - if (B.first != SelFile || E.first != SelFile)<br>
> - return;<br>
> - // Attempt to claim the remaining range. If there's nothing to claim,<br>
> only<br>
> - // children were selected.<br>
> - Claimed.claim(B.second, E.second, Result);<br>
> - if (Result)<br>
> - dlog("{1}hit selection: {0}",<br>
> - SourceRange(SM.getComposedLoc(B.first, B.second),<br>
> - SM.getComposedLoc(E.first, E.second))<br>
> - .printToString(SM),<br>
> - indent());<br>
> + for (const auto &ClaimedRange :<br>
> + UnclaimedExpandedTokens.erase(TokenBuf.expandedTokens(S)))<br>
> + update(Result, SelChecker.test(ClaimedRange));<br>
> +<br>
> + if (Result && Result != NoTokens)<br>
> + dlog("{1}hit selection: {0}", S.printToString(SM), indent());<br>
> }<br>
> <br>
> std::string indent(int Offset = 0) {<br>
> @@ -463,17 +619,11 @@ class SelectionVisitor : public<br>
> RecursiveASTVisitor<SelectionVisitor> { #ifndef NDEBUG<br>
> const PrintingPolicy &PrintPolicy;<br>
> #endif<br>
> + const syntax::TokenBuffer &TokenBuf;<br>
> std::stack<Node *> Stack;<br>
> - SelectedTokens Claimed;<br>
> + SelectionTester SelChecker;<br>
> + IntervalSet<syntax::Token> UnclaimedExpandedTokens;<br>
> std::deque<Node> Nodes; // Stable pointers as we add more nodes.<br>
> - FileID SelFile;<br>
> - // If the selection start slices a token in half, the beginning of that<br>
> token.<br>
> - // This is useful for checking whether the end of a token range<br>
> overlaps<br>
> - // the selection: range.end < SelBeginTokenStart is equivalent to<br>
> - // range.end + measureToken(range.end) < SelBegin (assuming range.end<br>
> points<br>
> - // to a token), and it saves a lex every time.<br>
> - unsigned SelBeginTokenStart;<br>
> - unsigned SelEnd;<br>
> };<br>
> <br>
> } // namespace<br>
> <br>
> diff --git a/clang-tools-extra/clangd/Selection.h b/clang-tools-<br>
> extra/clangd/Selection.h<br>
> index 9bcb9d5fb01f..a7050c49be6b 100644<br>
> --- a/clang-tools-extra/clangd/Selection.h<br>
> +++ b/clang-tools-extra/clangd/Selection.h<br>
> @@ -76,7 +76,7 @@ class SelectionTree {<br>
> unsigned Start, unsigned End);<br>
> <br>
> // Describes to what extent an AST node is covered by the selection.<br>
> - enum Selection {<br>
> + enum Selection : unsigned char {<br>
> // The AST node owns no characters covered by the selection.<br>
> // Note that characters owned by children don't count:<br>
> // if (x == 0) scream();<br>
> <br>
> diff --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp<br>
> b/clang-tools-extra/clangd/unittests/SelectionTests.cpp<br>
> index 6f4ccd88b978..ec9fd4185d94 100644<br>
> --- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp<br>
> +++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp<br>
> @@ -134,6 +134,15 @@ TEST(SelectionTest, CommonAncestor) {<br>
> )cpp",<br>
> "IfStmt",<br>
> },<br>
> + {<br>
> + R"cpp(<br>
> + int x(int);<br>
> + #define M(foo) x(foo)<br>
> + int a = 42;<br>
> + int b = M([[^a]]);<br>
> + )cpp",<br>
> + "DeclRefExpr",<br>
> + },<br>
> {<br>
> R"cpp(<br>
> void foo();<br>
> @@ -378,6 +387,7 @@ TEST(SelectionTest, Selected) {<br>
> $C[[return]];<br>
> }]] else [[{^<br>
> }]]]]<br>
> + char z;<br>
> }<br>
> )cpp",<br>
> R"cpp(<br>
> @@ -386,10 +396,10 @@ TEST(SelectionTest, Selected) {<br>
> void foo(^$C[[unique_ptr<$C[[unique_ptr<$C[[int]]>]]>]]^ a) {}<br>
> )cpp",<br>
> R"cpp(int a = [[5 >^> 1]];)cpp",<br>
> - R"cpp([[<br>
> + R"cpp(<br>
> #define ECHO(X) X<br>
> - ECHO(EC^HO([[$C[[int]]) EC^HO(a]]));<br>
> - ]])cpp",<br>
> + ECHO(EC^HO($C[[int]]) EC^HO(a));<br>
> + )cpp",<br>
> R"cpp( $C[[^$C[[int]] a^]]; )cpp",<br>
> R"cpp( $C[[^$C[[int]] a = $C[[5]]^]]; )cpp",<br>
> };<br>
> @@ -428,6 +438,56 @@ TEST(SelectionTest, PathologicalPreprocessor) {<br>
> EXPECT_EQ("WhileStmt", T.commonAncestor()->Parent->kind());<br>
> }<br>
> <br>
> +TEST(SelectionTest, IncludedFile) {<br>
> + const char *Case = R"cpp(<br>
> + void test() {<br>
> +#include "Exp^and.inc"<br>
> + break;<br>
> + }<br>
> + )cpp";<br>
> + Annotations Test(Case);<br>
> + auto TU = TestTU::withCode(Test.code());<br>
> + TU.AdditionalFiles["Expand.inc"] = "while(1)\n";<br>
> + auto AST = TU.build();<br>
> + auto T = makeSelectionTree(Case, AST);<br>
> +<br>
> + EXPECT_EQ("WhileStmt", T.commonAncestor()->kind()); }<br>
> +<br>
> +TEST(SelectionTest, MacroArgExpansion) {<br>
> + // If a macro arg is expanded several times, we consider them all<br>
> selected.<br>
> + const char *Case = R"cpp(<br>
> + int mul(int, int);<br>
> + #define SQUARE(X) mul(X, X);<br>
> + int nine = SQUARE(^3);<br>
> + )cpp";<br>
> + Annotations Test(Case);<br>
> + auto AST = TestTU::withCode(Test.code()).build();<br>
> + auto T = makeSelectionTree(Case, AST);<br>
> + // Unfortunately, this makes the common ancestor the CallExpr...<br>
> + // FIXME: hack around this by picking one?<br>
> + EXPECT_EQ("CallExpr", T.commonAncestor()->kind());<br>
> + EXPECT_FALSE(T.commonAncestor()->Selected);<br>
> + EXPECT_EQ(2u, T.commonAncestor()->Children.size());<br>
> + for (const auto* N : T.commonAncestor()->Children) {<br>
> + EXPECT_EQ("IntegerLiteral", N->kind());<br>
> + EXPECT_TRUE(N->Selected);<br>
> + }<br>
> +<br>
> + // Verify that the common assert() macro doesn't suffer from this.<br>
> + // (This is because we don't associate the stringified token with the<br>
> arg).<br>
> + Case = R"cpp(<br>
> + void die(const char*);<br>
> + #define assert(x) (x ? (void)0 : die(#x)<br>
> + void foo() { assert(^42); }<br>
> + )cpp";<br>
> + Test = Annotations(Case);<br>
> + AST = TestTU::withCode(Test.code()).build();<br>
> + T = makeSelectionTree(Case, AST);<br>
> +<br>
> + EXPECT_EQ("IntegerLiteral", T.commonAncestor()->kind()); }<br>
> +<br>
> TEST(SelectionTest, Implicit) {<br>
> const char* Test = R"cpp(<br>
> struct S { S(const char*); };<br>
> <br>
> diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-<br>
> tools-extra/clangd/unittests/TweakTests.cpp<br>
> index 4e481241acd8..dc7699904019 100644<br>
> --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp<br>
> +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp<br>
> @@ -269,7 +269,7 @@ TEST_F(ExtractVariableTest, Test) {<br>
> EXPECT_UNAVAILABLE(UnavailableCases);<br>
> <br>
> // vector of pairs of input and output strings<br>
> - const std::vector<std::pair<llvm::StringLiteral, llvm::StringLiteral>><br>
> + const std::vector<std::pair<std::string, std::string>><br>
> InputOutputs = {<br>
> // extraction from variable declaration/assignment<br>
> {R"cpp(void varDecl() {<br>
> @@ -321,17 +321,10 @@ TEST_F(ExtractVariableTest, Test) {<br>
> if(1)<br>
> LOOP(5 + [[3]])<br>
> })cpp",<br>
> - /*FIXME: It should be extracted like this. SelectionTree needs<br>
> to be<br>
> - * fixed for macros.<br>
> R"cpp(#define LOOP(x) while (1) {a = x;}<br>
> - void f(int a) {<br>
> - auto dummy = 3; if(1)<br>
> - LOOP(5 + dummy)<br>
> - })cpp"},*/<br>
> - R"cpp(#define LOOP(x) while (1) {a = x;}<br>
> void f(int a) {<br>
> - auto dummy = LOOP(5 + 3); if(1)<br>
> - dummy<br>
> + auto dummy = 3; if(1)<br>
> + LOOP(5 + dummy)<br>
> })cpp"},<br>
> {R"cpp(#define LOOP(x) do {x;} while(1);<br>
> void f(int a) {<br>
> @@ -644,13 +637,18 @@ void f(const int c) {<br>
> )cpp";<br>
> EXPECT_EQ(apply(TemplateFailInput), "unavailable");<br>
> <br>
> - // FIXME: This should be extractable after selectionTree works<br>
> correctly for<br>
> - // macros (currently it doesn't select anything for the following case)<br>
> - std::string MacroFailInput = R"cpp(<br>
> + std::string MacroInput = R"cpp(<br>
> #define F(BODY) void f() { BODY }<br>
> F ([[int x = 0;]])<br>
> )cpp";<br>
> - EXPECT_EQ(apply(MacroFailInput), "unavailable");<br>
> + std::string MacroOutput = R"cpp(<br>
> + #define F(BODY) void f() { BODY }<br>
> + void extracted() {<br>
> +int x = 0;<br>
> +}<br>
> +F (extracted();)<br>
> + )cpp";<br>
> + EXPECT_EQ(apply(MacroInput), MacroOutput);<br>
> <br>
> // Shouldn't crash.<br>
> EXPECT_EQ(apply("void f([[int a]]);"), "unavailable");<br>
> <br>
> diff --git a/clang/include/clang/Tooling/Syntax/Tokens.h<br>
> b/clang/include/clang/Tooling/Syntax/Tokens.h<br>
> index 301432d3888b..6f4d0e0c050a 100644<br>
> --- a/clang/include/clang/Tooling/Syntax/Tokens.h<br>
> +++ b/clang/include/clang/Tooling/Syntax/Tokens.h<br>
> @@ -175,6 +175,7 @@ class TokenBuffer {<br>
> /// All tokens produced by the preprocessor after all macro<br>
> replacements,<br>
> /// directives, etc. Source locations found in the clang AST will<br>
> always<br>
> /// point to one of these tokens.<br>
> + /// Tokens are in TU order (per<br>
> SourceManager::isBeforeInTranslationUnit()).<br>
> /// FIXME: figure out how to handle token splitting, e.g. '>>' can be<br>
> split<br>
> /// into two '>' tokens by the parser. However, TokenBuffer<br>
> currently<br>
> /// keeps it as a single '>>' token.<br>
> @@ -182,6 +183,10 @@ class TokenBuffer {<br>
> return ExpandedTokens;<br>
> }<br>
> <br>
> + /// Returns the subrange of expandedTokens() corresponding to the<br>
> + closed /// token range R.<br>
> + llvm::ArrayRef<syntax::Token> expandedTokens(SourceRange R) const;<br>
> +<br>
> /// Find the subrange of spelled tokens that produced the corresponding<br>
> \p<br>
> /// Expanded tokens.<br>
> ///<br>
> <br>
> diff --git a/clang/lib/Tooling/Syntax/Tokens.cpp<br>
> b/clang/lib/Tooling/Syntax/Tokens.cpp<br>
> index a2c3bc137d6b..5941507e086d 100644<br>
> --- a/clang/lib/Tooling/Syntax/Tokens.cpp<br>
> +++ b/clang/lib/Tooling/Syntax/Tokens.cpp<br>
> @@ -119,6 +119,22 @@ llvm::StringRef FileRange::text(const SourceManager<br>
> &SM) const {<br>
> return Text.substr(Begin, length());<br>
> }<br>
> <br>
> +llvm::ArrayRef<syntax::Token> TokenBuffer::expandedTokens(SourceRange<br>
> +R) const {<br>
> + if (R.isInvalid())<br>
> + return {};<br>
> + const Token *Begin =<br>
> + llvm::partition_point(expandedTokens(), [&](const syntax::Token &T)<br>
> {<br>
> + return SourceMgr->isBeforeInTranslationUnit(T.location(),<br>
> R.getBegin());<br>
> + });<br>
> + const Token *End =<br>
> + llvm::partition_point(expandedTokens(), [&](const syntax::Token &T)<br>
> {<br>
> + return !SourceMgr->isBeforeInTranslationUnit(R.getEnd(),<br>
> T.location());<br>
> + });<br>
> + if (Begin > End)<br>
> + return {};<br>
> + return {Begin, End};<br>
> +}<br>
> +<br>
> std::pair<const syntax::Token *, const TokenBuffer::Mapping *><br>
> TokenBuffer::spelledForExpandedToken(const syntax::Token *Expanded) const<br>
> {<br>
> assert(Expanded);<br>
> <br>
> diff --git a/clang/unittests/Tooling/Syntax/TokensTest.cpp<br>
> b/clang/unittests/Tooling/Syntax/TokensTest.cpp<br>
> index 6ffe2c43dd0f..2c462d49ee41 100644<br>
> --- a/clang/unittests/Tooling/Syntax/TokensTest.cpp<br>
> +++ b/clang/unittests/Tooling/Syntax/TokensTest.cpp<br>
> @@ -40,6 +40,7 @@<br>
> #include "llvm/Support/raw_ostream.h"<br>
> #include "llvm/Testing/Support/Annotations.h"<br>
> #include "llvm/Testing/Support/SupportHelpers.h"<br>
> +#include "gmock/gmock.h"<br>
> #include <cassert><br>
> #include <cstdlib><br>
> #include <gmock/gmock.h><br>
> @@ -663,6 +664,20 @@ TEST_F(TokenBufferTest, SpelledByExpanded) {<br>
> ValueIs(SameRange(findSpelled("not_mapped"))));<br>
> }<br>
> <br>
> +TEST_F(TokenBufferTest, ExpandedTokensForRange) {<br>
> + recordTokens(R"cpp(<br>
> + #define SIGN(X) X##_washere<br>
> + A SIGN(B) C SIGN(D) E SIGN(F) G<br>
> + )cpp");<br>
> +<br>
> + SourceRange R(findExpanded("C").front().location(),<br>
> + findExpanded("F_washere").front().location());<br>
> + // Sanity check: expanded and spelled tokens are stored separately.<br>
> + EXPECT_THAT(Buffer.expandedTokens(R),<br>
> + SameRange(findExpanded("C D_washere E F_washere")));<br>
> + EXPECT_THAT(Buffer.expandedTokens(SourceRange()),<br>
> +testing::IsEmpty()); }<br>
> +<br>
> TEST_F(TokenBufferTest, ExpansionStartingAt) {<br>
> // Object-like macro expansions.<br>
> recordTokens(R"cpp(<br>
> <br>
> <br>
> <br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank" rel="noreferrer">cfe-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>