ioeric added a comment.
Do we consider enum helpers?
Comment at: clang-move/ClangMove.cpp:171
+assert(ED);
+MoveTool->getMovedDecls().push_back(ED);
+MoveTool->getUnremovedDeclsInOldHeader().erase(ED);
These 3 lines seen to be repeated. Maybe pull t
ioeric added a comment.
In https://reviews.llvm.org/D28228#633925, @hokein wrote:
> In https://reviews.llvm.org/D28228#633863, @ioeric wrote:
>
> > Do we consider enum helpers?
>
>
> This patch excludes enum helpers, only considers the enum declarations which
> are defined in old.h.
> Ideally,
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lg
Comment at: clang-move/ClangMove.cpp:456
+void ClangMoveTool::addMovedAndDeletedDecl(const NamedDecl *D) {
+ MovedDecls.push_back(D);
Remove this?
ht
ioeric created this revision.
ioeric added a reviewer: djasper.
ioeric added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
This cleans up "\n" among continuous lines when they are deleted.
https://reviews.llvm.org/D28235
Files:
lib/Format/Format.cpp
unittests/Format/CleanupT
ioeric updated this revision to Diff 82889.
ioeric added a comment.
- Fixed a nit.
https://reviews.llvm.org/D28235
Files:
lib/Format/Format.cpp
unittests/Format/CleanupTest.cpp
Index: unittests/Format/CleanupTest.cpp
===
--- u
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lg
https://reviews.llvm.org/D28279
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
ioeric created this revision.
ioeric added a reviewer: hokein.
ioeric added a subscriber: cfe-commits.
https://reviews.llvm.org/D28282
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
==
ioeric updated this revision to Diff 83024.
ioeric marked an inline comment as done.
ioeric added a comment.
- Get rid of hacky replacement.
https://reviews.llvm.org/D28282
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/chan
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:563
+ // Create a replacement merely for retrieving file path and start offset.
+ const auto R = createReplacement(Start, Start, "", *Result.SourceManager);
MoveNamespace MoveNs;
ioeric updated this revision to Diff 83051.
ioeric marked an inline comment as done.
ioeric added a comment.
- fix a nit.
https://reviews.llvm.org/D28282
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/Change
This revision was automatically updated to reflect the committed changes.
Closed by commit rL290966: [change-namespace] get newlines around moved
namespace right. (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D28282?vs=83051&id=83052#toc
Repository:
rL LLVM
https:/
ioeric added a comment.
I'm not entirely sure about this change... looping in Manuel.
https://reviews.llvm.org/D28419
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric added a comment.
I ran `ninja check-all` with https://reviews.llvm.org/D28081 and
https://reviews.llvm.org/D28315 (in Linux), and some lit tests failed, namely:
Clang :: Format/style-on-command-line.cpp
Clang Tools :: clang-apply-replacements/basic.cpp
Clang Tools :: clang-apply-rep
ioeric added a comment.
post-vacation ping :)
https://reviews.llvm.org/D28235
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric created this revision.
https://reviews.llvm.org/D31076
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- un
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:291
+ assert(!SymbolSplitted.empty());
+ SymbolSplitted.pop_back();
+
hokein wrote:
> Is this needed? Looks like you are removing the name of the symbol here, but
> from the code be
ioeric updated this revision to Diff 92138.
ioeric marked 3 inline comments as done.
ioeric added a comment.
- Merged with origin/master
- Addressed comments. Merged with origin/master.
https://reviews.llvm.org/D30493
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/Ch
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298090: [change-namespace] do not rename specialized
template parameters. (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D31076?vs=92135&id=92141#toc
Repository:
rL LLVM
htt
ioeric added a comment.
Ping ;)
https://reviews.llvm.org/D30493
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric added a comment.
Ping ;)
https://reviews.llvm.org/D30777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric added a comment.
I think this is a great start!
First round with some nits.
Comment at: clang-rename/RenamingAction.cpp:87
+class QualifiedRenamingASTConsumer : public ASTConsumer {
+public:
Comments.
Comment at: clang-rename/Renam
ioeric added inline comments.
Comment at: change-namespace/ChangeNamespace.cpp:296
+assert(!NsSplitted.empty());
+for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) {
+ if (*I == SymbolSplitted.front())
hokein wrote:
> ioeric wr
ioeric updated this revision to Diff 92475.
ioeric added a comment.
- fixed newly added tests.
https://reviews.llvm.org/D30493
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298363: [change-namespace] avoid adding leading '::' when
possible. (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D30493?vs=92475&id=92476#toc
Repository:
rL LLVM
https://r
ioeric added inline comments.
Comment at: clang-rename/USRFinder.cpp:200
+ // Also find all USRs of nested declarations.
+ NestedNameSpecifierLocFinder Finder(const_cast(Context));
ioeric wrote:
> It is unclear to me what `nested declarations` are.
But what i
ioeric added inline comments.
Comment at: clang-rename/USRLocFinder.cpp:195
+// Find all locations identified by the given USRs. Traverse the AST and find
+// every AST node whose USR is in the given USRs' set.
+class RenameLocFinder
hokein wrote:
> ioeric wrote:
ioeric updated this revision to Diff 93227.
ioeric added a comment.
- generate html for the new matcher.
https://reviews.llvm.org/D28671
Files:
docs/LibASTMatchersReference.html
include/clang/ASTMatchers/ASTMatchers.h
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
Index: unittests/ASTMat
ioeric added a comment.
PTAL
https://reviews.llvm.org/D28671
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric updated this revision to Diff 93228.
ioeric marked an inline comment as done.
ioeric added a comment.
- Register the matcher to Dynamic.
https://reviews.llvm.org/D28671
Files:
docs/LibASTMatchersReference.html
include/clang/ASTMatchers/ASTMatchers.h
lib/ASTMatchers/Dynamic/Registry
ioeric added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:191
+const internal::VariadicDynCastAllOfMatcher
+typeAliasTemplateDecl;
+
aaron.ballman wrote:
> Be sure to add this to Registry.cpp so that it can be used as a dynamic
> matc
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298912: [ASTMatchers] add typeAliasTemplateDecl matcher.
(authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D28671?vs=93228&id=93230#toc
Repository:
rL LLVM
https://reviews.llvm
ioeric updated this revision to Diff 93231.
ioeric added a comment.
- addressed comment.
https://reviews.llvm.org/D30777
Files:
include/clang/Tooling/Refactoring/AtomicChange.h
lib/Tooling/Refactoring/AtomicChange.cpp
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/Refacto
This revision was automatically updated to reflect the committed changes.
Closed by commit rL298913: Added `applyAtomicChanges` function. (authored by
ioeric).
Changed prior to commit:
https://reviews.llvm.org/D30777?vs=93231&id=93232#toc
Repository:
rL LLVM
https://reviews.llvm.org/D30777
ioeric added a comment.
Please see comment from a previous patch
(https://reviews.llvm.org/D27054?id=79099#inline-234270). Generally,
`AtomicChange` is a higher level of abstraction than `Replacement`, and we
don't want users to deal with `Replacement` and the related errors.
https://reviews.
ioeric added a comment.
In https://reviews.llvm.org/D31492#713908, @alexshap wrote:
> to avoid misunderstanding - are the tools like clang-rename, change-namespace
> and clang-reorder-fields supposed to use AtomicChange (via the methods
> insert, replace etc) ?
> (i am asking just to make sure
ioeric added a comment.
Could you add a simple test?
https://reviews.llvm.org/D31492
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lg with a few nits.
Comment at: clang-rename/RenamingAction.cpp:104
+ USRList[I], NewNames[I], Context.getTranslationUnitDecl());
+ for (const auto AtomicChange
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Looks good and thanks for the cleanup.
https://reviews.llvm.org/D31492
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.
ioeric added a comment.
Still LGTM. Please run `ninja check-clang` and fix the warning. I'll land this
patch for you afterwards.
Comment at: lib/Tooling/RefactoringCallbacks.cpp:222
+}
+default:
+ llvm_unreachable("Element.Type not recognized");
`
ioeric updated this revision to Diff 94497.
ioeric added a comment.
Herald added a subscriber: mgorny.
- Try to unbreak buildbots after r298913.
- Add clangFormat to dependency
- Update cmake
https://reviews.llvm.org/D30777
Files:
include/clang/Tooling/Refactoring/AtomicChange.h
lib/Tooling
ioeric reopened this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
This (https://reviews.llvm.org/rL298913) was reverted upstream due to cyclic
dependency on GreenDragon bot. Investigating...
Repository:
rL LLVM
https://reviews.llvm.org/D30777
ioeric created this revision.
ioeric added a reviewer: hokein.
ioeric added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
https://reviews.llvm.org/D28671
Files:
include/clang/ASTMatchers/ASTMatchers.h
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
Index: unittests/ASTMatchers
ioeric created this revision.
ioeric added a reviewer: klimek.
ioeric added a subscriber: cfe-commits.
https://reviews.llvm.org/D28672
Files:
docs/LibASTMatchersReference.html
Index: docs/LibASTMatchersReference.html
===
--- docs/
ioeric added a comment.
In https://reviews.llvm.org/D28672#646336, @malcolm.parsons wrote:
> In https://reviews.llvm.org/D28672#646151, @aaron.ballman wrote:
>
> > I'm not seeing anything wrong, per se, but why has so much of this file
> > changed recently?
>
>
> The script looks for doxygen doc
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lg
https://reviews.llvm.org/D28293
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
ioeric added a comment.
Let me know when broken tests are fixed and this patch (and the corresponding
patch) is ready again for review. Also let me know if you need any help.
Comment at: change-namespace/ChangeNamespace.cpp:892
+ llvm::errs() << llvm::toString(Style.takeE
ioeric added a comment.
In https://reviews.llvm.org/D28315#647154, @amaiorano wrote:
> In https://reviews.llvm.org/D28315#647103, @ioeric wrote:
>
> > Let me know when broken tests are fixed and this patch (and the
> > corresponding patch) is ready again for review. Also let me know if you
> >
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lgtm. Thanks!
https://reviews.llvm.org/D28081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listi
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lgtm. Thanks!
https://reviews.llvm.org/D28315
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listi
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lg.
Comment at: test/clang-move/no-move-macro-helpers.cpp:1
+// RUN: mkdir -p %T/no-move-macro-helper
+// RUN: cp %S/Inputs/macro_helper_test.h
%T/no-move-macro-helper/macr
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
@davide I think this change makes sense. I'll accept this to unbreak our
internal build. Let us know if you have any concern.
https://reviews.llvm.org/D28799
__
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lgtm.
https://reviews.llvm.org/D28801
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
ioeric requested changes to this revision.
ioeric added a comment.
This revision now requires changes to proceed.
Per offline discussion, this change is not intact - we want using decls in
headers to be moveable symbols as well since it can have references outside of
the local TU.
https://revi
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
@amaiorano: The test itself is correct. It's just that this test failed in our
internal test. We could've fixed it internally, but the fix would be ugly.
Since the intended behavior is already
ioeric added a comment.
In https://reviews.llvm.org/D28943#651488, @amaiorano wrote:
> In https://reviews.llvm.org/D28943#651470, @ioeric wrote:
>
> > @amaiorano: The test itself is correct. It's just that this test failed in
> > our internal test. We could've fixed it internally, but the fix wo
ioeric added a comment.
In https://reviews.llvm.org/D28943#651536, @amaiorano wrote:
> In https://reviews.llvm.org/D28943#651489, @ioeric wrote:
>
> > In https://reviews.llvm.org/D28943#651488, @amaiorano wrote:
> >
> > > In https://reviews.llvm.org/D28943#651470, @ioeric wrote:
> > >
> > > > @am
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Lgtm
https://reviews.llvm.org/D28983
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
ioeric created this revision.
For example, when we change 'na' to "nb::nc", we need to add leading '::' to
references "::nc::X" in the changed namespace.
https://reviews.llvm.org/D29176
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: u
ioeric updated this revision to Diff 85903.
ioeric marked 3 inline comments as done.
ioeric added a comment.
- Addressed comments.
https://reviews.llvm.org/D29176
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespa
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293182: [change-namespace] add leading '::' to references in
new namespace when name… (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D29176?vs=85903&id=85904#toc
Repository:
ioeric created this revision.
https://reviews.llvm.org/D29182
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- un
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293187: [change-namespace] correctly shorten namespace when
references have leading '::' (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D29182?vs=85917&id=85919#toc
Repository:
ioeric created this revision.
This fixes mismatch between template decls and template specialization decls.
Also added a few more test cases.
https://reviews.llvm.org/D29447
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/ch
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293897: [change-namespace] check using shadow decl correctly
when shortening namespace… (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D29447?vs=86795&id=86815#toc
Repository:
ioeric created this revision.
https://reviews.llvm.org/D29460
Files:
change-namespace/ChangeNamespace.cpp
unittests/change-namespace/ChangeNamespaceTests.cpp
Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- uni
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293909: [change-namespace] fix unscoped enum constant
references. (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D29460?vs=86835&id=86843#toc
Repository:
rL LLVM
https://rev
ioeric added inline comments.
Comment at: lib/Tooling/RefactoringCallbacks.cpp:42
+ void HandleTranslationUnit(ASTContext &Context) override {
+for (const auto &Callback : Refactoring.Callbacks) {
+ Callback->getReplacements().clear();
Could you add a c
ioeric added inline comments.
Comment at: clang-query/QueryReplace.cpp:1
+#include "QueryReplace.h"
+#include "QueryParser.h"
Please add license header.
Comment at: clang-query/QueryReplace.cpp:37
+
+llvm::ErrorOr QueryReplaceSpec::parseFromJSO
ioeric added inline comments.
Comment at: lib/Tooling/RefactoringCallbacks.cpp:160
+llvm::Expected>
+ReplaceNodeWithTemplate::create(StringRef FromId, StringRef ToTemplate) {
+ std::vector ParsedTemplate;
Is this covered in the test?
Comment a
ioeric added a comment.
In https://reviews.llvm.org/D34696#793613, @klimek wrote:
> The main thing I'm concerned about is having the main code in core, but
> having all tests in tools-extra. I think if we go that route we should also
> move clang-rename and its tests to core. Thoughts?
Agreed
ioeric created this revision.
White spaces in file names are causing Phabricator/SVN to crash.
https://reviews.llvm.org/D35203
Files:
test/Driver/crash report spaces.c
test/Driver/crash-report-spaces.c
Index: test/Driver/crash-report-spaces.c
==
This revision was automatically updated to reflect the committed changes.
Closed by commit rL307551: Avoid white spaces in file names. NFC (authored by
ioeric).
Changed prior to commit:
https://reviews.llvm.org/D35203?vs=105860&id=105871#toc
Repository:
rL LLVM
https://reviews.llvm.org/D352
ioeric added a comment.
Some nits.
Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:23
+/// of the cursor or overlap with the selection range.
+class ASTSelectionFinder : public RecursiveASTVisitor {
+ const SourceLocation Location;
In LLVM, we use the fol
ioeric created this revision.
ioeric added a reviewer: bkramer.
Herald added subscribers: cfe-commits, klimek.
As result deduplication or reduction is not supported in the framework,
we should leave the deplication to tools (if needed) until the framework
supports it.
Repository:
rC Clang
ht
ioeric created this revision.
ioeric added reviewers: bkramer, sammccall.
Herald added subscribers: cfe-commits, ilya-biryukov, klimek.
After https://reviews.llvm.org/D42111, the executor framework no longer
deduplicate tool results.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.or
ioeric updated this revision to Diff 130201.
ioeric marked 17 inline comments as done.
ioeric added a comment.
Herald added a reviewer: jkorous-apple.
- Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41946
Files:
clangd/CMakeLists.txt
clangd/URI.c
ioeric added a comment.
Thanks for the comments! I've refactored the code according to your
suggestions. PTAL
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41946
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists
This revision was automatically updated to reflect the committed changes.
Closed by commit rC322691: [Tooling] Don't deduplicate tool results in the
All-TUs executor. (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42111?vs=129982&id=130204#toc
Repositor
ioeric updated this revision to Diff 130207.
ioeric marked 3 inline comments as done.
ioeric added a comment.
Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42113
Files:
clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
Index: clangd/global
ioeric added inline comments.
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:109
+ // Deduplicate the result by key.
+ // FIXME(ioeric): we need a better way to merge symbols with the same key.
For
+ // example, class forward-declarations can have the sa
This revision was automatically updated to reflect the committed changes.
Closed by commit rL322722: [clangd] Deduplicate symbols collected in
global-symbol-builder tool. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/
ioeric added inline comments.
Comment at: unittests/clangd/SymbolCollectorTests.cpp:164
+TEST_F(SymbolCollectorTest, IncludeEnums) {
+ CollectorOpts.IndexMainFiles = false;
Could you add a test case like the following and check whether `ns::X` is in
the resul
ioeric added a comment.
Herald added a reviewer: jkorous-apple.
Overall looks good! Some ideas about code structure inlined.
Comment at: clangd/CodeComplete.cpp:327
+// The CompletionRecorder captures Sema code-complete output, including
context.
+// It filters out ignored res
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Nice! Thanks for making the refactoring and adding tests! I think this is good
to go now.
I'm not very familiar with code outside of the index library (Driver, Basic
etc), but the changes see
ioeric added a comment.
LGTM
Comment at: clangd/CodeComplete.cpp:743
+ int NSema = 0, NIndex = 0, NBoth = 0; // Counters for logging.
+ bool Incomplete = false;
+ llvm::Optional Filter; // Initialized once Sema runs.
`InComplete` can probably be output varia
ioeric updated this revision to Diff 130621.
ioeric marked 14 inline comments as done.
ioeric added a comment.
- Address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41946
Files:
clangd/CMakeLists.txt
clangd/URI.cpp
clangd/URI.h
unittests/clangd/CMakeL
ioeric added inline comments.
Comment at: clangd/URI.cpp:43
+ Body.consume_front("/");
+return llvm::sys::path::convert_to_slash(Body);
+ }
sammccall wrote:
> Don't you want the opposite here, convert from unix to native?
Ah, right!
C
ioeric updated this revision to Diff 130766.
ioeric marked 16 inline comments as done.
ioeric added a comment.
- Addressed review comments.
- Check windows vs unit paths in tests.
- Properly check absolute paths.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41946
Files:
cla
ioeric added inline comments.
Comment at: clangd/URI.cpp:57
+ Body = "/";
+Body += path::convert_to_slash(AbsolutePath, path::Style::posix);
+return (llvm::Twine(Scheme) + ":" + percentEncode(Body)).str();
sammccall wrote:
> ioeric wrote:
> > sammcca
ioeric created this revision.
ioeric added a reviewer: hokein.
Herald added subscribers: cfe-commits, klimek.
Repository:
rC Clang
https://reviews.llvm.org/D42361
Files:
include/clang/Tooling/Tooling.h
lib/Tooling/Tooling.cpp
Index: lib/Tooling/Tooling.cpp
===
ioeric added inline comments.
Comment at: include/clang/Tooling/Tooling.h:331
+ /// \returns 0 on success; 1 if any error occured; 2 if there is no error but
+ /// some files are skipped due to missing compile commands.
int run(ToolAction *Action);
hokein wr
ioeric updated this revision to Diff 130862.
ioeric marked 4 inline comments as done.
ioeric added a comment.
- Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41946
Files:
clangd/CMakeLists.txt
clangd/URI.cpp
clangd/URI.h
unittests/clangd/CMak
ioeric added inline comments.
Comment at: clangd/URI.h:31
+public:
+ /// \brief Returns decoded scheme e.g. "https"
+ llvm::StringRef scheme() const { return Scheme; }
sammccall wrote:
> nit: prefer to omit `\brief` unless you want the brief comment to be
> so
This revision was automatically updated to reflect the committed changes.
Closed by commit rL323101: [clangd] Add support for different file URI schemas.
(authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D41946
Files:
ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, jkorous-apple, ilya-biryukov, klimek.
o Replace the existing clangd::URI with a wrapper of FileURI which also
carries a resolved file path.
o s/FileURI/URI/
Repository:
rCTE Clang Tools Ext
ioeric added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:386
+ if (!U)
+llvm_unreachable(
+"onDiagnosticsReady: Expect creating URI for file to always work.");
not sure if this is the right thing to do. idea?
Comment at
ioeric added inline comments.
Comment at: clangd/ClangdServer.h:324
+ /// Returns estimated memory usage for each of the currently files.
+ /// The order of results is unspecified.
s/currently/current/ ? or current open?
Comment at: clangd/
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
lg
Comment at: clangd/ClangdUnit.cpp:664
+ std::lock_guard Lock(Mutex);
+ return ASTMemUsage + PreambleMemUsage;
+}
ilya-biryukov wrote:
> ioeric wrote:
>
ioeric updated this revision to Diff 131576.
ioeric marked 8 inline comments as done.
ioeric added a comment.
- Address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42419
Files:
clangd/ClangdLSPServer.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/UR
ioeric added a comment.
Thanks for the comments!
Comment at: clangd/ClangdLSPServer.cpp:284
+ std::string ResultUri = "";
+ if (Result) {
+auto U = URI::create(*Result);
sammccall wrote:
> But basically I think this shows that the API is awkward. We shoul
1301 - 1400 of 1511 matches
Mail list logo
