Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Differential D61774 Diff 199896 clang/include/clang/Tooling/Refactoring/RangeSelector.h

Changeset View

Standalone View

View Options

clang/include/clang/Tooling/Refactoring/RangeSelector.h

  • This file was added.
//===--- RangeSelector.h - Source-selection library ---------*- 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
//
//===----------------------------------------------------------------------===//
///
/// \file
/// Defines a combinator library supporting the definition of _selectors_,
/// which select source ranges based on (bound) AST nodes.
///
//===----------------------------------------------------------------------===//
#ifndef LLVM_CLANG_TOOLING_REFACTOR_RANGE_SELECTOR_H_
#define LLVM_CLANG_TOOLING_REFACTOR_RANGE_SELECTOR_H_
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include <functional>
namespace clang {
namespace tooling {
using RangeSelector = std::function<Expected<CharSourceRange>(
const ast_matchers::MatchFinder::MatchResult &)>;
inline RangeSelector charRange(CharSourceRange R) {
return [R](const ast_matchers::MatchFinder::MatchResult &)
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

Maybe try putting it into the tooling namespace directly?
Or are there immediate and unfortunate conflicts with existing names?

ilya-biryukov: Maybe try putting it into the tooling namespace directly? Or are there immediate and…
ymandelAuthorUnsubmitted
Done
ReplyInline Actions

No conflicts. Was just being cautious.

ymandel: No conflicts. Was just being cautious.
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

I can see why a separate namespace would make sense, but since the tooling namespace is not densely populated I hope we can get away with putting things here directly and saving some typing on the use-sites.

Hope that does not backfire in the future, though.

ilya-biryukov: I can see why a separate namespace would make sense, but since the `tooling` namespace is not…
ymandelAuthorUnsubmitted
Done
ReplyInline Actions

SGTM.

ymandel: SGTM.
-> Expected<CharSourceRange> { return R; };
}
/// Selects from the start of \p Begin and to the end of \p End.
RangeSelector range(RangeSelector Begin, RangeSelector End);
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

NIT: maybe explain what Id is?

ilya-biryukov: NIT: maybe explain what Id is?
/// Convenience version of \c range where end-points are bound nodes.
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

NIT: I believe LLVM naming rules would mandate to name it ID

ilya-biryukov: NIT: I believe LLVM naming rules would mandate to name it `ID`
ymandelAuthorUnsubmitted
Done
ReplyInline Actions

here and throughout.

ymandel: here and throughout.
RangeSelector range(StringRef BeginID, StringRef EndID);
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

Maybe a shorter description could suffice?

Selects a node and a trailing semicolon. Useful for selecting expression statements.
ilya-biryukov: Maybe a shorter description could suffice? ``` Selects a node and a trailing semicolon. Useful…
/// Selects a node, including trailing semicolon (for non-expression
/// statements). \p ID is the node's binding in the match result.
RangeSelector node(StringRef ID);
/// Selects a node, including trailing semicolon (always). Useful for selecting
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

NIT: maybe name it statement?

ilya-biryukov: NIT: maybe name it `statement`?
/// expression statements. \p ID is the node's binding in the match result.
RangeSelector statement(StringRef ID);
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

Could this be an overload with the same name?

ilya-biryukov: Could this be an overload with the same name?
/// Given a \c MemberExpr, selects the member token. \p ID is the node's
/// binding in the match result.
RangeSelector member(StringRef ID);
/// Given a node with a "name", (like \c NamedDecl, \c DeclRefExpr or \c
/// CxxCtorInitializer) selects the name's token. \p ID is the node's binding in
/// the match result.
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

NIT: this is super-specialized, but has a very generic name, therefore might cause confusion. Maybe call it ctorInitializerName?

ilya-biryukov: NIT: this is super-specialized, but has a very generic name, therefore might cause confusion.
ymandelAuthorUnsubmitted
Done
ReplyInline Actions

It works with others as well, particularly NamedDecl. This is one place where typed node ids would be nice, b/c that would allow us to make this interface typed, like the matchers.

I kept the name but tried to clarify the comments. WDYT?

ymandel: It works with others as well, particularly NamedDecl. This is one place where typed node ids…
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

Ah, sorry, misread the original comment. The name actually makes sense in that case.
Am I correct to assume it will only select the final identifier of a qualified name, but not the qualifier?
E.g. for ::foo::bar::baz, it would only select baz, right?
What happens when we also have template args? E.g. for ::foo::bar::baz<int>, it would only select baz, right?

Maybe add those examples to the documentation? It's something that's very easy to get wrong.

ilya-biryukov: Ah, sorry, misread the original comment. The name actually makes sense in that case. Am I…
RangeSelector name(StringRef ID);
// Given a \c CallExpr (bound to \p ID), selects the arguments' source text (all
// source between the call's parentheses).
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

Same thing, maybe rename to callExprArgs?
And other nodes too

ilya-biryukov: Same thing, maybe rename to `callExprArgs`? And other nodes too
ymandelAuthorUnsubmitted
Done
ReplyInline Actions

i'd like to keep these as lightweight as possible. Compromised on callArgs?

ymandel: i'd like to keep these as lightweight as possible. Compromised on callArgs?
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

callArgs LG, thanks!

ilya-biryukov: `callArgs` LG, thanks!
RangeSelector callArgs(StringRef ID);
// Given a \c CompoundStmt (bound to \p ID), selects the source of the
// statements (all source between the braces).
RangeSelector statements(StringRef ID);
// Given a \c InitListExpr (bound to \p ID), selects the range of the elements
// (all source between the braces).
RangeSelector initListElements(StringRef ID);
/// Selects the range from which `S` was expanded (possibly along with other
/// source), if `S` is an expansion, and `S` itself, otherwise. Corresponds to
/// `SourceManager::getExpansionRange`.
RangeSelector expansion(RangeSelector S);
} // namespace tooling
} // namespace clang
ilya-biryukovUnsubmitted
Done
ReplyInline Actions

NIT: maybe call it expansion?

contraction is a new term, might be confusing to use.

ilya-biryukov: NIT: maybe call it `expansion`? `contraction` is a new term, might be confusing to use.
ymandelAuthorUnsubmitted
Done
ReplyInline Actions

Sigh. You're right. Its just that the existing terminology is confusing. that said, anyone using this combinator is already doing advanced work, so consistency is probably more important than clarity.

ymandel: Sigh. You're right. Its just that the existing terminology is confusing. that said, anyone…
#endif // LLVM_CLANG_TOOLING_REFACTOR_RANGE_SELECTOR_H_