[PATCH] D64863: [clangd] Ignore diags from builtin files

2019-07-22 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Diagnostics.cpp:563 FillDiagBase(*LastDiag); -adjustDiagFromHeader(*LastDiag, Info, *LangOpts); +if (!InsideMainFile) + LastDiagWasAdjusted = adjustDiagFromHeader(*LastDiag, Info,

[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

2019-07-22 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 211035. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Rewrite code as suggested in the review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64762/new/

[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

2019-07-22 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2332 S->isSemanticForm() ? S->getSyntacticForm() : S, Queue)); TRY_TO(TraverseSynOrSemInitListExpr( S->isSemanticForm() ? S : S->getSemanticForm(), Queue));

[PATCH] D64863: [clangd] Ignore diags from builtin files

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Diagnostics.cpp:128 +return false; + Position StartPos = sourceLocToPosition(SM, IncludeInMainFile); NIT: inline `StartPos`, it has online a single usage now.

[PATCH] D64863: [clangd] Ignore diags from builtin files

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Diagnostics.cpp:601 - if (mentionsMainFile(*LastDiag) || - (LastDiag->Severity >= DiagnosticsEngine::Level::Error && - IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) {

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. The fix for a race condition on remove has landed in rL366577 , this revision would need a small update after it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64475/new/

[PATCH] D64985: [clangd] Provide a way to publish highlightings in non-racy manner

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210823. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Group PublishFn with onMainAST Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64985/new/

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307 +llvm::StringRef NewCode; +std::vector DiffedLines; + } TestCases[]{ @hokein rightfully pointed out that mentioning all changed lines

[PATCH] D64638: [CrossTU] Fix plist macro expansion if macro in other file.

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. `StaticAnalyzer/Core` does not depend on `clangFrontend` now, you can see this by looking at `lib/StaticAnalyzer/Core/CMakeLists.txt`: add_clang_library(clangStaticAnalyzerCore ... LINK_LIBS clangAST clangASTMatchers clangAnalysis

[PATCH] D64985: [clangd] Provide a way to publish highlightings in non-racy manner

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Thanks for all the suggestions. This is ready for the next round now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64985/new/ https://reviews.llvm.org/D64985 ___

[PATCH] D64985: [clangd] Provide a way to publish highlightings in non-racy manner

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210808. ilya-biryukov added a comment. - Remove a leftover comment from the previous version Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64985/new/ https://reviews.llvm.org/D64985 Files:

[PATCH] D64985: [clangd] Provide a way to publish highlightings in non-racy manner

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210807. ilya-biryukov added a comment. - Update usage of DiagsMu in a comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64985/new/ https://reviews.llvm.org/D64985 Files:

[PATCH] D64985: [clangd] Provide a way to publish highlightings in non-racy manner

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210805. ilya-biryukov added a comment. - Use the same mechanism for diagnostics - Change typedef to function)> - Update a comment - s/PublishResults/PublishFn - Reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D64985: [clangd] Provide a way to publish highlightings in non-racy manner

2019-07-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, javed.absar. Herald added a project: clang. By exposing a callback that can guard code publishing results of 'onMainAST' callback in the same manner we

[PATCH] D64924: [LibTooling] Add function to translate and validate source range for editing

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/Tooling/Refactoring/SourceCode.h:80 +llvm::Optional +getRangeForEdit(const CharSourceRange , const ASTContext ); } //

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov 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/D64518/new/ https://reviews.llvm.org/D64518

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. @rsmith, I'll look into emitting the typos when we pop expression evaluation context, but do we expect this to cover **all** cases where `TypoExpr`s are produced? (conservatively assuming that the answer is "no") should we land this patch and also emit at the end

[PATCH] D64915: [clangd] cleanup: unify the implemenation of checking a location is inside main file.

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:448 + }; + for (const auto *HeaderDecl : {"Header1", "Header2", "Header3"}) +

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458 +// edit there are stale previous highlightings. +std::lock_guard Lock(HighlightingsMutex); +

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:307 +TEST(SemanticHighlighting, HighlightingDiffer) { + struct { Can we test this in a more direct manner by specifying: 1. annotated input for

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:274 +std::vector +diffHighlightings(ArrayRef NewHighlightings, + ArrayRef OldHighlightings) { NIT: maybe rename to `New` and `Old`, the suffix of

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. A few suggestions from me, I haven't looked into the diffing algorithm and the tests yet. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:458 +// edit there are stale previous highlightings. +std::lock_guard

[PATCH] D64918: [ASTUnit] Fix a regression in cached completions

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang/test/Index/complete-qualified-with-preamble.cpp:7 +// START-OF-LINE: Class +// -- With preambles. +// RUN: CINDEXTEST_EDITING=1 c-index-test -code-completion-at=%s:3:1 %s \ sammccall wrote: > Nit: This (and

[PATCH] D64627: [clangd] Suppress unwritten scopes when expanding auto.

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov 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/D64627/new/ https://reviews.llvm.org/D64627

[PATCH] D64918: [ASTUnit] Fix a regression in cached completions

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210555. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - s/preamble/cached completions (in tests) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64918/new/

[PATCH] D64915: [clangd] cleanup: unify the implemenation of checking a location is inside main file.

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Neat! Many thanks, that's a very useful cleanup. A few suggestions from my side. Comment at: clang-tools-extra/clangd/SourceCode.cpp:332 +bool isInsideMainFile(SourceLocation Loc, const SourceManager ) { + return Loc.isValid() &&

[PATCH] D64918: [ASTUnit] Fix a regression in cached completions

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added a subscriber: arphaman. Herald added a project: clang. After r345152 cached completions started adding namespaces after nested name specifiers, e.g. in `some_name::^` The CCC_Symbol indicates the

[PATCH] D64907: [AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Wanted to write unit tests, but couldn't find any that check traversal order. I'm aware of `Tooling/RecursiveASTVisitorTests`, but they mostly check that implicit nodes are visited. Do we have other tests for traversals? Repository: rG LLVM Github Monorepo

[PATCH] D64907: [AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210507. ilya-biryukov added a comment. - Visit attributes before visiting the Decl Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64907/new/ https://reviews.llvm.org/D64907 Files:

[PATCH] D64907: [AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added reviewers: rsmith, gribozavr. Herald added a project: clang. Instead of traversing inside the TraverseDecl() function. Previously the attributes were traversed after Travese(Some)Decl returns. Logically attributes are properties of

[PATCH] D64863: [clangd] Ignore diags from builtin files

2019-07-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Diagnostics.cpp:473 + SourceManager = Info.getSourceManager(); + if (!InsideMainFile && SM.isWrittenInBuiltinFile(Info.getLocation())) { +IgnoreDiagnostics::log(DiagLevel, Info);

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. In D64518#1589857 , @ymandel wrote: > In D64518#1589768 , @ilya-biryukov > wrote: > > > In D64518#1588092 , @ymandel wrote: > > > > > This

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. In D64518#1588092 , @ymandel wrote: > This seems like a good candidate for configuration -- the user could then > choose which mode to run in. But, I'm also open to just reporting these > conditions as errors. It's

[PATCH] D64863: [clangd] Ignore diags from builtin files

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Thanks for switching to `SM` everywhere, makes the code much more readable! A rough proposal for testing this: // test.cpp // RUN: clang++ -DFOO=b ./test.cpp int a = FOO; This produces a note inside a `` buffer. We could probably have something similar

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210338. ilya-biryukov added a comment. - Remove -disable-free from the test, it is no longer required to workaround the crash Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64799/new/

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. I tried to find a good place to emit unresolved typos earlier (to make sure CodeGen does not ever get `TypoExpr`), but couldn't find one. Please let me know if there's some obvious place I'm missing. Also open to suggestions for putting assertions on whether

[PATCH] D64861: [clang-tidy] Adjust location of namespace comment diagnostic

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added reviewers: alexfh, hokein. Herald added subscribers: kadircet, xazax.hun. Herald added a project: clang. If there is no comment, place it at the closing brace of a namespace definition. Previously it was placed at the next character after

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Thanks for pointing out I missed stuff. The strategy for conflicting highlightings LG, just a few NITs left from my side. In D64741#1589144 , @jvikstrom wrote: > Already added the case you sent a comment on in the test

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

2019-07-17 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. > I had completely missed that there could be conflicting tokens when only > highlighting macro arguments as well. Added code to just remove conflicting > tokens. Picking one of the highlightings looks fine, but we probably want to make sure it's deterministic.

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

2019-07-16 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210105. ilya-biryukov added a comment. Fix a typo (xD) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64799/new/ https://reviews.llvm.org/D64799 Files: clang/lib/Sema/Sema.cpp

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

2019-07-16 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added reviewers: rnk, sammccall. Herald added a subscriber: kadircet. Herald added a project: clang. Instead of asserting all typos are corrected in the sema destructor. The sema destructor is not run in the common case of running the compiler

[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

2019-07-16 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2332 S->isSemanticForm() ? S->getSyntacticForm() : S, Queue)); TRY_TO(TraverseSynOrSemInitListExpr( S->isSemanticForm()

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

2019-07-16 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. How should this behave when if the same token is used multiple times inside a macro and the uses are different? Could we add this to tests? E.g. #define W(a) class a { void test() { int a = 10; } } W(foo); // <-- `foo` is a variable of a class? Repository:

[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

2019-07-16 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 210052. ilya-biryukov added a comment. - Add a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64762/new/ https://reviews.llvm.org/D64762 Files: clang/include/clang/AST/RecursiveASTVisitor.h

[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

2019-07-16 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:2332 S->isSemanticForm() ? S->getSyntacticForm() : S, Queue)); TRY_TO(TraverseSynOrSemInitListExpr( S->isSemanticForm() ? S : S->getSemanticForm(), Queue));

[PATCH] D64762: [AST] Treat semantic form of InitListExpr as implicit code in traversals

2019-07-15 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr. Herald added a subscriber: kadircet. Herald added a project: clang. In particular, do not traverse the semantic form shouldVisitImplicitCode() returns false. This simplifies the common case of traversals, avoiding

[PATCH] D64518: [LibTooling] Relax Transformer to allow rewriting macro expansions

2019-07-15 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. This clearly increases the utility of the library, but also seems to add corner cases that the library won't handle (see the comment about unittests for an example). WDYT about those? Are they important, should we support producing warnings in those cases to let

[PATCH] D64747: [clangd] Skip implicit nodes when traversing for highlightings

2019-07-15 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov abandoned this revision. ilya-biryukov added a comment. Ah, sorry, that's actually the default... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64747/new/ https://reviews.llvm.org/D64747

[PATCH] D64747: [clangd] Skip implicit nodes when traversing for highlightings

2019-07-15 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added reviewers: jvikstrom, hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Implicit code adds corner cases to handle and does not provide any value as those nodes cannot be mapped back to

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

2019-07-15 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM (I think @hokein 's comments were addressed too) > So it seems to be expected. (and looking at the documentation for > InitListExpr it seems to be difficult to change

[PATCH] D64634: [clangd] Fix duplicate highlighting tokens appearing in initializer lists

2019-07-15 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Have we tried figuring out why `RecursiveASTVisitor` visits the argument lists twice? Is that an expected behavior? Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:37 +llvm::sort(Tokens, + [](const

[PATCH] D64576: [Syntax] Do not add a node for 'eof' into the tree

2019-07-11 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: kristof.beyls, javed.absar. Herald added a project: clang. While useful as a sentinel value when iterating over tokens, having 'eof' in the tree, seems to do more harm than good.

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

2019-07-11 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added a subscriber: mgorny. Herald added a project: clang. ilya-biryukov added a parent revision: D63835: [Syntax] Add nodes for most common statements. This patch adds facilities to mutate the syntax trees

[PATCH] D64565: [clangd] Don't run the prepare for tweaks that are disabled.

2019-07-11 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.h:136 -/// Returns true if the StringRef is a tweak that should be enabled -std::function TweakFilter = [](llvm::StringRef TweakToSearch) {return true;}; +/// Returns true if

[PATCH] D63835: [Syntax] Add nodes for most common statements

2019-07-10 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. This is ready for another round Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:25 /// A kind of a syntax node, used for implementing casts. enum class NodeKind : uint16_t { Leaf, sammccall wrote: > there are going to

[PATCH] D63835: [Syntax] Add nodes for most common statements

2019-07-10 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 208995. ilya-biryukov added a comment. - Mark groups of kinds for statements and expressions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63835/new/ https://reviews.llvm.org/D63835 Files:

[PATCH] D63835: [Syntax] Add nodes for most common statements

2019-07-10 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:185 +/// if (cond) else +class IfStatement final : public Statement { +public: sammccall wrote: > I guess the missing cond here (and similar below) are due to

[PATCH] D63835: [Syntax] Add nodes for most common statements

2019-07-10 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 208937. ilya-biryukov marked 5 inline comments as done. ilya-biryukov added a comment. - Rebase - Address comments - Restructure the roles - Remove the role from tree dumps for now With too many roles it is annoying to update the test outputs on

[PATCH] D64481: [clangd] Add a flag to clangdServer rename function to control whether we want format the replacements.

2019-07-10 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a small NIT. Was also thinking about adding a test for this, but the amount of work required to do so seems to outweigh the usefulness. Therefore seems ok to land

[PATCH] D63085: Provide a fix-it hint for -Wswitch, which adds missing cases. If there are >3 cases, the inserted text will contain newlines so it will not be shown in console output (but will be appl

2019-07-10 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. One major drawback that I see is the lack of indentation (and other format options) in the added code. Should we have this fix at a higher level that can have formatting (either now or in the future)? E.g. in `clangd` directly? Comment at:

[PATCH] D63835: [Syntax] Add nodes for most common statements

2019-07-09 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. Submitting a few comments to start up the discussions. The actual changes will follow. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:99 /// An abstract node for C++ statements, e.g.

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-07-09 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Relanded in rL365466 with a fix to the crash. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61637/new/ https://reviews.llvm.org/D61637

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-07-09 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. In D61637#1575542 , @RKSimon wrote: > @ilya-biryukov We're seeing buildbot failures in SyntaxTests.exe : > > http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/50927 > >

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-07-08 Thread Ilya Biryukov via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rGb736969eddce: [Syntax] Introduce syntax trees (authored by ilya-biryukov). Changed prior to commit: https://reviews.llvm.org/D61637?vs=208454=208455#toc Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-07-08 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:35 +/// A root node for a translation unit. Parent is always null. +class TranslationUnitDeclaration final : public Tree { +public: sammccall wrote: > I don't think TU is

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-07-08 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 208454. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - s/TranslationUnitDeclaration/TranslationUnit - Remove accessor from 'eof', add a FIXME to remove it from the tree altogether Repository: rG LLVM Github Monorepo

[PATCH] D61681: [clangd] A code tweak to expand a macro

2019-07-08 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp:57 + if (It == Spelled.begin()) +return Spelled.end(); + // Check the token we found actually touches the cursor position. sammccall wrote: > it's

[PATCH] D61681: [clangd] A code tweak to expand a macro

2019-07-08 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 208425. ilya-biryukov marked 5 inline comments as done. ilya-biryukov added a comment. - Replace bsearch with partition_point. - Include macro name in the title. - Added a FIXME for empty selection case. - Return null when no token is found.

[PATCH] D63835: [Syntax] Add nodes for most common statements

2019-06-26 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. This change mostly aims to illustrate that `TreeBuilder` seems to be powerful enough to go beyond basic nodes. But it also introduces enough nodes to make the syntax trees minimally useful for traversing statement nodes. Hopefully that could become a good basis to

[PATCH] D63835: [Syntax] Add nodes for most common statements

2019-06-26 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added a project: clang. ilya-biryukov added a parent revision: D61637: [Syntax] Introduce syntax trees. Most of the statements mirror the ones provided by clang AST. Major differences are: - expressions are

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-06-26 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. This is now in a pretty good shape, I've incorporated changes after our offline discussions about child roles. The builder interface is also much richer now, removing a requirement that the tree has to be traversed left-to-right (bottom-up is still required!).

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-06-26 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 206715. ilya-biryukov added a comment. - Remove (outdated) changes to gn files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61637/new/ https://reviews.llvm.org/D61637 Files:

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-06-26 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 206714. ilya-biryukov added a comment. - Introduce roles to allow distinguishing the child nodes. - Remove recovery node, use an unknown role instead. - TreeBuidler now can consume children at any point, not just suffix nodes. Repository: rG LLVM

[PATCH] D63817: [clangd] No longer getting template instantiations from header files in Main AST.

2019-06-26 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:47 #include +#include NIT: This include is redundant, maybe remove? Probably added by accident. Comment at:

[PATCH] D61681: [clangd] A code tweak to expand a macro

2019-06-25 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 206388. ilya-biryukov added a comment. - Rebase - Update some comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61681/new/ https://reviews.llvm.org/D61681 Files:

[PATCH] D63194: [clangd] Link and initialize target infos

2019-06-25 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:14 #include "clang/Tooling/CompilationDatabase.h" +#include "clang/Tooling/Tooling.h"

[PATCH] D63755: [clang][Tooling] Infer target and mode from argv[0] when using JSONCompilationDatabase

2019-06-25 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov 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/D63755/new/ https://reviews.llvm.org/D63755

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

2019-06-24 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:319 + /// 2. macro name and arguments for macro expansions. + using PPExpansions = llvm::DenseMap; class Builder; sammccall wrote: > do I understand right that

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

2019-06-24 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 206246. ilya-biryukov marked 10 inline comments as done. ilya-biryukov added a comment. - Address comments, document code. - s/Expansion/CollectedExpansions. - Added FIXMEs for macro arguments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver

2019-06-24 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Thanks, this looks very good! Leaving a few nitpicky comments, but nothing important really. Two more general NITs: - could we update the title and description of this revision to mirror that it focuses on code in tooling rather than in clangd? - maybe land the

[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver

2019-06-21 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. Sorry about the confusion, I can now see why the original version of the patch was actually simpler. I was put off by the fact that we override by adding command-line arguments instead of passing things around in the code, but it appears that's actually simpler

[PATCH] D63264: [clang][Driver] Deduce target triplet from clang executable name

2019-06-21 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. After looking at this, adjusting arguments on the clangd side seems to be a better approach. Let's abandon this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63264/new/ https://reviews.llvm.org/D63264

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-06-21 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. Although there are still rough edges, I believe the storage model is agreed upon and we can hopefully address the rest in the follow-ups. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-06-21 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov marked 13 inline comments as done. ilya-biryukov added a comment. This is ready for another round now. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:37 + /// and \p Last are added as children of the new node. + void learnNode(SourceLocation Fist,

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-06-21 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 205974. ilya-biryukov added a comment. - A few more renames and docs - Cleanups and comments - Reformat the code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61637/new/ https://reviews.llvm.org/D61637

[PATCH] D63621: [git-clang-format] recognize hxx as a C++ file

2019-06-21 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov added a comment. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63621/new/ https://reviews.llvm.org/D63621 ___

[PATCH] D63562: [clangd] Format changes produced by rename

2019-06-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, kadircet, sammccall. Herald added subscribers: arphaman, jkorous, MaskRay. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D63562 Files:

[PATCH] D61637: [Syntax] Introduce syntax trees

2019-06-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 205589. ilya-biryukov added a comment. - Do the renames Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61637/new/ https://reviews.llvm.org/D61637 Files: clang/include/clang/Tooling/Syntax/BuildTree.h

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

2019-06-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. To clarify, I don't think there are new concepts in this patch. Previously, we only took information from source locations into account when building token buffers. That works fine in most cases, but not enough to find the boundaries of empty macro expansions. In

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

2019-06-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 205537. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62953/new/ https://reviews.llvm.org/D62953 Files:

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

2019-06-19 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov marked 7 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:318 + /// 2. macro name and arguments for macro expansions. + using SpelledMappings = + llvm::DenseMap;

[PATCH] D62956: [clangd] Collect tokens of main files when building the AST

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. In D62956#1547826 , @sammccall wrote: > Can we add a test using TestTU that does a very basic verification of > expanded/spelled tokens (first after preamble, last token in file)? Done. Will land this tomorrow, want to

[PATCH] D62956: [clangd] Collect tokens of main files when building the AST

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 205387. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Rename tokens() to getTokens() for consistency with the rest of ParsedAST. - Update comments. - Add a test. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added a comment. In D62953#1547799 , @sammccall wrote: > Can you explain why this is important? > (in the code) I've added a few comments into the code that builds token buffers, but I couldn't figure out a good place to mention this in

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 205377. ilya-biryukov added a comment. - Added comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62953/new/ https://reviews.llvm.org/D62953 Files: clang/include/clang/Tooling/Syntax/Tokens.h

[PATCH] D62954: [Syntax] Add a helper to find expansion by its first spelled token

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 205374. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Added a message to the assertion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62954/new/

[PATCH] D62954: [Syntax] Add a helper to find expansion by its first spelled token

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:209 + ///#define FOO 1 2 3 // Expansion from "#define FOO 1" to an empty range. + ///FOO // Expansion from "FOO" to "1 2 3". + struct Expansion {

[PATCH] D62954: [Syntax] Add a helper to find expansion by its first spelled token

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov updated this revision to Diff 205352. ilya-biryukov marked 7 inline comments as done. ilya-biryukov added a comment. - Address comments. - s/findExpansion/expansionStartingAt. - Add tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D62538: [clangd] Add hidden tweaks to dump AST/selection.

2019-06-18 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few NITs Comment at: clangd/refactor/Tweak.h:104 + /// Is this a 'hidden' tweak, which are off by default. + virtual bool hidden() const { return

[PATCH] D63194: [clangd] Link in target infos and pass target and mode while invoking driver

2019-06-13 Thread Ilya Biryukov via Phabricator via cfe-commits
ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. Nice, thanks! The clangd parts obviously LGTM, but let's be more conservative with the driver bits (see the relevant comment) Comment at: clang-tools-extra/clangd/test/target_info.test:28 +} +#

  1   2   3   4   5   6   7   8   9   10   >