[PATCH] D90240: [SyntaxTree] Add reverse links to syntax Nodes.

2020-10-27 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:189 /// EXPECTS: Child->Role != Detached void prependChildLowLevel(Node *Child); friend class TreeBuilder; eduucaldas wrote: > Should we provide an

[PATCH] D88859: APINotes: add APINotesYAMLCompiler

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. > I am having a hard time to accept "this is how it is implemented in our fork" > as a technical argument. Besides, I am not sure how could the Clang community > benefit about being backward compatible with a specialized fork and thus > making superfluous

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:182 +/// The element, or nullptr if past-the-end. +NodeT *asPointer() const { return N; } + }; sammccall wrote: > gribozavr2 wrote: > > "nodePointer()" ? > I find

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Could you add tests that verify the pairing of elements and delimiters? Comment at: clang/include/clang/Tooling/Syntax/Tree.h:214 + /// elements and delimiters are represented as null pointers. Below we give + /// examples of what this iteration

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:182 +/// The element, or nullptr if past-the-end. +NodeT *asPointer() const { return N; } + };

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/AST/CommentLexer.h:74 /// contains the length of the string that starts at TextPtr. unsigned IntVal;

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/AST/CommentLexer.h:244 + /// command, including command marker. + SmallString<16> VerbatimBlockEndCommandName; + I'm not a fan of this change to `Lexer` because it breaks the grouping of

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

2020-10-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:172-174 /// Find the first node with a corresponding role. Node *findChild(NodeRole R); + const Node *findChild(NodeRole R) const; eduucaldas wrote: > I think that

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

2020-10-20 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:37 -// Recursively collects FoldingRange from a symbol and its children. -void collectFoldingRanges(DocumentSymbol Symbol, - std::vector ) { +FoldingRange

[PATCH] D89608: Make sure Objective-C category support in IncludeSorter handles top-level imports

2020-10-17 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Any chance of adding a test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89608/new/ https://reviews.llvm.org/D89608

[PATCH] D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied.

2020-10-17 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp:409 + +namespace std { + flx wrote: > gribozavr2 wrote: > > Could you add a nested inline namespace to better imitate what

[PATCH] D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied.

2020-10-15 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp:409 + +namespace std { + Could you add a

[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`

2020-10-14 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:122 #endif + Node * = BeforeBegin ? BeforeBegin->NextSibling : FirstChild; + Could you move this

[PATCH] D89303: [SyntaxTree] Improve safety of `replaceChildRangeLowLevel`

2020-10-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:103 #ifndef NDEBUG - for (auto *N = New; N; N = N->getNextSibling()) { + for (auto *N = New; N; N = N->NextSibling) { assert(N->Parent == nullptr); eduucaldas wrote: >

[PATCH] D88106: [SyntaxTree] Provide iterator-like functions for Lists

2020-10-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:211 + return element == Other.element && delimiter == Other.delimiter; +} }; Please also define `operator!=` even if it is not used yet.

[PATCH] D88886: [Clang][unittests][NFC] Break up test in Callbacks.cpp

2020-10-06 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/unittests/Tooling/RecursiveASTVisitorTests/CallbacksCommon.h:1 + +using namespace clang; Please add a license comment and all necessary includes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D88886: [Clang][unittests][NFC] Break up test in Callbacks.cpp

2020-10-06 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a reviewer: gribozavr2. gribozavr2 added a comment. Thank you! LGTM assuming you only moved the tests into separate cpp files (I didn't verify that the text is exactly the same). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D88831: [clang-tidy] Remove obsolete checker google-runtime-references

2020-10-05 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Google C++ style guide does not have this rule anymore. Thanks for the cleanup! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88831/new/

[PATCH] D88403: Migrate Declarators to use the List API

2020-09-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:593 +/// Shrink \p Range to a subrange that only contains tokens of a list. +ArrayRef shrinkToFitList(ArrayRef Range) { Comment at:

[PATCH] D87839: [SyntaxTree] Test the List API

2020-09-22 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:224 + /// "a, b c" <=> [("a", ","), ("b", nul), ("c", nul)] + /// "a, b,"<=> [("a", ","), ("b", ","), (nul, nul)] /// I'd slightly prefer "null" b/c "nul" refers

[PATCH] D87749: [SyntaxTree][Synthesis] Implement `deepCopy`

2020-09-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:205 + if (L->canModify()) +syntax::FactoryImpl::setCanModify(Leaf); + eduucaldas wrote: > gribozavr2 wrote: > > Since we are creating new leaves, why prohibit their

[PATCH] D87749: [SyntaxTree][Synthesis] Implement `deepCopy`

2020-09-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:216 + Child = Child->getNextSibling()) +if (!Child->canModify()) + return false; Shouldn't we call canModifyAllDescendants recursively? Comment

[PATCH] D87720: Sema: add support for `__attribute__((__swift_private__))`

2020-09-18 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3592 + let Content = [{ +Objective-C declarations marked with the ``swift_private`` attribute are hidden +from the framework client but are still made available for use within the

[PATCH] D87779: [SyntaxTree] Test `findFirstLeaf` and `findLastLeaf`

2020-09-18 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:9 +// +// This file tests the Syntax Tree base API. +// Please elaborate what "base API" covers -- specifically, what classes and such. However, I'm not sure it is

[PATCH] D87534: Sema: introduce `__attribute__((__swift_name__))`

2020-09-18 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. > This introduces the new swift_name attribute that allows annotating > interfaces with an alternate spelling for Swift. This is used as part > of the importing mechanism to allow interfaces to be imported with a new

[PATCH] D87895: [SyntaxTree] Test for '\' inside token.

2020-09-18 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:173 + R"cpp( +in/ +t a; That looks like the wrong slash to me (forward instead

[PATCH] D87749: [SyntaxTree][Synthesis] Implement `deepCopy`

2020-09-17 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:45 +/// Deep copies `N`. +/// "Creates a completely independent copy of `N` (a deep

[PATCH] D87683: [clang-tidy] Crash fix for bugprone-misplaced-pointer-arithmetic-in-alloc

2020-09-15 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. In D87683#2274883 , @martong wrote: > In D87683#2274663 , > @baloghadamsoftware wrote: > >> In D87683#2274569 , @njames93 wrote: >> >>> Please

[PATCH] D87600: [SyntaxTree][List] `assertInvariants` for `List`s

2020-09-14 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:236 + + auto *L = dyn_cast(T); + if (!L) Comment at:

[PATCH] D87482: Fix clang Wrange-loop-analysis in BuildTree.cpp

2020-09-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Sorry, I didn't realize you don't have commit access. Pushed this patch to git, thanks for the contribution! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87482/new/ https://reviews.llvm.org/D87482

[PATCH] D87482: Fix clang Wrange-loop-analysis in BuildTree.cpp

2020-09-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe10df779f097: Fix clang Wrange-loop-analysis in BuildTree.cpp (authored by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D87395: Sema: add support for `__attribute__((__swift_objc_members__))`

2020-09-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87395/new/ https://reviews.llvm.org/D87395

[PATCH] D87533: [SyntaxTree][Synthesis] Add support for Tree.

2020-09-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:39 +syntax::Tree * +createTree(Arena , std::vector>, + syntax::NodeKind);

[PATCH] D87495: [SyntaxTree][Synthesis] Add support for simple Leafs and test based on tree dump

2020-09-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:38 +StringRef Spelling) { + auto *Leaf = createLeafLowLevel(A,

[PATCH] D87495: [SyntaxTree][Synthesis] Add support for simple Leafs and test based on tree dump

2020-09-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/Synthesis.cpp:51 + return createLeaf(A, tok::getKeywordSpelling(K), K); +} + eduucaldas wrote: > gribozavr2 wrote: > > Could we make a combined function that does not require the user to

[PATCH] D87495: [SyntaxTree][Synthesis] Add support for simple Leafs and test based on tree dump

2020-09-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:30-31 +syntax::Leaf *createKeyword(Arena , tok::TokenKind K); +syntax::Leaf *createLeaf(syntax::Arena , const char *spelling, + tok::TokenKind K); +

[PATCH] D87395: Sema: add support for `__attribute__((__swift_objc_members__))`

2020-09-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/test/SemaObjC/attr-swift_objc_members.m:4 +#if !__has_attribute(swift_objc_members) +#error cannot verify precense of swift_objc_members attribute +#endif compnerd wrote: > aaron.ballman wrote: > > gribozavr2

[PATCH] D87395: Sema: add support for `__attribute__((__swift_objc_members__))`

2020-09-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3387 +The ``swift_objc_members`` attribute maps to the ``@objcMembers`` Swift +attribute, which indicates that Swift members of this class, its subclasses, and +all extensions thereof, will

[PATCH] D87395: Sema: add support for `__attribute__((__swift_objc_members__))`

2020-09-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3387 +The ``swift_objc_members`` attribute maps to the ``@objcMembers`` Swift +attribute, which indicates that Swift members of this class, its subclasses, and +all extensions thereof, will

[PATCH] D87395: Sema: add support for `__attribute__((__swift_objc_members__))`

2020-09-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/test/SemaObjC/attr-swift_objc_members.m:4 +#if !__has_attribute(swift_objc_members) +#error cannot verify precense of swift_objc_members attribute +#endif aaron.ballman wrote: > gribozavr2 wrote: > > > The

[PATCH] D87331: Sema: add support for `__attribute__((__swift_error__))`

2020-09-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3377-3382 +def SwiftDocs : DocumentationCategory<"Controlling Swift Import"> { + let Content = [{ +Clang supports additional attributes for controlling how

[PATCH] D87395: Sema: add support for `__attribute__((__swift_objc_members__))`

2020-09-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3386-3388 +The ``swift_objc_members`` attribute maps to the ``@objcMembers`` Swift +attribute, which indicates that Swift members of this class, its subclasses, and +all extensions thereof, will

[PATCH] D87374: [SyntaxTree] Specialize `TreeTestBase` for `BuildTreeTest` and `MutationsTest`

2020-09-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/MutationsTest.cpp:48 + +TEST_P(MutationTest, Mutations) { + if (!GetParam().isCXX11OrLater()) {

[PATCH] D87249: [SyntaxTree] Fix crash on functions with default arguments.

2020-09-08 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:4177 + auto x1 = [[X(1)]]; + auto x2 = [[X(1, '2')]]; +} Please also add tests

[PATCH] D87249: [SyntaxTree] Fix crash on functions with default arguments.

2020-09-07 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Please also add tests for calls to constructors without arguments and calls to implicit constructors 1 argument, as requested in https://reviews.llvm.org/D86700. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87249/new/

[PATCH] D86700: [SyntaxTree] Ignore leaf implicit `CXXConstructExpr`

2020-09-07 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:1137 +// Ignore the implicit default constructs. +if ((S->getNumArgs() == 0 || isa(S->getArg(0))) && +S->getParenOrBraceRange().isInvalid()) eduucaldas wrote: >

[PATCH] D86699: [SyntaxTree] Ignore implicit non-leaf `CXXConstructExpr`

2020-09-07 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:48 +// Ignores the implicit `CXXConstructExpr` for copy/move constructors generated +// by the compiler, as well as in implicit conversions like the one

[PATCH] D86700: [SyntaxTree] Ignore leaf implicit `CXXConstructExpr`

2020-09-07 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:1136 + bool WalkUpFromCXXConstructExpr(CXXConstructExpr *S) { +// Ignore the implicit default constructs. +if ((S->getNumArgs() == 0 ||

[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.

2020-09-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:388 +llvm::Expected> +rewriteDescendants(const Decl , RewriteRule Rule, + const ast_matchers::MatchFinder::MatchResult ); ymandel wrote: >

[PATCH] D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.

2020-09-02 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:388 +llvm::Expected> +rewriteDescendants(const Decl , RewriteRule Rule, +

[PATCH] D86699: [SyntaxTree] Ignore implicit non-leaf `CXXConstructExpr`

2020-08-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:48-58 +static Expr *IgnoreImplicitCXXConstructExpr(Expr *E) { + if (auto *C = dyn_cast(E)) { +auto

[PATCH] D86700: [SyntaxTree] Ignore leaf implicit `CXXConstructExpr`

2020-08-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:552 [[::n::S s1]]; [[n::S s2]]; } Do we have tests for calling constructors

[PATCH] D86778: Extract infrastructure to ignore intermediate expressions into `clang/AST/IgnoreExpr.h`

2020-08-31 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/AST/IgnoreExpr.h:15 +namespace clang { +namespace { +/// Given an expression E and functions Fn_1,...,Fn_n : Expr * -> Expr *,

[PATCH] D86778: Extract infrastructure to ignore intermediate expressions into `clang/AST/IgnoreExpr.h`

2020-08-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. > This allows users to use IgnoreExprNodes outside of clang/AST/Expr.h Did you mean "outside of Expr.cpp"? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86778/new/ https://reviews.llvm.org/D86778

[PATCH] D86719: [SyntaxTree][NFC] Refactor function templates into functions taking base class

2020-08-28 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:202 +/// location of the `^`: +/// `int ^a;` +/// `int ^a::S::f(){}` Comment at:

[PATCH] D85214: [OpenMP] Ensure testing for versions 4.5 and default - Part 3

2020-08-27 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. In D85214#2243167 , @saiislam wrote: > In D85214#2243160 , @gribozavr2 > wrote: > >> @saiislam This change seems to have broken a buildbot, please take a look at >> your earliest

[PATCH] D85214: [OpenMP] Ensure testing for versions 4.5 and default - Part 3

2020-08-27 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. @saiislam This change seems to have broken a buildbot, please take a look at your earliest convenience. Before: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/35354 After: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/35355

[PATCH] D86719: [SyntaxTree][NFC] Refactor function templates into functions taking base class

2020-08-27 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. > Perhaps we should inline getQualifiedNameStart and getInitializerRange and > make getDeclaratorRange a member function taking a Decl. What do you think? These helpers seem to have

[PATCH] D86679: [SyntaxTree][NFC] Append "get" to syntax Nodes accessor names

2020-08-27 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. > It's worth noting that accessors in the base APIs don't follow this rule. > Should we refactor them as well? I'd say yes. > In this patch? Up to you. Repository: rG LLVM

[PATCH] D86636: [SyntaxTree] Refactor `NodeRole`s

2020-08-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:156 // Roles specific to particular node kinds. - OperatorExpression_operatorToken, -

[PATCH] D86600: [SyntaxTree] Migrate `ParamatersAndQualifiers` to use the new List API

2020-08-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:4041 +template +[[void test(T , Args... );]] +)cpp", Could you also add one with a

[PATCH] D86600: [SyntaxTree] Migrate `ParamatersAndQualifiers` to use the new List API

2020-08-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Could you add some tests with variadic parameter lists & variadic function templates? If we don't handle them correctly, please leave a FIXME in the test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86600/new/

[PATCH] D86600: [SyntaxTree] Migrate `ParamatersAndQualifiers` to use the new List API

2020-08-26 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. I think we also need to update `List::getTerminationKind()` and other similar List methods to handle this list kind. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86600/new/ https://reviews.llvm.org/D86600

[PATCH] D86544: [SyntaxTree] Add support for call expressions

2020-08-25 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:333 +/// call-arguments: +/// delimited_list(expression, ',', ) +/// Note: This construct is a

[PATCH] D86441: [SyntaxTree] Split ExplicitTemplateInstantiation test

2020-08-24 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:3054 -TEST_P(SyntaxTreeTest, ExplicitTemplateInstantations) { +TEST_P(SyntaxTreeTest,

[PATCH] D85330: [SyntaxTree] Extend the syntax tree dump

2020-08-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:107-110 int main() { if (1) {} if (1) {} else if (0) {} } eduucaldas wrote: > I just noticed that we didn't yet use annotations on statement tests. > I think

[PATCH] D85330: [SyntaxTree] Extend the syntax tree dump

2020-08-21 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Output format LGTM! Comment at: clang/lib/Tooling/Syntax/Tree.cpp:160 + assert(N); + if (auto *L = llvm::dyn_cast(N)) { +OS << "'"; Comment at: clang/lib/Tooling/Syntax/Tree.cpp:169 - auto *T = cast(N);

[PATCH] D86227: [SyntaxTree] Add support for `MemberExpression`

2020-08-20 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:911 + S->getEndLoc()), + idExpression, nullptr); +

[PATCH] D86227: [SyntaxTree] Add support for `MemberExpression`

2020-08-20 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:180 + MemberExpression_member, + MemberExpression_accessToken, }; Could you order new items in source order? object, access token, member. Comment at:

[PATCH] D86139: [SyntaxTree] Split tests related to Namespace

2020-08-18 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:2151 + +TEST_P(SyntaxTreeTest, Namepace_UsingDirective) { + if (!GetParam().isCXX()) {

[PATCH] D85962: [SyntaxTree] Create annotations infrastructure and apply it in expression tests.

2020-08-14 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Very nice improvement to tests! Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:784 + +TEST_P(SyntaxTreeTest, QualifiedId_ComplexDeclaration) { + if

[PATCH] D85962: [SyntaxTree] Create annotations infrastructure and apply it in expression tests.

2020-08-14 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/unittests/Tooling/Syntax/TreeTestBase.cpp:197 + + bool failed = false; + auto AnnotatedRanges = AnnotatedCode.ranges(); Comment at: clang/unittests/Tooling/Syntax/TreeTestBase.cpp:199 +

[PATCH] D85913: [SyntaxTree] Split `TreeTestBase` into header and source

2020-08-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/TreeTestBase.cpp:38 +namespace { +static ArrayRef tokens(syntax::Node *N) { + assert(N->isOriginal() && "tokens of

[PATCH] D84005: Introduce ns_error_domain attribute.

2020-08-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGa5b8757506b0: Introduce ns_error_domain attribute. (authored by MForster, committed by gribozavr). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84005/new/

[PATCH] D85897: [SyntaxTree] Split `TreeTest.cpp`

2020-08-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/BuildTreeTest.cpp:16 +namespace clang { +namespace syntax_test { namespace { Ditto, please use

[PATCH] D85898: [SyntaxTree] Clean `#includes` in `TreeTestBase.h`

2020-08-13 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/TreeTestBase.h:14 #include "clang/AST/ASTConsumer.h" -#include "clang/AST/Decl.h" -#include "clang/AST/Stmt.h"

[PATCH] D85819: [SyntaxTree] Split tests

2020-08-12 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Please also consider splitting the file into multiple. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:1238 +namespace n { +template +struct ST {

[PATCH] D85750: [SyntaxTree] Unbox operators into tokens for nodes generated from `CXXOperatorCallExpr`

2020-08-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:1024 + // representation of built-in and user-defined operators. + if (child->getBeginLoc() == S->getOperatorLoc()) +continue; eduucaldas wrote: > Here we want

[PATCH] D85750: [SyntaxTree] Unbox operators into tokens for nodes generated from `CXXOperatorCallExpr`

2020-08-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:1016 // operand because it does not correspond to anything written in source // code + if

[PATCH] D85734: [libTooling] Move RewriteRule include edits to ASTEdit granularity.

2020-08-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:39 + // Inserts an include in the file. The `Replacement` field is the name of the + // file for which to add an include. + AddInclude,

[PATCH] D85733: [libTooling] Cleanup and reorder `RewriteRule.h`.

2020-08-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:147 +// Every rewrite rules is triggered by a match against some AST node. +// Transformer

[PATCH] D85713: [SyntaxTree]: Use Annotations in tests to reduce noise

2020-08-11 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:186-187 +auto AnnotatedRanges = AnnotatedCode.ranges(); +assert(AnnotatedRanges.size() == TreeDumps.size()); +for (auto i = 0u; i < AnnotatedRanges.size(); i++) { + auto

[PATCH] D85440: [SyntaxTree] Implement `NNS` using the `List` base API

2020-08-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:362 + switch (this->kind()) { + case NodeKind::NestedNameSpecifier: { +return clang::tok::coloncolon; Please drop the braces within 'case'

[PATCH] D85439: [SyntaxTree] Expand support for `NestedNameSpecifier`

2020-08-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/include/clang/AST/NestedNameSpecifier.h:20 #include "clang/Basic/SourceLocation.h" +#include "llvm/ADT//DenseMapInfo.h" #include "llvm/ADT/FoldingSet.h"

[PATCH] D84005: Introduce ns_error_domain attribute.

2020-08-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. Comment at: clang/test/AST/ast-print-attr.c:20 + +2@interface NSString +@end Stray "2". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D84781: [SyntaxTree] Use PointerUnion instead of inheritance for alternative clauses in NNS

2020-08-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Seeing the API, I like the inheritance-based approach better. It seems more uniform. Comment at: clang/lib/Tooling/Syntax/Nodes.cpp:219 + syntax::EmptyNode *, syntax::Leaf *, syntax::DecltypeSpecifier *, + syntax::SimpleTemplateSpecifier

[PATCH] D85295: [SyntaxTree] Implement the List construct.

2020-08-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:194 +/// A Tree that represents a syntactic list of elements. +/// """ A list of

[PATCH] D85440: [SyntaxTree] Implement `NNS` using the `List` base API

2020-08-10 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. LGTM after my comments are addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85440/new/ https://reviews.llvm.org/D85440 ___ cfe-commits mailing list

[PATCH] D85439: [SyntaxTree] Expand support for `NestedNameSpecifier`

2020-08-07 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:227-254 +namespace llvm { +template <> struct DenseMapInfo { + using FirstInfo = DenseMapInfo; + using

[PATCH] D85440: [SyntaxTree] Implement `NNS` using the `List` base API

2020-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Could you add unit tests for methods in List and NNS? OK if they are in a separate patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85440/new/ https://reviews.llvm.org/D85440

[PATCH] D85440: [SyntaxTree] Implement `NNS` using the `List` base API

2020-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. Please also add appropriate handling to `syntax::List::getDelimiterTokenKind`, `getTerminationKind`, and `canBeEmpty`. Comment at: clang/lib/Tooling/Syntax/Nodes.cpp:240 return Children; -} +}; Repository: rG LLVM Github

[PATCH] D85295: [SyntaxTree] Implement the List construct.

2020-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:247 + + TerminationKind getTerminationKind(); + I just realized that the rest of the syntax tree API does not use the `get~` prefix. However, most of Clang does, as far as

[PATCH] D85295: [SyntaxTree] Implement the List construct.

2020-08-06 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tree.h:197 + MaybeTerminated, + Separated, +}; eduucaldas wrote: > gribozavr2 wrote: > > Add a "WithoutDelimiters" case as well? > I think we might want to treat

[PATCH] D85330: [SyntaxTree] Extend the syntax tree dump

2020-08-05 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added inline comments. Comment at: clang/lib/Tooling/Syntax/Tree.cpp:158 if (!Marks.empty()) OS << Marks << ": "; Maybe the marks should be moved after the primary identifier of the node, WDYT? That would be more consistent with AST dump:

[PATCH] D85316: [SyntaxTree] Proposition of new tree dump

2020-08-05 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85316/new/ https://reviews.llvm.org/D85316 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D85316: [SyntaxTree] Proposition of new tree dump

2020-08-05 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. I'd suggest to drop the `<>` quotes (because the AST dump does not add quotes unless it is printing a multi-word thing, and because `<>` don't exactly scream "role" helping to read the output). I'd also suggest to replace multiple spaces before `<>` with a single

[PATCH] D85305: [SyntaxTree] Remove dead code on dump functions

2020-08-05 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. > Node::dumpTokens was never used. I think it is because it was meant to be a debugger-only helper. I think we should keep it. However, simplifying `static void dumpTokens` to only take one token instead of `ArrayRef` makes sense! Repository: rG LLVM Github

[PATCH] D85295: [SyntaxTree] Implement the List construct.

2020-08-05 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 added a comment. I feel uneasy about adding this code without tests. Could we maybe port the function parameter list to use this infrastructure, and then add tests that exercise `getElementsAsNodesAndDelimiters`? Comment at:

[PATCH] D85185: [SyntaxTree] Add test coverage for `->*` operator

2020-08-04 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2330 +int X::*pmi; +void test(X x, X y, X* xp) { x = y; Could you add `pmi` as a

[PATCH] D85146: [SyntaxTree] Fix crash on pointer to member function

2020-08-03 Thread Dmitri Gribenko via Phabricator via cfe-commits
gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:4076-4077 + R"cpp( +struct X{ + struct Y{}; +}; Please add spaces before `{`.

  1   2   3   4   5   6   7   8   9   >