Re: [clang-tools-extra] r351788 - [clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.
Hey Hans,
There is actually a parent for this patch at
https://reviews.llvm.org/rC351531 and it hasn't been merged.
In addition to that not exactly as a follow-up, but there is
https://reviews.llvm.org/rCTE351738 which is kind of related to this two
patches, you might also consider merging that but it is more of a NFC.
On Tue, Jan 22, 2019 at 8:08 PM Hans Wennborg wrote:
> This has been merged to the 8.0 branch in r351860. Please let me know
> if there are any follow-ups so they can be merged too.
>
> Thanks,
> Hans
>
> On Tue, Jan 22, 2019 at 1:10 AM Kadir Cetinkaya via cfe-commits
> wrote:
> >
> > Author: kadircet
> > Date: Tue Jan 22 01:10:20 2019
> > New Revision: 351788
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=351788&view=rev
> > Log:
> > [clangd] Filter out plugin related flags and move all commandline
> manipulations into OverlayCDB.
> >
> > Summary:
> > Some projects make use of clang plugins when building, but clangd is
> > not aware of those plugins therefore can't work with the same compile
> command
> > arguments.
> >
> > There were multiple places clangd performed commandline manipulations,
> > this one also moves them all into OverlayCDB.
> >
> > Reviewers: ilya-biryukov
> >
> > Subscribers: klimek, sammccall, ioeric, MaskRay, jkorous, arphaman,
> cfe-commits
> >
> > Differential Revision: https://reviews.llvm.org/D56841
> >
> > Modified:
> > clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> > clang-tools-extra/trunk/clangd/ClangdServer.cpp
> > clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
> > clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
> > clang-tools-extra/trunk/clangd/index/Background.cpp
> > clang-tools-extra/trunk/clangd/index/Background.h
> > clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
> > clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
> >
> clang-tools-extra/trunk/unittests/clangd/GlobalCompilationDatabaseTests.cpp
> >
> > Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=351788&r1=351787&r2=351788&view=diff
> >
> ==
> > --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
> > +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Jan 22
> 01:10:20 2019
> > @@ -289,7 +289,8 @@ void ClangdLSPServer::onInitialize(const
> >if (UseDirBasedCDB)
> > BaseCDB =
> llvm::make_unique(
> > CompileCommandsDir);
> > - CDB.emplace(BaseCDB.get(),
> Params.initializationOptions.fallbackFlags);
> > + CDB.emplace(BaseCDB.get(), Params.initializationOptions.fallbackFlags,
> > + ClangdServerOpts.ResourceDir);
> >Server.emplace(*CDB, FSProvider, static_cast &>(*this),
> > ClangdServerOpts);
> >applyConfiguration(Params.initializationOptions.ConfigSettings);
> >
> > Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=351788&r1=351787&r2=351788&view=diff
> >
> ==
> > --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
> > +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Tue Jan 22 01:10:20
> 2019
> > @@ -37,11 +37,6 @@ namespace clang {
> > namespace clangd {
> > namespace {
> >
> > -std::string getStandardResourceDir() {
> > - static int Dummy; // Just an address in this process.
> > - return CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy);
> > -}
> > -
> > class RefactoringResultCollector final
> > : public tooling::RefactoringResultConsumer {
> > public:
> > @@ -107,8 +102,6 @@ ClangdServer::ClangdServer(const GlobalC
> > DiagnosticsConsumer &DiagConsumer,
> > const Options &Opts)
> > : CDB(CDB), FSProvider(FSProvider),
> > - ResourceDir(Opts.ResourceDir ? *Opts.ResourceDir
> > - : getStandardResourceDir()),
> >DynamicIdx(Opts.BuildDynamicSymbolIndex
> > ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex)
> > : nullptr),
> > @@ -136,7 +129,7 @@ ClangdServer::ClangdServer(const GlobalC
> > AddIndex(Opts.StaticIndex);
> >if (Opts.BackgroundIndex) {
> > BackgroundIdx = llvm::make_unique(
> > -Context::current().clone(), ResourceDir, FSProvider, CDB,
> > +Context::current().clone(), FSProvider, CDB,
> > BackgroundIndexStorage::createDiskBackedStorageFactory(),
> > Opts.BackgroundIndexRebuildPeriodMs);
> > AddIndex(BackgroundIdx.get());
> > @@ -461,10 +454,6 @@ tooling::CompileCommand ClangdServer::ge
> >llvm::Optional C =
> CDB.getCompileCommand(File);
> >if (!C) // FIXME: Suppress diagnostics? Let the user k
Re: [clangd] Print template arguments helper
Thanks Bruno, sent out rL358293 to address the issue. On Fri, Apr 12, 2019 at 5:21 PM Bruno Ricci wrote: > Hi, > > It seems that one of r358272, r358273 or r358274 is causing some asan > failure on my machine. Not sure why it is not spotted by the bots. > > Failure log attached. > > Bruno > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r358665 - [clang][CIndex] Use llvm::set_thread_priority
Thanks for the fix Nico!
On Sun, Apr 21, 2019 at 9:17 PM Nico Weber wrote:
> r358858 might help with this.
>
> On Sat, Apr 20, 2019 at 7:15 PM Nico Weber wrote:
>
>> This breaks building with LLVM_ENABLE_THREADS=OFF. The call probably
>> needs to be behind a `#if LLVM_ENABLE_THREADS`.
>>
>> FAILED: bin/c-index-test
>> ...
>> lib/libclang.a(CIndex.cpp.o): In function `void llvm::function_ref> ()>::callback_fn(long)':
>> CIndex.cpp:(.text._ZN4llvm12function_refIFvvEE11callback_fnIZ25clang_saveTranslationUnitEUlvE_EEvl+0x5a):
>> undefined reference to `llvm::set_thread_priority(llvm::ThreadPriority)'
>> lib/libclang.a(CIndex.cpp.o): In function
>> `clang::setThreadBackgroundPriority()':
>> CIndex.cpp:(.text._ZN5clang27setThreadBackgroundPriorityEv+0x27):
>> undefined reference to `llvm::set_thread_priority(llvm::ThreadPriority)'
>> lib/libclang.a(CIndex.cpp.o): In function `clang_saveTranslationUnit':
>> CIndex.cpp:(.text.clang_saveTranslationUnit+0x316): undefined reference
>> to `llvm::set_thread_priority(llvm::ThreadPriority)'
>>
>>
>> On Thu, Apr 18, 2019 at 9:47 AM Kadir Cetinkaya via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: kadircet
>>> Date: Thu Apr 18 06:49:20 2019
>>> New Revision: 358665
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=358665&view=rev
>>> Log:
>>> [clang][CIndex] Use llvm::set_thread_priority
>>>
>>> Reviewers: jkorous, gribozavr
>>>
>>> Subscribers: dexonsmith, arphaman, cfe-commits
>>>
>>> Tags: #clang
>>>
>>> Differential Revision: https://reviews.llvm.org/D60867
>>>
>>> Modified:
>>> cfe/trunk/tools/libclang/CIndex.cpp
>>>
>>> Modified: cfe/trunk/tools/libclang/CIndex.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CIndex.cpp?rev=358665&r1=358664&r2=358665&view=diff
>>>
>>> ==
>>> --- cfe/trunk/tools/libclang/CIndex.cpp (original)
>>> +++ cfe/trunk/tools/libclang/CIndex.cpp Thu Apr 18 06:49:20 2019
>>> @@ -8723,9 +8723,7 @@ void clang::setThreadBackgroundPriority(
>>>if (getenv("LIBCLANG_BGPRIO_DISABLE"))
>>> return;
>>>
>>> -#ifdef USE_DARWIN_THREADS
>>> - setpriority(PRIO_DARWIN_THREAD, 0, PRIO_DARWIN_BG);
>>> -#endif
>>> + llvm::set_thread_priority(llvm::ThreadPriority::Background);
>>> }
>>>
>>> void cxindex::printDiagsToStderr(ASTUnit *Unit) {
>>>
>>>
>>> ___
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang-tools-extra] r366458 - [clangd] Refactor background-index shard loading
Hi Azhar, D64980 should fix the problem. I am reverting your revert while
adding the fix in r366559.
On Fri, Jul 19, 2019 at 11:29 AM Azhar Mohammed wrote:
> Reverted in r366551.
>
>
> Revert r366458, r366467 and r366468
>
> r366458 is causing test failures. r366467 and r366468 had to be
> reverted as
> they were casuing conflict while reverting r366458.
>
> r366468 [clangd] Remove dead code from BackgroundIndex
> r366467 [clangd] BackgroundIndex stores shards to the closest project
> r366458 [clangd] Refactor background-index shard loading
>
> On Jul 18, 2019, at 6:21 PM, Azhar Mohammed wrote:
>
> Hi Kadir
>
> This change is causing test failures, can you please look into it. Refer
> to
> http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/58104/testReport/
> .
>
> Assertion failed: (TUsIt != FileToTU.end() && "No TU registered for the
> shard"), function takeResult, file
> /Users/buildslave/jenkins/workspace/clang-stage1-configure-RA/llvm/tools/clang/tools/extra/clangd/index/BackgroundIndexLoader.cpp,
> line 131.
>
>
>
> Failing Tests (10):
> Clangd :: did-change-configuration-params.test
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.CmdLineHash
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.DirectIncludesTest
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.IndexTwoFiles
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.NoCrashOnErrorFile
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.NoDotsInAbsPath
> Clangd Unit Tests ::
> ./ClangdTests/BackgroundIndexTest.ShardStorageEmptyFile
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.ShardStorageLoad
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.ShardStorageTest
> Clangd Unit Tests :: ./ClangdTests/BackgroundIndexTest.UncompilableFiles
>
>
>
> On Jul 18, 2019, at 9:25 AM, Kadir Cetinkaya via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> Author: kadircet
> Date: Thu Jul 18 09:25:36 2019
> New Revision: 366458
>
> URL: http://llvm.org/viewvc/llvm-project?rev=366458&view=rev
> Log:
> [clangd] Refactor background-index shard loading
>
> Reviewers: sammccall
>
> Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D64712
>
> Added:
>clang-tools-extra/trunk/clangd/index/BackgroundIndexLoader.cpp
>clang-tools-extra/trunk/clangd/index/BackgroundIndexLoader.h
> Modified:
>clang-tools-extra/trunk/clangd/CMakeLists.txt
>clang-tools-extra/trunk/clangd/index/Background.cpp
>clang-tools-extra/trunk/clangd/index/Background.h
>clang-tools-extra/trunk/clangd/index/BackgroundRebuild.cpp
>clang-tools-extra/trunk/clangd/index/BackgroundRebuild.h
>clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
>
> Modified: clang-tools-extra/trunk/clangd/CMakeLists.txt
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CMakeLists.txt?rev=366458&r1=366457&r2=366458&view=diff
>
> ==
> --- clang-tools-extra/trunk/clangd/CMakeLists.txt (original)
> +++ clang-tools-extra/trunk/clangd/CMakeLists.txt Thu Jul 18 09:25:36 2019
> @@ -73,6 +73,7 @@ add_clang_library(clangDaemon
> XRefs.cpp
>
> index/Background.cpp
> + index/BackgroundIndexLoader.cpp
> index/BackgroundIndexStorage.cpp
> index/BackgroundQueue.cpp
> index/BackgroundRebuild.cpp
>
> Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=366458&r1=366457&r2=366458&view=diff
>
> ==
> --- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
> +++ clang-tools-extra/trunk/clangd/index/Background.cpp Thu Jul 18
> 09:25:36 2019
> @@ -10,6 +10,7 @@
> #include "ClangdUnit.h"
> #include "Compiler.h"
> #include "Context.h"
> +#include "FSProvider.h"
> #include "Headers.h"
> #include "Logger.h"
> #include "Path.h"
> @@ -18,6 +19,7 @@
> #include "Threading.h"
> #include "Trace.h"
> #include "URI.h"
> +#include "index/BackgroundIndexLoader.h"
> #include "index/FileIndex.h"
> #include "index/IndexAction.h"
> #include "index/MemIndex.h"
> @@ -28,6 +30,8 @@
> #include "clang/Basic/SourceLocation.h"
> #include "clang/Basic/SourceManager.h"
> #include "clang/Driver/Types.h"
> +#include "llvm/ADT/ArrayRef.h"
> +#include "llvm/ADT/DenseSet.h"
> #include "llvm/ADT/Hashing.h"
> #include "llvm/ADT/STLExtras.h"
> #include "llvm/ADT/ScopeExit.h"
> @@ -42,6 +46,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -49,6 +54,8 @@
> #include
> #include
> #include
> +#include
> +#include
>
> namespace clang {
> namespace clangd {
> @@ -119,6 +126,18 @@ llvm::SmallString<128> getAbsolutePath(c
> }
> return AbsolutePath;
> }
> +
> +bo
Re: [clang-tools-extra] r365123 - [clangd] Make HadErrors part of background index's internal state
https://reviews.llvm.org/rL365140 should fix it, please let me know if it
doesnt.
On Thu, Jul 4, 2019 at 3:45 PM wrote:
> Hi Kadir,
>
> Your change is causing a build failure on our internal linux build bot
> running gcc 5.4:
>
> FAILED: CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache
> /usr/lib/ccache/g++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE
> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
> -Itools/clang/tools/extra/clangd
> -I/home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd
> -I/home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/include
> -Itools/clang/include -Iinclude
> -I/home/siadmin/jenkins/w/opensource/opensource_build/llvm/include -fPIC
> -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra
> -Wno-unused-parameter -Wwrite-strings -Wcast-qual
> -Wno-missing-field-initializers -pedantic -Wno-long-long
> -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment
> -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common
> -Woverloaded-virtual -fno-strict-aliasing -O3-UNDEBUG -fno-exceptions
> -fno-rtti -MMD -MT
> tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o
> -MF
> tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o.d
> -o
> tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o
> -c
> /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.cpp
> In file included from
> /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdServer.h:24:0,
> from
> /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.h:12,
> from
> /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.cpp:9:
> /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/index/Background.h:99:24:
> error: array must be initialized with a brace-enclosed initializer
> FileDigest Digest{0};
> ^
>
> Can you please take a look?
>
> Douglas Yung
>
> -Original Message-
> From: cfe-commits On Behalf Of Kadir
> Cetinkaya via cfe-commits
> Sent: Thursday, July 4, 2019 2:52
> To: cfe-commits@lists.llvm.org
> Subject: [clang-tools-extra] r365123 - [clangd] Make HadErrors part of
> background index's internal state
>
> Author: kadircet
> Date: Thu Jul 4 02:52:12 2019
> New Revision: 365123
>
> URL: http://llvm.org/viewvc/llvm-project?rev=365123&view=rev
> Log:
> [clangd] Make HadErrors part of background index's internal state
>
> Reviewers: sammccall
>
> Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D64147
>
> Modified:
> clang-tools-extra/trunk/clangd/index/Background.cpp
> clang-tools-extra/trunk/clangd/index/Background.h
> clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
>
> Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=365123&r1=365122&r2=365123&view=diff
>
> ==
> --- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
> +++ clang-tools-extra/trunk/clangd/index/Background.cpp Thu Jul 4
> +++ 02:52:12 2019
> @@ -101,28 +101,6 @@ IncludeGraph getSubGraph(const URI &U, c
>return IG;
> }
>
> -// Creates a filter to not collect index results from files with
> unchanged -// digests.
> -// \p FileDigests contains file digests for the current indexed files.
> -decltype(SymbolCollector::Options::FileFilter)
> -createFileFilter(const llvm::StringMap &FileDigests) {
> - return [&FileDigests](const SourceManager &SM, FileID FID) {
> -const auto *F = SM.getFileEntryForID(FID);
> -if (!F)
> - return false; // Skip invalid files.
> -auto AbsPath = getCanonicalPath(F, SM);
> -if (!AbsPath)
> - return false; // Skip files without absolute path.
> -auto Digest = digestFile(SM, FID);
> -if (!Digest)
> - return false;
> -auto D = FileDigests.find(*AbsPath);
> -if (D != FileDigests.end() && D->second == Digest)
> - return false; // Skip files that haven't changed.
> -return true;
> - };
> -}
> -
> // We cannot use vfs->makeAbsolute because Cmd.FileName is either
> absolute or // relative to Cmd.Directory, which might not be the same as
> current working // directory.
> @@ -274,12 +252,12 @@ void BackgroundIndex::enqueueTask(Task T }
>
> /// Given index results from a TU, only update symbols coming from files
> that -/// are different or missing from than \p DigestsSnapshot. Also
> stores new index -/// information on IndexStorage.
> -void BackgroundIndex::update(llvm::
Re: [clang-tools-extra] r374163 - [clangd] Propagate context into reply handlers
thanks! sent out D68702.
On Wed, Oct 9, 2019 at 2:56 PM Nico Weber wrote:
> Looks like this breaks three clangd tests on macOS:
>
> Failing Tests (3):
> Clangd :: code-action-request.test
> Clangd :: execute-command.test
> Clangd :: tweaks-format.test
>
> libc++abi.dylib: terminating with uncaught exception of type
> std::__1::system_error: mutex lock failed: Invalid argument
>
> http://45.33.8.238/mac/1245/step_7.txt
>
> On Wed, Oct 9, 2019 at 8:46 AM Kadir Cetinkaya via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: kadircet
>> Date: Wed Oct 9 05:48:41 2019
>> New Revision: 374163
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=374163&view=rev
>> Log:
>> [clangd] Propagate context into reply handlers
>>
>> Modified:
>> clang-tools-extra/trunk/clangd/ClangdLSPServer.h
>>
>> Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=374163&r1=374162&r2=374163&view=diff
>>
>> ==
>> --- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original)
>> +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Wed Oct 9 05:48:41
>> 2019
>> @@ -157,7 +157,7 @@ private:
>>void call(StringRef Method, llvm::json::Value Params,
>> Callback CB) {
>> // Wrap the callback with LSP conversion and error-handling.
>> auto HandleReply =
>> -[CB = std::move(CB)](
>> +[CB = std::move(CB), Ctx = Context::current().clone()](
>> llvm::Expected RawResponse) mutable {
>>Response Rsp;
>>if (!RawResponse) {
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs
Hi Jan, I don't think there's much point in running ReplayPreamble with an empty preamble, but this should already be a no-op as there can't be any includes inside the preamble region if size is 0. I can't seem to reproduce a failure with the root causes you've provided. Even when ReplayPreamble::create doesn't take the early exit, for an empty preamble we should not have any includes, hence ReplayPreamble::replay would be a no-op. That's what I am getting while running the tests (you can check this by printing the MainFileIncludes in ReplayPreamble::create before early exit. Note that PatchesAdditionalIncludes builds two ASTs, the first one will have additional includes as it builds a full AST with a non-empty preamble, but the second AST should be built with an empty preamble and empty preamble-includes) It seems like something else is going on here, any chance you are inserting an implicit include inside your custom PP logic? If that's the case we should look for a fix in include extraction logic as it shouldn't pick up includes that are coming from implicit(more over non-main-file) sources. On Tue, Jun 2, 2020 at 8:09 AM Jan Korous via Phabricator < revi...@reviews.llvm.org> wrote: > jkorous added a comment. > > Hi @kadircet! > I am investigating a failure of `PatchesAdditionalIncludes` test that we > got internally. It's a failed assert in `ReplayPreamble::replay`. > Our clangd source code is for all practical purposes identical to upstream > version but not so with clang source. Specifically what we do is that in > `CompilerInstance::createPreprocessor` we always add one particular > callback. > This means that when in the test we are calling `buildPreamble` for > `TestTU TU` with empty buffer we never hit the early return in > `ReplayPreamble::attach()` (ParsedAST.cpp:124) like upstream version does > and proceed to create a new `ReplayPreamble` object with `PreambleBounds` > of `size() == 0` which leads to `ReplayPreamble::MainFileTokens` to be > empty and later we hit failed assert in `ReplayPreamble::replay` about > `assert(HashTok != MainFileTokens.end() && ...)`. > > Now, while I can just tweak either `ReplayPreamble::attach()` or remove > the PP callback in the test internally I am wondering whether you consider > empty preamble & PP with callbacks valid use-case of `ReplayPreamble` and > worth a fix. > > This is where we are creating the empty `MainFileTokens`: > > * frame #0: 0x000103337649 ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, > Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, > SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, > PB=0x7ffeefbfc658) + 169 at ParsedAST.cpp:142 > frame #1: 0x00010333756d ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, > Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, > SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, > PB=0x7ffeefbfc658) + 77 at ParsedAST.cpp:139 > frame #2: 0x000103334fa7 ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::attach(Includes=0x7ffeefbfc678, > Clang=0x000114f33ea0, PB=0x7ffeefbfc658) + 183 at ParsedAST.cpp:126 > frame #3: 0x000103334189 ClangdTests` > clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp", > Length = 20), Inputs=0x7ffeefbfdf98, CI=nullptr, > CompilerInvocationDiags=ArrayRef @ 0x7ffeefbfd830, > Preamble=std::__1::shared_ptr clang::clangd::PreambleData>::element_type @ 0x000114f3e728 strong=2 > weak=1) + 3897 at ParsedAST.cpp:385 > frame #4: 0x0001006f1032 ClangdTests` clang::clangd::(anonymous > namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x000114f04090) > + 1778 at ParsedASTTests.cpp:477 > > This is the failed assert: > > * frame #4: 0x000103337a0c ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::replay(this=0x000114f45da0) + 556 at > ParsedAST.cpp:182 > frame #5: 0x00010333777c ClangdTests` clang::clangd::(anonymous > namespace)::ReplayPreamble::FileChanged(this=0x000114f45da0, Loc=(ID = > 74), Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2)) + 156 at > ParsedAST.cpp:166 > frame #6: 0x000101b7900a ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x000116204080, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 90 at > PPCallbacks.h:390 > frame #7: 0x000101b79046 ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x0001162040c0, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 150 at > PPCallbacks.h:391 > frame #8: 0x000101b79046 ClangdTests` > clang::PPChainedCallbacks::FileChanged(this=0x000116204100, Loc=(ID = > 74), Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2)) + 150 at > PPCallbacks.h:391 > frame #9: 0x000101b7904
Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs
managed to reproduce the issue. sent out https://reviews.llvm.org/D80988 for a fix. On Tue, Jun 2, 2020 at 12:08 PM Sam McCall wrote: > On Tue, Jun 2, 2020 at 10:38 AM Kadir Çetinkaya > wrote: > >> Hi Jan, >> >> I don't think there's much point in running ReplayPreamble with an empty >> preamble, but this should already be a no-op as there can't be any includes >> inside the preamble region if size is 0. >> >> I can't seem to reproduce a failure with the root causes you've provided. >> Even when ReplayPreamble::create doesn't take the early exit, for an empty >> preamble we should not have any includes, hence ReplayPreamble::replay >> would be a no-op. That's what I am getting while running the tests (you can >> check this by printing the MainFileIncludes in ReplayPreamble::create >> before early exit. Note that PatchesAdditionalIncludes builds two ASTs, the >> first one will have additional includes as it builds a full AST with a >> non-empty preamble, but the second AST should be built with an empty >> preamble and empty preamble-includes) >> > Yeah, this sounds suspicious to me. > In that loop just before the failed assert, can you dump the details of > `Inc`? > Specifically `Written`, `Resolved`, and HashOffset, and ideally work out > what buffer HashOffset really indexes into. > > >> >> It seems like something else is going on here, any chance you are >> inserting an implicit include inside your custom PP logic? If that's the >> case we should look for a fix in include extraction logic as it shouldn't >> pick up includes that are coming from implicit(more over non-main-file) >> sources. >> >> On Tue, Jun 2, 2020 at 8:09 AM Jan Korous via Phabricator < >> revi...@reviews.llvm.org> wrote: >> >>> jkorous added a comment. >>> >>> Hi @kadircet! >>> I am investigating a failure of `PatchesAdditionalIncludes` test that we >>> got internally. It's a failed assert in `ReplayPreamble::replay`. >>> Our clangd source code is for all practical purposes identical to >>> upstream version but not so with clang source. Specifically what we do is >>> that in `CompilerInstance::createPreprocessor` we always add one particular >>> callback. >>> This means that when in the test we are calling `buildPreamble` for >>> `TestTU TU` with empty buffer we never hit the early return in >>> `ReplayPreamble::attach()` (ParsedAST.cpp:124) like upstream version does >>> and proceed to create a new `ReplayPreamble` object with `PreambleBounds` >>> of `size() == 0` which leads to `ReplayPreamble::MainFileTokens` to be >>> empty and later we hit failed assert in `ReplayPreamble::replay` about >>> `assert(HashTok != MainFileTokens.end() && ...)`. >>> >>> Now, while I can just tweak either `ReplayPreamble::attach()` or remove >>> the PP callback in the test internally I am wondering whether you consider >>> empty preamble & PP with callbacks valid use-case of `ReplayPreamble` and >>> worth a fix. >>> >>> This is where we are creating the empty `MainFileTokens`: >>> >>> * frame #0: 0x000103337649 ClangdTests` clang::clangd::(anonymous >>> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, >>> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, >>> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, >>> PB=0x7ffeefbfc658) + 169 at ParsedAST.cpp:142 >>> frame #1: 0x00010333756d ClangdTests` clang::clangd::(anonymous >>> namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, >>> Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, >>> SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, >>> PB=0x7ffeefbfc658) + 77 at ParsedAST.cpp:139 >>> frame #2: 0x000103334fa7 ClangdTests` clang::clangd::(anonymous >>> namespace)::ReplayPreamble::attach(Includes=0x7ffeefbfc678, >>> Clang=0x000114f33ea0, PB=0x7ffeefbfc658) + 183 at ParsedAST.cpp:126 >>> frame #3: 0x000103334189 ClangdTests` >>> clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp", >>> Length = 20), Inputs=0x7ffeefbfdf98, CI=nullptr, >>> CompilerInvocationDiags=ArrayRef @ 0x7ffeefbfd830, >>> Preamble=std::__1::shared_ptr>> clang::clangd::PreambleData>::element_type @ 0x000114f3e728 strong=2 >>> weak=1) + 3897 at ParsedAST.cpp:385 >>> frame #4: 0x0001006f1032 ClangdTests` clang::clangd::(anonymous >>> namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x000114f04090) >>> + 1778 at ParsedASTTests.cpp:477 >>> >>> This is the failed assert: >>> >>> * frame #4: 0x000103337a0c ClangdTests` clang::clangd::(anonymous >>> namespace)::ReplayPreamble::replay(this=0x000114f45da0) + 556 at >>> ParsedAST.cpp:182 >>> frame #5: 0x00010333777c ClangdTests` clang::clangd::(anonymous >>> namespace)::ReplayPreamble::FileChanged(this=0x000114f45da0, Loc=(ID = >>> 74), Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2)) + 156 at >>> ParsedAST.cpp:166 >>> frame #6: 0x000101b7900a Cl
Re: [PATCH] D80293: [clangd] Run PreambleThread in async mode behind a flag
thanks for letting me know. it is annoying that the test is flaky, but I don't see that flakiness on any other buildbot I've access to. Is there any chance that you are running in a custom build configuration that I can try to repro the failure? Also what's the falkiness ratio? On Tue, Jun 2, 2020 at 1:43 AM Wolfgang Pieb via Phabricator < revi...@reviews.llvm.org> wrote: > wolfgangp added a comment. > > Hi! > > Shortly after this patch went in, we started to see intermittent failures > with the unittest TUSchedulerTests.ManyUpdates. The build server (Windows) > we're running it on is quite powerful and our own hand-run tests only take > about 400 ms, so we don't really understand what could be happening. Any > help would be appreciated. > > Here is an excerpt from the log: > > Note: Google Test filter = TUSchedulerTests.ManyUpdates > > [==] Running 1 test from 1 test case. > > [--] Global test environment set-up. > > [--] 1 test from TUSchedulerTests > > [ RUN ] TUSchedulerTests.ManyUpdates > > C:\j\w\...\clang-tools-extra\clangd\unittests\TUSchedulerTests.cpp(508): > error: Value of: S.blockUntilIdle(timeoutSeconds(10)) > > Actual: false > > Expected: true > > [ FAILED ] TUSchedulerTests.ManyUpdates (10576 ms) > > [--] 1 test from TUSchedulerTests (10576 ms total) > > > Repository: > rG LLVM Github Monorepo > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D80293/new/ > > https://reviews.llvm.org/D80293 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D63194: [clangd] Link and initialize target infos
Hi Filipe! Thanks for letting me know, I've sent
https://reviews.llvm.org/D77944 to enable this in clangd lit config.
Regarding the failure, I can't seem to repro on machines I have access to.
The premerge-tests also seems to be succeeding, please see
https://results.llvm-merge-guard.org/BETA_amd64_debian_testing_clang8-626/console-log.txt
.
Can you provide more info about how you've ran the test?
On Fri, Apr 10, 2020 at 4:23 PM Filipe Cabecinhas wrote:
> Hi Kadir,
>
> Can you fix the target_info.test clangd test you committed in this
> revision, please?
> I see you've tried fixing it later by adding `REQUIRES:
> x86-registered-target`, but now it's never running because that feature
> isn't (ever) set.
> Here's a buildbot run showing it as unsupported (x86 target is built):
> http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/26746/steps/test-check-all/logs/stdio
>
> I saw you tried to fix the test in r364413 ([clangd] Disable failing
> unittest on non-x86 platforms) because it was always failing. The problem
> is that, even though the feature check is needed, the test still isn't
> working well. I just ran it manually and didn't get the target changed from
> my host one.
>
> Here's a diff for the feature check that you can apply to propagate the
> feature. But we still need to get the clangd code fixed so it picks up the
> target:
>
> ```
> diff --git a/clang-tools-extra/clangd/test/lit.cfg.py
> b/clang-tools-extra/clangd/test/lit.cfg.py
> index 5030ca356ef..54406498af0 100644
> --- a/clang-tools-extra/clangd/test/lit.cfg.py
> +++ b/clang-tools-extra/clangd/test/lit.cfg.py
> @@ -3,6 +3,19 @@ import lit.llvm
> lit.llvm.initialize(lit_config, config)
> lit.llvm.llvm_config.use_clang()
>
> +# required for target_info.test
> +def calculate_arch_features(arch_string):
> +features = []
> +for arch in arch_string.split():
> +features.append(arch.lower() + '-registered-target')
> +return features
> +
> +lit.llvm.llvm_config.feature_config(
> +[('--assertion-mode', {'ON': 'asserts'}),
> + ('--cxxflags', {r'-D_GLIBCXX_DEBUG\b': 'libstdcxx-safe-mode'}),
> + ('--targets-built', calculate_arch_features)
> + ])
> +
> config.name = 'Clangd'
> config.suffixes = ['.test']
> config.excludes = ['Inputs']
> ```
>
> Thank you,
>
> Filipe
>
> Filipe
>
>
> On Wed, Jun 26, 2019 at 8:48 AM Kadir Cetinkaya via Phabricator via
> cfe-commits wrote:
>
>> kadircet closed this revision.
>> kadircet added a comment.
>>
>> Landed as rL364387
>>
>>
>> Repository:
>> rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>> https://reviews.llvm.org/D63194/new/
>>
>> https://reviews.llvm.org/D63194
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs
Hi Mikael,
sent out 4f4a8ae72e95f2c7fa5e4ca56dd6b1a83a304680, please let me know if it
helps!
On Thu, Jun 4, 2020 at 12:40 PM Mikael Holmén via Phabricator <
revi...@reviews.llvm.org> wrote:
> uabelho added inline comments.
>
>
>
> Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1034
>const PreambleData &Preamble;
> - const PreamblePatch &Patch;
> + llvm::Optional Patch;
>llvm::StringRef Contents;
>
> Hi!
>
> When compiling with gcc 7.4 I see a warning that I think originates from
> this line:
>
> ```
> [4032/4668] Building CXX object
> tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o
> In file included from
> /data/repo/master/llvm/include/llvm/ADT/STLExtras.h:19:0,
> from
> /data/repo/master/llvm/include/llvm/ADT/StringRef.h:12,
> from /data/repo/master/clang-tools-extra/clangd/URI.h:12,
> from
> /data/repo/master/clang-tools-extra/clangd/Protocol.h:26,
> from
> /data/repo/master/clang-tools-extra/clangd/Headers.h:12,
> from
> /data/repo/master/clang-tools-extra/clangd/CodeComplete.h:18,
> from
> /data/repo/master/clang-tools-extra/clangd/CodeComplete.cpp:20:
> /data/repo/master/llvm/include/llvm/ADT/Optional.h: In instantiation of
> 'void llvm::optional_detail::OptionalStorage
> >::emplace(Args&& ...) [with Args = {const clang::clangd::PreamblePatch}; T
> = const clang::clangd::PreamblePatch; bool = false]':
> /data/repo/master/llvm/include/llvm/ADT/Optional.h:55:14: required from
> 'llvm::optional_detail::OptionalStorage
> >::OptionalStorage(llvm::optional_detail::OptionalStorage
> >&&) [with T = const clang::clangd::PreamblePatch; bool =
> false]'
> /data/repo/master/llvm/include/llvm/ADT/Optional.h:228:3: required from
> here
> /data/repo/master/llvm/include/llvm/ADT/Optional.h:89:12: warning: cast
> from type 'const clang::clangd::PreamblePatch*' to type 'void*' casts away
> qualifiers [-Wcast-qual]
> ::new ((void *)std::addressof(value)) T(std::forward(args)...);
> ^
> ```
>
>
>
> Repository:
> rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
> https://reviews.llvm.org/D77644/new/
>
> https://reviews.llvm.org/D77644
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang-tools-extra] dffa9df - [clangd] Shard preamble symbols in dynamic index
Thanks Mikael, 66b54d586fa73499714e2bfef3cedffeabb08f34 should fix this. On Fri, Apr 17, 2020 at 2:42 PM Mikael Holmén wrote: > Hi Kadir, > > I noticed in our buildbots that run with sanitizers that we got a new > complaint with this commit. It's still there on current top-of-tree now > so the fixes you pushed after this patch doesn't seem to address this > one: > > ==89727==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 136 byte(s) in 1 object(s) allocated from: > #0 0x5b9838 in operator new(unsigned long) > /repo/uabkaka/tmp/llvm/projects/compiler- > rt/lib/asan/asan_new_delete.cc:106 > #1 0xc50879 in make_unique clang::clangd::RefSlab> > /proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:3132:28 > #2 0xc50879 in refSlab /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../../clang-tools-extra/clangd/unittests/FileIndexTests.cpp:87 > #3 0xc50879 in clang::clangd::(anonymous > namespace)::FileShardedIndexTest_Sharding_Test::TestBody() > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all-asan/../../clang-tools- > extra/clangd/unittests/FileIndexTests.cpp:535 > #4 0x1620190 in HandleExceptionsInMethodIfSupported void> /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc > #5 0x1620190 in testing::Test::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2474 > #6 0x1623875 in testing::TestInfo::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2656:11 > #7 0x1624cf0 in testing::TestCase::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2774:28 > #8 0x1643714 in testing::internal::UnitTestImpl::RunAllTests() > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:4649:43 > #9 0x16428c0 in > HandleExceptionsInMethodIfSupported bool> /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc > #10 0x16428c0 in testing::UnitTest::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:4257 > #11 0x16048f0 in RUN_ALL_TESTS /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/include/gtest/gtest.h:2233:46 > #12 0x16048f0 in main /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/UnitTestMain/TestMain.cpp:50 > #13 0x7f558b333544 in __libc_start_main (/lib64/libc.so.6+0x22544) > > SUMMARY: AddressSanitizer: 136 byte(s) leaked in 1 allocation(s). > > /Mikael > > On Wed, 2020-04-15 at 00:15 -0700, Kadir Cetinkaya via cfe-commits > wrote: > > Author: Kadir Cetinkaya > > Date: 2020-04-15T09:10:10+02:00 > > New Revision: dffa9dfbda56820c02e357ad34c24ce8759b4d26 > > > > URL: > > > https://protect2.fireeye.com/v1/url?k=1ed901c9-4253d508-1ed94152-863d9bcb726f-37cce004cef08ce5&q=1&e=d51603fa-ef7d-4c66-99fd-b868e84ed659&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fcommit%2Fdffa9dfbda56820c02e357ad34c24ce8759b4d26 > > DIFF: > > > https://protect2.fireeye.com/v1/url?k=27955bfa-7b1f8f3b-27951b61-863d9bcb726f-307938d8468e38e8&q=1&e=d51603fa-ef7d-4c66-99fd-b868e84ed659&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fcommit%2Fdffa9dfbda56820c02e357ad34c24ce8759b4d26.diff > > > > LOG: [clangd] Shard preamble symbols in dynamic index > > > > Summary: > > This reduces memory usage by dynamic index from more than 400MB to > > 32MB > > when all files in clang-tools-extra/clangd/*.cpp are active in > > clangd. > > > > Reviewers: sammccall > > > > Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, > > cfe-commits > > > > Tags: #clang > > > > Differential Revision: https://reviews.llvm.org/D77732 > > > > Added: > > > > > > Modified: > > clang-tools-extra/clangd/index/Background.cpp > > clang-tools-extra/clangd/index/FileIndex.cpp > > clang-tools-extra/clangd/index/FileIndex.h > > clang-tools-extra/clangd/index/SymbolCollector.cpp > > clang-tools-extra/clangd/unittests/FileIndexTests.cpp > > clang-tools-extra/clangd/unittests/TestTU.cpp > > > > Removed: > > > > > > > > # > > ### > > diff --git a/clang-tools-extra/clangd/index/Background.cpp b/clang- > > tools-extra/clangd/index/Background.cpp > > index 1c26dd48093e..4c5719d0526c 100644 > > --- a/clang-tools-extra/clangd/ind
Re: [clang] 2214b90 - [clangd] Make signatureHelp work with stale preambles
Thanks Mikael, sent out an ugly fix at 89cb5d558895706e053bc3af972aa5b15aa82863 to get sanitizer build bots running. Will change the testing scheme for a proper fix. On Thu, Apr 23, 2020 at 4:53 PM Mikael Holmén wrote: > Hi Kadir, > > I start seeing some sanitizer complaints with this commit for the > following two testcases: > ./ClangdTests/PreamblePatchTest.ContainsNewIncludes > ./ClangdTests/PreamblePatchTest.IncludeParsing > > The complaints look like this: > > = > ==75126==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 369 byte(s) in 4 object(s) allocated from: > #0 0x5bbe40 in operator new(unsigned long, std::nothrow_t const&) > /repo/uabkaka/tmp/llvm/projects/compiler- > rt/lib/asan/asan_new_delete.cc:112 > #1 0x149a3e9 in > llvm::WritableMemoryBuffer::getNewUninitMemBuffer(unsigned long, > llvm::Twine const&) /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../lib/Support/MemoryBuffer.cpp:298:34 > #2 0x1498db3 in getMemBufferCopyImpl /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../lib/Support/MemoryBuffer.cpp:127:14 > #3 0x1498db3 in > llvm::MemoryBuffer::getMemBufferCopy(llvm::StringRef, llvm::Twine > const&) /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all-asan/../lib/Support/MemoryBuffer.cpp:136 > #4 0x506012e in > clang::clangd::PreamblePatch::apply(clang::CompilerInvocation&) const > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all-asan/../../clang-tools- > extra/clangd/Preamble.cpp:337:7 > #5 0xf910e6 in clang::clangd::(anonymous > namespace)::PreamblePatchTest_IncludeParsing_Test::TestBody() > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all-asan/../../clang-tools- > extra/clangd/unittests/PreambleTests.cpp:91:57 > #6 0x164e8e0 in HandleExceptionsInMethodIfSupported void> /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc > #7 0x164e8e0 in testing::Test::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2474 > #8 0x1651fc5 in testing::TestInfo::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2656:11 > #9 0x1653440 in testing::TestCase::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:2774:28 > #10 0x1671e64 in testing::internal::UnitTestImpl::RunAllTests() > /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize- > asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:4649:43 > #11 0x1671010 in > HandleExceptionsInMethodIfSupported bool> /repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master- > sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc > #12 0x1671010 in testing::UnitTest::Run() /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/src/gtest.cc:4257 > #13 0x1633040 in RUN_ALL_TESTS /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/googletest/include/gtest/gtest.h:2233:46 > #14 0x1633040 in main /repo/bbiswjenk/fem023- > eiffel003/workspace/llvm/llvm-master-sanitize-asan/llvm/build-all- > asan/../utils/unittest/UnitTestMain/TestMain.cpp:50 > #15 0x7f7dfdfe6544 in __libc_start_main (/lib64/libc.so.6+0x22544) > > SUMMARY: AddressSanitizer: 369 byte(s) leaked in 4 allocation(s). > > Regards, > Mikael > > On Tue, 2020-04-21 at 01:34 -0700, Kadir Cetinkaya via cfe-commits > wrote: > > Author: Kadir Cetinkaya > > Date: 2020-04-21T10:27:26+02:00 > > New Revision: 2214b9076f1d3a4784820c4479e2417685e5c980 > > > > URL: > > > https://github.com/llvm/llvm-project/commit/2214b9076f1d3a4784820c4479e2417685e5c980 > > DIFF: > > > https://github.com/llvm/llvm-project/commit/2214b9076f1d3a4784820c4479e2417685e5c980.diff > > > > LOG: [clangd] Make signatureHelp work with stale preambles > > > > Summary: > > This is achieved by calculating newly added includes and implicitly > > parsing them as if they were part of the main file. > > > > This also gets rid of the need for consistent preamble reads. > > > > Reviewers: sammccall > > > > Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, mgrang, > > arphaman, jfb, usaxena95, cfe-commits > > > > Tags: #clang > > > > Differential Revision: https://reviews.llvm.org/D77392 > > > > Added: > > clang-tools-extra/clangd/unittests/PreambleTests.cpp > > > > Modified: > > clang-tools-extra
Re: [clang-tools-extra] 97c407d - [clangd] Make use of URIs in FileShardedIndex
Thanks for bringing this up Jeremy, for some reason(I suppose due to the existing breakage in the bot) I haven't received any mails regarding this breakage. Sent out 2 patches, first one seemed to fix something, but wasn't enough. The bots have a really slow turnaround though, so it takes a while to verify :/ Will update this thread again once i am sure they are fixed. On Thu, Apr 30, 2020 at 2:45 PM Jeremy Morse wrote: > Hi Kadir, > > I'm seeing a couple of windows buildbots failing to build > FileIndexTests.cpp, even after the follow up in 0dedb43153e6: > > > http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/32109 > http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/15808 > > (It looks like the follow-up fixes VS2017, but VS2019 is doing > something different?) > > -- > Thanks, > Jeremy > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D79302: [clangd] Propogate context in LSPServer tests
i was trying to provide a more generic "callback" mechanism, but you are
right, it is not needed for this test.
going to keep context prop logic though, as it might be necessary later on.
SG?
On Sun, May 3, 2020 at 9:09 PM Sam McCall via Phabricator <
revi...@reviews.llvm.org> wrote:
> sammccall added inline comments.
>
>
>
> Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:164
> +WithContextValue Ctx(
> +llvm::make_scope_exit([&CallFinished] { CallFinished.notify();
> }));
> +llvm::consumeError(Client.call(MethodName, {}).take().takeError());
>
> Sorry, I didn't really put all the pieces together in my head the first
> time around.
> The context propagation seems OK, but it's too fiddly as a way to control
> sequencing - can't you just call Client.sync() and assert after that?
>
>
> Repository:
> rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
> https://reviews.llvm.org/D79302/new/
>
> https://reviews.llvm.org/D79302
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D78429: [clangd] Metric tracking through Tracer
thanks Hans! On Mon, May 4, 2020 at 12:38 PM Hans Wennborg via Phabricator < revi...@reviews.llvm.org> wrote: > hans added a comment. > > For anyone running into the same problem, this broke the build with GCC 5: > > /work/llvm.monorepo/clang-tools-extra/clangd/ClangdServer.cpp:374:75: > error: could not convert ‘(const char*)""’ from ‘const char*’ to > ‘llvm::StringLiteral’ > > trace::Metric::Distribution); > >^ > > Hopefully 3c2c7760d9eea0236c5c54e8939b3901f4208835 < > https://reviews.llvm.org/rG3c2c7760d9eea0236c5c54e8939b3901f4208835> > fixes it. > > > Repository: > rG LLVM Github Monorepo > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D78429/new/ > > https://reviews.llvm.org/D78429 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D78598: [clangd] Remove vscode plugin: now https://github.com/clangd/vscode-clangd
yes that's the case. See https://github.com/clangd/vscode-clangd/pulls?q=is%3Apr On Mon, May 11, 2020 at 9:57 AM Nathan Ridge via Phabricator < revi...@reviews.llvm.org> wrote: > nridge added a comment. > > Also, does this mean that patches to vscode-clangd should now be submitted > as a Github PR rather than via Phabricator? > > > Repository: > rG LLVM Github Monorepo > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D78598/new/ > > https://reviews.llvm.org/D78598 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve sys header mapping (PR #72122)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/72122
None
From a73b13d6fe7691a3d12840895ad56a859d5d9f9b Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Mon, 13 Nov 2023 15:52:21 +0100
Subject: [PATCH] [clangd] Improve sys header mapping
---
clang-tools-extra/clangd/index/CanonicalIncludes.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
index 9d6c09cd2ab4b71..9ba4d7a95415bc0 100644
--- a/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
+++ b/clang-tools-extra/clangd/index/CanonicalIncludes.cpp
@@ -668,6 +668,8 @@ const std::pair
IncludeMappings[] = {
{"bits/syslog-path.h", ""},
{"bits/termios.h", ""},
{"bits/types.h", ""},
+{"bits/types/siginfo_t.h", ""},
+{"bits/types/struct_itimerspec.h", ""},
{"bits/uio.h", ""},
{"bits/ustat.h", ""},
{"bits/utmp.h", ""},
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Add handling for FriendDecls (PR #72125)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/72125
Fixes https://github.com/llvm/llvm-project/issues/64382.
From 8a96c67f70f9ae77d3e1576f49cd1ff7880e6e00 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Mon, 13 Nov 2023 16:20:01 +0100
Subject: [PATCH] [include-cleaner] Add handling for FriendDecls
Fixes https://github.com/llvm/llvm-project/issues/64382.
---
clang-tools-extra/include-cleaner/lib/WalkAST.cpp| 9 +
.../include-cleaner/unittests/WalkASTTest.cpp| 5 +
2 files changed, 14 insertions(+)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index 307e0652f9ccaf8..6c4d9b7862d915b 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -11,6 +11,7 @@
#include "clang/AST/ASTFwd.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclFriend.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
@@ -243,6 +244,14 @@ class ASTWalker : public RecursiveASTVisitor {
return true;
}
+ bool VisitFriendDecl(FriendDecl *D) {
+// We already visit the TypeLoc properly, but need to special case the decl
+// case.
+if (auto *FD = D->getFriendDecl())
+ report(D->getLocation(), FD);
+return true;
+ }
+
bool VisitConceptReference(const ConceptReference *CR) {
report(CR->getConceptNameLoc(), CR->getFoundDecl());
return true;
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 9b99d5a5c32bad3..bdfc24b8edee38f 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -550,5 +550,10 @@ TEST(WalkAST, Concepts) {
// FIXME: Foo should be explicitly referenced.
testWalk("template concept Foo = true;", "void func() { ^Foo
auto x = 1; }");
}
+
+TEST(WalkAST, FriendDecl) {
+ testWalk("void $explicit^foo();", "struct Bar { friend void ^foo(); };");
+ testWalk("struct $explicit^Foo {};", "struct Bar { friend struct ^Foo; };");
+}
} // namespace
} // namespace clang::include_cleaner
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Add handling for FriendDecls (PR #72125)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/72125 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve sys header mapping (PR #72122)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/72122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Make sure exports of stdlib also works for physical files (PR #72246)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/72246
This was creating discrepancy in cases where we might have a physical
file entry (e.g. because we followed a source location from a stdlib
file) and tried to find its exporters.
From 49ea3ae3468c5d67b2b6b03339d642e25d4fa6b0 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Tue, 14 Nov 2023 13:08:52 +0100
Subject: [PATCH] [include-cleaner] Make sure exports of stdlib also works for
physical files
This was creating discrepancy in cases where we might have a physical
file entry (e.g. because we followed a source location from a stdlib
file) and tried to find its exporters.
---
.../include-cleaner/lib/Record.cpp| 26 +--
.../include-cleaner/unittests/RecordTest.cpp | 2 ++
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp
b/clang-tools-extra/include-cleaner/lib/Record.cpp
index 7a8e10a9c675496..a98129452e2bdde 100644
--- a/clang-tools-extra/include-cleaner/lib/Record.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -232,6 +232,17 @@ class PragmaIncludes::RecordPragma : public PPCallbacks,
public CommentHandler {
void checkForExport(FileID IncludingFile, int HashLine,
std::optional IncludedHeader,
OptionalFileEntryRef IncludedFile) {
+auto AddExport = [&] {
+ auto ExportingFileName = SM.getFileEntryForID(IncludingFile)->getName();
+ if (IncludedFile) {
+Out->IWYUExportBy[IncludedFile->getUniqueID()].push_back(
+ExportingFileName);
+ }
+ if (IncludedHeader && IncludedHeader->kind() == Header::Standard) {
+Out->StdIWYUExportBy[IncludedHeader->standard()].push_back(
+ExportingFileName);
+ }
+};
if (ExportStack.empty())
return;
auto &Top = ExportStack.back();
@@ -240,20 +251,7 @@ class PragmaIncludes::RecordPragma : public PPCallbacks,
public CommentHandler {
// Make sure current include is covered by the export pragma.
if ((Top.Block && HashLine > Top.SeenAtLine) ||
Top.SeenAtLine == HashLine) {
- if (IncludedHeader) {
-switch (IncludedHeader->kind()) {
-case Header::Physical:
- Out->IWYUExportBy[IncludedHeader->physical().getUniqueID()]
- .push_back(Top.Path);
- break;
-case Header::Standard:
- Out->StdIWYUExportBy[IncludedHeader->standard()].push_back(Top.Path);
- break;
-case Header::Verbatim:
- assert(false && "unexpected Verbatim header");
- break;
-}
- }
+ AddExport();
// main-file #include with export pragma should never be removed.
if (Top.SeenAtFile == SM.getMainFileID() && IncludedFile)
Out->ShouldKeep.insert(IncludedFile->getUniqueID());
diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
index 36850731d514539..dfefa66887b0f24 100644
--- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
@@ -452,6 +452,8 @@ TEST_F(PragmaIncludeTest, IWYUExportForStandardHeaders) {
auto &FM = Processed.fileManager();
EXPECT_THAT(PI.getExporters(*tooling::stdlib::Header::named(""), FM),
testing::UnorderedElementsAre(FileNamed("export.h")));
+ EXPECT_THAT(PI.getExporters(llvm::cantFail(FM.getFileRef("string")), FM),
+ testing::UnorderedElementsAre(FileNamed("export.h")));
}
TEST_F(PragmaIncludeTest, IWYUExportBlock) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Make sure exports of stdlib also works for physical files (PR #72246)
@@ -232,6 +232,17 @@ class PragmaIncludes::RecordPragma : public PPCallbacks,
public CommentHandler {
void checkForExport(FileID IncludingFile, int HashLine,
std::optional IncludedHeader,
OptionalFileEntryRef IncludedFile) {
+auto AddExport = [&] {
kadircet wrote:
this was preparation for a follow-up change, but I guess it's better to do it
there.
https://github.com/llvm/llvm-project/pull/72246
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Make sure exports of stdlib also works for physical files (PR #72246)
@@ -232,6 +232,17 @@ class PragmaIncludes::RecordPragma : public PPCallbacks,
public CommentHandler {
void checkForExport(FileID IncludingFile, int HashLine,
std::optional IncludedHeader,
OptionalFileEntryRef IncludedFile) {
+auto AddExport = [&] {
+ auto ExportingFileName = SM.getFileEntryForID(IncludingFile)->getName();
+ if (IncludedFile) {
kadircet wrote:
i'd rather use the name from file entry, and get rid of `Path` stored in Top
(in a follow-up change), as it's not needed anymore.
which later on should bring us 1-step closer to making `PragmaIncludes`
copyable, so that clangd can easily make a copy and extend the one from
Preamble build, with pragmas inside the main-file AST.
https://github.com/llvm/llvm-project/pull/72246
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Make sure exports of stdlib also works for physical files (PR #72246)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/72246 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Make sure exports of stdlib also works for physical files (PR #72246)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/72246
From 84f320a1c746eddea3171bd984613464a5872cbb Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Tue, 14 Nov 2023 13:08:52 +0100
Subject: [PATCH] [include-cleaner] Make sure exports of stdlib also works for
physical files
This was creating discrepancy in cases where we might have a physical
file entry (e.g. because we followed a source location from a stdlib
file) and tried to find its exporters.
---
.../include-cleaner/lib/Record.cpp | 18 --
.../include-cleaner/unittests/RecordTest.cpp | 2 ++
2 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp
b/clang-tools-extra/include-cleaner/lib/Record.cpp
index 7a8e10a9c675496..6e00ff93a7fe2fa 100644
--- a/clang-tools-extra/include-cleaner/lib/Record.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -240,20 +240,10 @@ class PragmaIncludes::RecordPragma : public PPCallbacks,
public CommentHandler {
// Make sure current include is covered by the export pragma.
if ((Top.Block && HashLine > Top.SeenAtLine) ||
Top.SeenAtLine == HashLine) {
- if (IncludedHeader) {
-switch (IncludedHeader->kind()) {
-case Header::Physical:
- Out->IWYUExportBy[IncludedHeader->physical().getUniqueID()]
- .push_back(Top.Path);
- break;
-case Header::Standard:
- Out->StdIWYUExportBy[IncludedHeader->standard()].push_back(Top.Path);
- break;
-case Header::Verbatim:
- assert(false && "unexpected Verbatim header");
- break;
-}
- }
+ if (IncludedFile)
+Out->IWYUExportBy[IncludedFile->getUniqueID()].push_back(Top.Path);
+ if (IncludedHeader && IncludedHeader->kind() == Header::Standard)
+Out->StdIWYUExportBy[IncludedHeader->standard()].push_back(Top.Path);
// main-file #include with export pragma should never be removed.
if (Top.SeenAtFile == SM.getMainFileID() && IncludedFile)
Out->ShouldKeep.insert(IncludedFile->getUniqueID());
diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
index 36850731d514539..dfefa66887b0f24 100644
--- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
@@ -452,6 +452,8 @@ TEST_F(PragmaIncludeTest, IWYUExportForStandardHeaders) {
auto &FM = Processed.fileManager();
EXPECT_THAT(PI.getExporters(*tooling::stdlib::Header::named(""), FM),
testing::UnorderedElementsAre(FileNamed("export.h")));
+ EXPECT_THAT(PI.getExporters(llvm::cantFail(FM.getFileRef("string")), FM),
+ testing::UnorderedElementsAre(FileNamed("export.h")));
}
TEST_F(PragmaIncludeTest, IWYUExportBlock) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Make sure exports of stdlib also works for physical files (PR #72246)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/72246 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
kadircet wrote: Thanks! I believe the idea is great, but I am not really sure if this is useful enough without handling anon namespaces properly. It's very rare that someone has a class/struct in a cc file, that isn't in an anon-namespace. Would you mind evolving the patch (and the infra) to work for method-definitions inside anon namespaces? https://github.com/llvm/llvm-project/pull/69704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
kadircet wrote: Yes, the current logic that finds insertion point wont work in presence of anon namespaces. We normally do some pseudo parsing to find a location to insert the definiton, but because these are happening inside the main-file now, you can directly make use of sourcelocations inside the AST to determine an insertion point. The other part is spelling of the function name and return type, there's a good chance they'll also do a weird thing when used for entities inside anon namespaces. https://github.com/llvm/llvm-project/pull/69704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add includes from source to non-self-contained headers (PR #72479)
kadircet wrote: hi! thanks for the interest @sr-tream but I am afraid this is likely to cause disruption in more cases than it might improve. apart from technical details like the threading concerns and reliance on certain variants that we don't really have (eg order of includes); at a high level the idea of "finding a representative source file for the header and replicating the PP state" is hard to work in general. The current approach of finding a candidate through `HeaderIncluders` will pick an arbitrary source file that transitively includes the header you're interested in. Hence you'll actually create a set of `-include XX` commands, that **might** (and probably will) recursively include the header itself. Also there's nothing detecting a failure in processing of the compilation unit, this is done without checking self-containedness (which is again hard to check) hence will definitely regress both performance and correctness for self-contained headers as well. sorry for forgetting to hit send on friday, i see that you did some more iterations on the weekend. but i think it'd be better to discuss the general approach first, to see if that makes sense. not only to ensure non-self-contained headers can be handled properly rather than adding an edge case where things look like they're working (but they're not, hence resulting in more bug reports and hard-to-maintain fixes), but also to make sure they're not regressing anything for regular code paths. https://github.com/llvm/llvm-project/pull/72479 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Add includes from source to non-self-contained headers (PR #72479)
kadircet wrote: > We can limit ourselves to only the paired source file, and cancel inclusions > if the source file does not include a non-self-contained header directly. > This will reduce the number of situations in which this hack works, but will > avoid the problems you describe deducing the paired source file is also a heuristic, which will likely go wrong at times. more importantly it'll still regress the behavior for self-contained headers, even if we ignore that, it still creates this new illusion of working when the user is lucky, and will trigger extra maintenance load (that's IMHO not justified) by already thin stretched maintainers. https://github.com/llvm/llvm-project/pull/72479 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/69704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
https://github.com/kadircet commented: thanks, LG in general just some small adjustments https://github.com/llvm/llvm-project/pull/69704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -435,14 +407,17 @@ class DefineOutline : public Tweak {
if (MD->getParent()->isTemplated())
return false;
- // The refactoring is meaningless for unnamed classes and definitions
- // within unnamed namespaces.
+ // The refactoring is meaningless for unnamed classes.
for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
if (auto *ND = llvm::dyn_cast(DC)) {
- if (ND->getDeclName().isEmpty())
+ if (ND->getDeclName().isEmpty() &&
+ (!SameFile || !llvm::dyn_cast(ND)))
return false;
}
}
+} else if (SameFile) {
kadircet wrote:
nit: might be better to move this to the top, e.g:
```
auto *MD = dyn_cast...;
if (!MD)
return !SameFile; // Can't outline free-standing functions in the same file.
// rest of the verification for methods
```
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -435,14 +407,17 @@ class DefineOutline : public Tweak {
if (MD->getParent()->isTemplated())
return false;
- // The refactoring is meaningless for unnamed classes and definitions
- // within unnamed namespaces.
+ // The refactoring is meaningless for unnamed classes.
kadircet wrote:
`... and namespaces, unless we're outlining in the same file`
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -499,17 +473,64 @@ class DefineOutline : public Tweak {
HeaderUpdates = HeaderUpdates.merge(*DelInline);
}
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
- return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
- std::move(HeaderFE->second));
+if (SameFile) {
+ tooling::Replacements &R = Effect->ApplyEdits[*CCFile].Replacements;
+ R = R.merge(HeaderUpdates);
+} else {
+ auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+ if (!HeaderFE)
+return HeaderFE.takeError();
+ Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
return std::move(*Effect);
}
+ // Returns the most natural insertion point for \p QualifiedName in \p
+ // Contents. This currently cares about only the namespace proximity, but in
+ // feature it should also try to follow ordering of declarations. For
example,
+ // if decls come in order `foo, bar, baz` then this function should return
+ // some point between foo and baz for inserting bar.
+ llvm::Expected getInsertionPoint(llvm::StringRef Contents,
+ const Selection &Sel) {
+// If the definition goes to the same file and there is a namespace,
+// we should (and, in the case of anonymous namespaces, need to)
+// put the definition into the original namespace block.
+// Within this constraint, the same considerations apply as in
+// the FIXME below.
kadircet wrote:
can you move fixme to the function comment instead?
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -499,17 +473,64 @@ class DefineOutline : public Tweak {
HeaderUpdates = HeaderUpdates.merge(*DelInline);
}
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
- return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
- std::move(HeaderFE->second));
+if (SameFile) {
+ tooling::Replacements &R = Effect->ApplyEdits[*CCFile].Replacements;
+ R = R.merge(HeaderUpdates);
+} else {
+ auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+ if (!HeaderFE)
+return HeaderFE.takeError();
+ Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
return std::move(*Effect);
}
+ // Returns the most natural insertion point for \p QualifiedName in \p
+ // Contents. This currently cares about only the namespace proximity, but in
+ // feature it should also try to follow ordering of declarations. For
example,
+ // if decls come in order `foo, bar, baz` then this function should return
+ // some point between foo and baz for inserting bar.
+ llvm::Expected getInsertionPoint(llvm::StringRef Contents,
+ const Selection &Sel) {
+// If the definition goes to the same file and there is a namespace,
+// we should (and, in the case of anonymous namespaces, need to)
+// put the definition into the original namespace block.
+// Within this constraint, the same considerations apply as in
+// the FIXME below.
+if (SameFile) {
+ if (auto *Namespace = Source->getEnclosingNamespaceContext()) {
kadircet wrote:
i think we should just fail the code action if enclosing namespace is nullptr,
something is definitely wrong with that file (and falling back to textual
matching feels suspicious).
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -349,6 +349,44 @@ TEST_F(DefineOutlineTest, ApplyTest) {
}
}
+TEST_F(DefineOutlineTest, InCppFile) {
+ FileName = "Test.cpp";
+
+ struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ // Member function with some adornments
+ // FIXME: What's with the extra spaces?
+ {
+ "namespace {\n"
kadircet wrote:
no need for all the complicated test cases, let's just validate the insertion
location here for a test case like:
foo.cc
```
namespace foo {
namespace {
struct Foo { void ba^r() {} };
}
}
```
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -19,7 +19,7 @@ TWEAK_TEST(DefineOutline);
TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
FileName = "Test.cpp";
- // Not available unless in a header file.
+ // Not available for free function unless in a header file.
kadircet wrote:
can you add some availability tests here?
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -349,6 +349,44 @@ TEST_F(DefineOutlineTest, ApplyTest) {
}
}
+TEST_F(DefineOutlineTest, InCppFile) {
+ FileName = "Test.cpp";
+
+ struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ // Member function with some adornments
+ // FIXME: What's with the extra spaces?
+ {
+ "namespace {\n"
kadircet wrote:
can you use rawstring literals instead?
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -349,6 +358,36 @@ TEST_F(DefineOutlineTest, ApplyTest) {
}
}
+TEST_F(DefineOutlineTest, InCppFile) {
+ FileName = "Test.cpp";
+
+ struct {
+llvm::StringRef Test;
+llvm::StringRef ExpectedSource;
+ } Cases[] = {
+ {
+ R"cpp(
+namespace foo {
+namespace {
+struct Foo { void ba^r() {} };
kadircet wrote:
might be worth also inserting some extra declarations in between to see how
insertion point selection works
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -499,17 +473,64 @@ class DefineOutline : public Tweak {
HeaderUpdates = HeaderUpdates.merge(*DelInline);
}
-auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-if (!HeaderFE)
- return HeaderFE.takeError();
-
-Effect->ApplyEdits.try_emplace(HeaderFE->first,
- std::move(HeaderFE->second));
+if (SameFile) {
+ tooling::Replacements &R = Effect->ApplyEdits[*CCFile].Replacements;
+ R = R.merge(HeaderUpdates);
+} else {
+ auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+ if (!HeaderFE)
+return HeaderFE.takeError();
+ Effect->ApplyEdits.try_emplace(HeaderFE->first,
+ std::move(HeaderFE->second));
+}
return std::move(*Effect);
}
+ // Returns the most natural insertion point for \p QualifiedName in \p
+ // Contents. This currently cares about only the namespace proximity, but in
+ // feature it should also try to follow ordering of declarations. For
example,
+ // if decls come in order `foo, bar, baz` then this function should return
+ // some point between foo and baz for inserting bar.
+ llvm::Expected getInsertionPoint(llvm::StringRef Contents,
+ const Selection &Sel) {
+// If the definition goes to the same file and there is a namespace,
+// we should (and, in the case of anonymous namespaces, need to)
+// put the definition into the original namespace block.
+// Within this constraint, the same considerations apply as in
+// the FIXME below.
+if (SameFile) {
+ if (auto *Namespace = Source->getEnclosingNamespaceContext()) {
kadircet wrote:
yes, `getEnclosingNamespaceContext` can return `TranslationUnitDecl`, e.g. for:
```
struct Foo { void b^ar() {} };
```
and things are getting messy in that scenario, I missed the part around getting
source locations, it'll actually be invalid in that case.
so what about changing this logic to get to enclosing decl instead (i.e.
TagDecl) and insert right after its end location?
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Allow "move function body out-of-line" in non-header files (PR #69704)
@@ -19,7 +19,7 @@ TWEAK_TEST(DefineOutline);
TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
FileName = "Test.cpp";
- // Not available unless in a header file.
+ // Not available for free function unless in a header file.
kadircet wrote:
well, available for:
```
namespace {
struct Foo { void ba^r() {} };
}
```
and
```
namespace ns {
struct Foo { void ba^r() {} };
}
```
and
```
struct Foo { void ba^r() {} };
```
unavailable for:
```
namespace ns {
struct Foo { void bar(); };
void Foo::b^ar() {}
}
```
https://github.com/llvm/llvm-project/pull/69704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reland "[clang][Sema] Use original template pattern when declaring implicit deduction guides for nested template classes" (PR #69676)
kadircet wrote:
thanks for the revert! I also encountered certain crashes bisecting back to
this patch, with a similar reproducer:
```
template
struct Module {
template
struct Baz {
template
explicit Baz(X);
};
Baz(int) -> Baz;
};
struct Bar {};
void foo() { Module::Baz x{2}; }
```
https://github.com/llvm/llvm-project/pull/69676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Track IWYU pragmas for non-preamble includes (PR #75612)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/75612
This makes PragmaIncldues copyable, and copies it from preamble when building a
new AST.
Fixes https://github.com/clangd/clangd/issues/1843
Fixes https://github.com/clangd/clangd/issues/1571
From 8801af07ad4e469f832570b5027e1522f41a6d06 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 15 Dec 2023 15:14:05 +0100
Subject: [PATCH] [clangd] Track IWYU pragmas for non-preamble includes
---
clang-tools-extra/clangd/Hover.cpp| 4 ++--
clang-tools-extra/clangd/IncludeCleaner.cpp | 4 ++--
clang-tools-extra/clangd/ParsedAST.cpp| 21 +++
clang-tools-extra/clangd/ParsedAST.h | 9
clang-tools-extra/clangd/XRefs.cpp| 2 +-
clang-tools-extra/clangd/index/FileIndex.cpp | 2 +-
.../clangd/unittests/FileIndexTests.cpp | 12 +--
.../clangd/unittests/IncludeCleanerTests.cpp | 2 ++
clang-tools-extra/clangd/unittests/TestTU.cpp | 4 ++--
.../include/clang-include-cleaner/Record.h| 2 +-
.../include-cleaner/lib/Record.cpp| 5 +++--
11 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/clang-tools-extra/clangd/Hover.cpp
b/clang-tools-extra/clangd/Hover.cpp
index 82323fe16c82b6..06b949bc4a2b55 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -1194,7 +1194,7 @@ void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo
&HI,
const SourceManager &SM = AST.getSourceManager();
llvm::SmallVector RankedProviders =
- include_cleaner::headersForSymbol(Sym, SM,
AST.getPragmaIncludes().get());
+ include_cleaner::headersForSymbol(Sym, SM, &AST.getPragmaIncludes());
if (RankedProviders.empty())
return;
@@ -1254,7 +1254,7 @@ void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI,
const Inclusion &Inc) {
llvm::DenseSet UsedSymbols;
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef Providers) {
if (Ref.RT != include_cleaner::RefType::Explicit ||
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp
b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 2f34c949349200..563a1f5d22f5b5 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -311,7 +311,7 @@ getUnused(ParsedAST &AST,
auto IncludeID = static_cast(*MFI.HeaderID);
if (ReferencedFiles.contains(IncludeID))
continue;
-if (!mayConsiderUnused(MFI, AST, AST.getPragmaIncludes().get())) {
+if (!mayConsiderUnused(MFI, AST, &AST.getPragmaIncludes())) {
dlog("{0} was not used, but is not eligible to be diagnosed as unused",
MFI.Written);
continue;
@@ -403,7 +403,7 @@ IncludeCleanerFindings
computeIncludeCleanerFindings(ParsedAST &AST) {
.getBuiltinDir();
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef Providers) {
bool Satisfied = false;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp
b/clang-tools-extra/clangd/ParsedAST.cpp
index d91ce7283ecee4..14a91797f4d2ea 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -653,6 +653,7 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
}
IncludeStructure Includes;
+ include_cleaner::PragmaIncludes PI;
// If we are using a preamble, copy existing includes.
if (Preamble) {
Includes = Preamble->Includes;
@@ -660,11 +661,15 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
// Replay the preamble includes so that clang-tidy checks can see them.
ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
Patch->modifiedBounds());
+PI = *Preamble->Pragmas;
}
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
// Otherwise we would collect the replayed includes again...
// (We can't *just* use the replayed includes, they don't have Resolved
path).
Includes.collect(*Clang);
+ // Same for pragma-includes, we're already inheriting preamble includes, so
we
+ // should only receive callbacks for non-preamble mainfile includes.
+ PI.record(*Clang);
// Copy over the macros in the preamble region of the main file, and combine
// with non-preamble macros below.
MainFileMacros Macros;
@@ -735,7 +740,7 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
ParsedAST Result(Filename, Inputs.Version, std::move(P
[clang-tools-extra] [clangd] Track IWYU pragmas for non-preamble includes (PR #75612)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/75612
From 8801af07ad4e469f832570b5027e1522f41a6d06 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 15 Dec 2023 15:14:05 +0100
Subject: [PATCH 1/2] [clangd] Track IWYU pragmas for non-preamble includes
---
clang-tools-extra/clangd/Hover.cpp| 4 ++--
clang-tools-extra/clangd/IncludeCleaner.cpp | 4 ++--
clang-tools-extra/clangd/ParsedAST.cpp| 21 +++
clang-tools-extra/clangd/ParsedAST.h | 9
clang-tools-extra/clangd/XRefs.cpp| 2 +-
clang-tools-extra/clangd/index/FileIndex.cpp | 2 +-
.../clangd/unittests/FileIndexTests.cpp | 12 +--
.../clangd/unittests/IncludeCleanerTests.cpp | 2 ++
clang-tools-extra/clangd/unittests/TestTU.cpp | 4 ++--
.../include/clang-include-cleaner/Record.h| 2 +-
.../include-cleaner/lib/Record.cpp| 5 +++--
11 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/clang-tools-extra/clangd/Hover.cpp
b/clang-tools-extra/clangd/Hover.cpp
index 82323fe16c82b6..06b949bc4a2b55 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -1194,7 +1194,7 @@ void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo
&HI,
const SourceManager &SM = AST.getSourceManager();
llvm::SmallVector RankedProviders =
- include_cleaner::headersForSymbol(Sym, SM,
AST.getPragmaIncludes().get());
+ include_cleaner::headersForSymbol(Sym, SM, &AST.getPragmaIncludes());
if (RankedProviders.empty())
return;
@@ -1254,7 +1254,7 @@ void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI,
const Inclusion &Inc) {
llvm::DenseSet UsedSymbols;
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef Providers) {
if (Ref.RT != include_cleaner::RefType::Explicit ||
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp
b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 2f34c949349200..563a1f5d22f5b5 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -311,7 +311,7 @@ getUnused(ParsedAST &AST,
auto IncludeID = static_cast(*MFI.HeaderID);
if (ReferencedFiles.contains(IncludeID))
continue;
-if (!mayConsiderUnused(MFI, AST, AST.getPragmaIncludes().get())) {
+if (!mayConsiderUnused(MFI, AST, &AST.getPragmaIncludes())) {
dlog("{0} was not used, but is not eligible to be diagnosed as unused",
MFI.Written);
continue;
@@ -403,7 +403,7 @@ IncludeCleanerFindings
computeIncludeCleanerFindings(ParsedAST &AST) {
.getBuiltinDir();
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef Providers) {
bool Satisfied = false;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp
b/clang-tools-extra/clangd/ParsedAST.cpp
index d91ce7283ecee4..14a91797f4d2ea 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -653,6 +653,7 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
}
IncludeStructure Includes;
+ include_cleaner::PragmaIncludes PI;
// If we are using a preamble, copy existing includes.
if (Preamble) {
Includes = Preamble->Includes;
@@ -660,11 +661,15 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
// Replay the preamble includes so that clang-tidy checks can see them.
ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
Patch->modifiedBounds());
+PI = *Preamble->Pragmas;
}
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
// Otherwise we would collect the replayed includes again...
// (We can't *just* use the replayed includes, they don't have Resolved
path).
Includes.collect(*Clang);
+ // Same for pragma-includes, we're already inheriting preamble includes, so
we
+ // should only receive callbacks for non-preamble mainfile includes.
+ PI.record(*Clang);
// Copy over the macros in the preamble region of the main file, and combine
// with non-preamble macros below.
MainFileMacros Macros;
@@ -735,7 +740,7 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
ParsedAST Result(Filename, Inputs.Version, std::move(Preamble),
std::move(Clang), std::move(Action), std::move(Tokens),
std::move(Macros), std::move(Marks), std::move(ParsedDecls),
- std
[clang-tools-extra] [clangd] Track IWYU pragmas for non-preamble includes (PR #75612)
@@ -113,7 +113,7 @@ class PragmaIncludes {
llvm::DenseSet ShouldKeep;
/// Owns the strings.
- llvm::BumpPtrAllocator Arena;
+ std::shared_ptr Arena;
kadircet wrote:
you're right in theory, but in practice we actually never build ASTs
concurrently from a preamble in clangd (we've a single ASTWorker peer per
preamble). Moreover mutating the same arena bit is actually never the case, but
quite subtle. We build the arena specifically in PPCallbacks and just move it
to PI at the end. Hence in practice we have 2 separate PIs, one inside
preamble, whose arena is never mutated. Another in ParsedAST, which can refer
to strings both inside its own Arena, but also to the arena "kept" alive by the
preamble. so I think this was safe for clangd, but too subtle to keep working
in the long run I guess.
storing a vector of const arenas in PI instead, and changing the `record` to
just push each arena to the end, rather than overwriting the existing Arena
(while extending all other structures).
https://github.com/llvm/llvm-project/pull/75612
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Perform self-containedness check at EOF (PR #75965)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/75965
Header gurads are not detected until we hit EOF. Make sure we postpone
any such detection until then.
From 32cfb9f9c196e665009051f83489ab09c36dd87a Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Tue, 19 Dec 2023 21:01:58 +0100
Subject: [PATCH] [clangd] Perform self-containedness check at EOF
Header gurads are not detected until we hit EOF. Make sure we postpone
any such detection until then.
---
.../clangd/index/SymbolCollector.cpp | 38 ++-
.../clangd/index/SymbolCollector.h| 9 -
.../clangd/unittests/IndexActionTests.cpp | 30 +++
3 files changed, 59 insertions(+), 18 deletions(-)
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp
b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index cf6102db8dd317..7ef4b15febad22 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -826,22 +826,8 @@ void SymbolCollector::setIncludeLocation(const Symbol &S,
SourceLocation DefLoc,
// We update providers for a symbol with each occurence, as SymbolCollector
// might run while parsing, rather than at the end of a translation unit.
// Hence we see more and more redecls over time.
- auto [It, Inserted] = SymbolProviders.try_emplace(S.ID);
- auto Headers =
+ SymbolProviders[S.ID] =
include_cleaner::headersForSymbol(Sym, SM, Opts.PragmaIncludes);
- if (Headers.empty())
-return;
-
- auto *HeadersIter = Headers.begin();
- include_cleaner::Header H = *HeadersIter;
- while (HeadersIter != Headers.end() &&
- H.kind() == include_cleaner::Header::Physical &&
- !tooling::isSelfContainedHeader(H.physical(), SM,
- PP->getHeaderSearchInfo())) {
-H = *HeadersIter;
-HeadersIter++;
- }
- It->second = H;
}
llvm::StringRef getStdHeader(const Symbol *S, const LangOptions &LangOpts) {
@@ -889,7 +875,7 @@ void SymbolCollector::finish() {
llvm::DenseMap HeaderSpelling;
// Fill in IncludeHeaders.
// We delay this until end of TU so header guards are all resolved.
- for (const auto &[SID, OptionalProvider] : SymbolProviders) {
+ for (const auto &[SID, Providers] : SymbolProviders) {
const Symbol *S = Symbols.find(SID);
if (!S)
continue;
@@ -931,9 +917,27 @@ void SymbolCollector::finish() {
continue;
}
-assert(Directives == Symbol::Include);
// For #include's, use the providers computed by the include-cleaner
// library.
+assert(Directives == Symbol::Include);
+// Ignore providers that are not self-contained, this is especially
+// important for symbols defined in the main-file. We want to prefer the
+// header, if possible.
+// TODO: Limit this to specifically ignore main file, when we're indexing a
+// non-header file?
+auto SelfContainedProvider =
+[this](llvm::ArrayRef Providers)
+-> std::optional {
+ for (const auto &H : Providers) {
+if (H.kind() != include_cleaner::Header::Physical)
+ return H;
+if (tooling::isSelfContainedHeader(H.physical(),
PP->getSourceManager(),
+ PP->getHeaderSearchInfo()))
+ return H;
+ }
+ return std::nullopt;
+};
+const auto OptionalProvider = SelfContainedProvider(Providers);
if (!OptionalProvider)
continue;
const auto &H = *OptionalProvider;
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.h
b/clang-tools-extra/clangd/index/SymbolCollector.h
index 10765020de518b..20116fca7c51e3 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.h
+++ b/clang-tools-extra/clangd/index/SymbolCollector.h
@@ -15,18 +15,25 @@
#include "index/Relation.h"
#include "index/Symbol.h"
#include "index/SymbolID.h"
+#include "index/SymbolLocation.h"
#include "index/SymbolOrigin.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Decl.h"
+#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Index/IndexDataConsumer.h"
#include "clang/Index/IndexSymbol.h"
#include "clang/Sema/CodeCompleteConsumer.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
#include
#include
#include
+#include
+#include
namespace clang {
namespace clangd {
@@ -177,7 +184,7 @@ class SymbolCollector : public index::IndexDataConsumer {
// Providers for Symbol.IncludeHeaders.
// The final spelling is calculated in finish().
- llvm::DenseMap>
+ llvm::DenseMap>
SymbolProviders;
// Files which contain ObjC symbols.
// This is finalized and used in finish().
diff --git a/clang-tools-extra/clangd/unittests/IndexActionTests.cpp
b/clang-tools-extra/clangd/unittests/IndexActionTests.cpp
index fa3d9c3212f9ca..2a9b8c
[clang-tools-extra] [clangd][RFC] Add container field to remote index Refs grpc method (PR #71605)
https://github.com/kadircet requested changes to this pull request. thanks a lot (and sorry for missing this, so far) https://github.com/llvm/llvm-project/pull/71605 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd][RFC] Add container field to remote index Refs grpc method (PR #71605)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/71605 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd][RFC] Add container field to remote index Refs grpc method (PR #71605)
@@ -189,6 +189,10 @@ llvm::Expected Marshaller::fromProtobuf(const
Ref &Message) {
return Location.takeError();
Result.Location = *Location;
Result.Kind = static_cast(Message.kind());
+ auto ContainerID = SymbolID::fromStr(Message.container());
+ if (!ContainerID)
+return ContainerID.takeError();
kadircet wrote:
this shouldn't be a failure, as an old server might provide a ref response
without a container, but the clients should still keep working.
https://github.com/llvm/llvm-project/pull/71605
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd][RFC] Add container field to remote index Refs grpc method (PR #71605)
@@ -223,6 +223,7 @@ TEST(RemoteMarshallingTest, RefSerialization) {
Location.FileURI = testPathURI(
"llvm-project/llvm/clang-tools-extra/clangd/Protocol.h", Strings);
Ref.Location = Location;
+ Ref.Container = llvm::cantFail(SymbolID::fromStr("0001"));
kadircet wrote:
this is actually not being tested properly, can you update
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/index/YAMLSerialization.cpp#L318
to serialize container to YAML ?
https://github.com/llvm/llvm-project/pull/71605
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] check for synthesized symbols when tracking include locations (PR #75128)
https://github.com/kadircet approved this pull request. thanks and sorry for leaving this out there, I was busy with other things and then OOO. https://github.com/llvm/llvm-project/pull/75128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] check for synthesized symbols when tracking include locations (PR #75128)
@@ -821,7 +821,8 @@ void SymbolCollector::setIncludeLocation(const Symbol &S, SourceLocation DefLoc, // Use the expansion location to get the #include header since this is // where the symbol is exposed. - IncludeFiles[S.ID] = SM.getDecomposedExpansionLoc(DefLoc).first; + if (FileID FID = SM.getDecomposedExpansionLoc(DefLoc).first; FID.isValid()) kadircet wrote: i still think it's wasteful to check both in both places, but now that you do a `lookup` instead of `at` below, i'll leave this to you. https://github.com/llvm/llvm-project/pull/75128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] check for synthesized symbols when tracking include locations (PR #75128)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/75128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd][RFC] Add container field to remote index Refs grpc method (PR #71605)
https://github.com/kadircet approved this pull request. thanks! https://github.com/llvm/llvm-project/pull/71605 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] don't lower severity of clang-tidy modernize-* checks to remarks (PR #75706)
kadircet wrote: hi @5chmidti and @HighCommander4! it was deliberate to cover `modernize-` checks as they're pretty similar to `-wdeprecated` in nature, and they're both explicit, user needs to annotate code or opt-in for particular clang-tidy checks. feedback we received in the past was, these kind of findings are hard to surface during builds, due to sheer amount of such findings in an existing code base. surfacing these findings on editing/review workflows OTOH is quite desirable to decrease the amount of new violations, but it can easily clutter any aggregate finding views (e.g. VSCode's problems panel). Hence we went with emitting these as `Hints` with `Remarks` tags. So that editors have some signals to prevent such cluttering, while still displaying these findings, especially near the cursor where new code usually lives. That being said, what exactly is the motivation for this patch? You mentioned `inconsistencies`, is it inside clangd or in other places that surface clang-tidy findings? https://github.com/llvm/llvm-project/pull/75706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix more bugs in isStartOfName() (PR #72336)
kadircet wrote: hi @owenca looks like this is still regressing certain patterns, e.g: ```cpp int foo BAR; ``` annotations before your modifications to `isStartOfName`: ``` AnnotatedTokens(L=0): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x216ebe0 Text='int' M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x21a3268 Text='foo' M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=130 Name=identifier L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x21a3298 Text='BAR' M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';' ``` new annotations: ``` AnnotatedTokens(L=0, P=0, T=5, C=0): M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x558b37655290 Text='int' M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=23 Name=identifier L=7 PPK=2 FakeLParens= FakeRParens=0 II=0x558b3769d410 Text='foo' M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=11 PPK=2 FakeLParens= FakeRParens=0 II=0x558b3769d440 Text='BAR' M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';' ``` as you can see `foo` is no longer recognized as `StartOfName`, resulting in drastically different penalties for breaking around the token and changing formatting for a lot of existing code. clang-format previously recognized these type of trailing attribute macros without any extra user configuration. it's quite obvious in https://github.com/llvm/llvm-project/commit/199fc973ced20016b04ba540cf63a1d4914fa513#diff-3f6f57cda9809a57c5b79e22b4181b3f3aaac7216262d0ef44108f39b0443e9bR8484, you're explicitly adding an attribute macro that wasn't explicitly told before (google style didn't have any pre-configured macros up until efeb546865c233dfa7706ee0316c676de9f69897). not sure if this was deliberate, but it's resulting in a lot of changes for our codebase and even independent of that not recognizing identifier `foo` as `StartOfName` seem clearly wrong to me. since we're close to release cut can you quickly remedy this regression that has started with efeb546865c233dfa7706ee0316c676de9f69897 and accumulated more commits on top? https://github.com/llvm/llvm-project/pull/72336 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] unexpected break after binOp '<<' (PR #69859)
kadircet wrote: @s1Sharp are there any conclusions here? our code also incidentally relied on having new lines between string literals, mostly for the sake of having a line break after string literals that end with `\n`. what do people think about restoring the functionality to break after string literals that end with `\n`? https://github.com/search?q=%2F%5C%5Cn%5C%22%5Cn%5Cs*%3C%3C%5C+%5C%22%2F+language%3AC%2B%2B+&type=code&ref=advsearch shows a lot of examples of such pattern, so i feel like having that without an option initially easy relatively easy and likely to preserve clang-format's behavior for a lot of existing code for the next release. afterwards we can discuss introducing more options again. https://github.com/llvm/llvm-project/pull/69859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Break after string literals with trailing line breaks (PR #76795)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/76795
This restores a subset of functionality that was forego in
d68826dfbd987377ef6771d40c1d984f09ee3b9e.
Streaming multiple string literals is rare enough in practice, hence
that change makes sense in general. But it seems people were
incidentally relying on this for having line breaks after string
literals that ended with `\n`.
This patch tries to restore that behavior to prevent regressions in the
upcoming LLVM release, until we can implement some configuration based
approach as proposed in https://github.com/llvm/llvm-project/pull/69859.
From b09c89ee1dc88e030040bcf30063ea16ec5a0b58 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 3 Jan 2024 09:28:35 +0100
Subject: [PATCH] [clang-format] Break after string literals with trailing line
breaks
This restores a subset of functionality that was forego in
d68826dfbd987377ef6771d40c1d984f09ee3b9e.
Streaming multiple string literals is rare enough in practice, hence
that change makes sense in general. But it seems people were
incidentally relying on this for having line breaks after string
literals that ended with `\n`.
This patch tries to restore that behavior to prevent regressions in the
upcoming LLVM release, until we can implement some configuration based
approach as proposed in https://github.com/llvm/llvm-project/pull/69859.
---
clang/lib/Format/TokenAnnotator.cpp | 8
clang/unittests/Format/TokenAnnotatorTest.cpp | 10 ++
2 files changed, 18 insertions(+)
diff --git a/clang/lib/Format/TokenAnnotator.cpp
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..27c6bb0ef6fe4f 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5151,6 +5151,14 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine
&Line,
return true;
if (Left.IsUnterminatedLiteral)
return true;
+ if (Right.is(tok::lessless) && Right.Next && Left.is(tok::string_literal) &&
+ // FIXME: Breaking after newlines seems useful in general. Turn this into
+ // an option and Recognize more cases like endl etc, and break
independent
+ // of what comes after operator lessless.
+ Right.Next->is(tok::string_literal) &&
+ Left.TokenText.ends_with("\\n\"")) {
+return true;
+ }
if (Right.is(TT_RequiresClause)) {
switch (Style.RequiresClausePosition) {
case FormatStyle::RCPS_OwnLine:
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp
b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 2cafc0438ffb46..cd3ffb611839d2 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -10,6 +10,7 @@
#include "FormatTestUtils.h"
#include "TestLexer.h"
+#include "clang/Basic/TokenKinds.h"
#include "gtest/gtest.h"
namespace clang {
@@ -2499,6 +2500,15 @@ TEST_F(TokenAnnotatorTest, BraceKind) {
EXPECT_BRACE_KIND(Tokens[6], BK_Block);
}
+TEST_F(TokenAnnotatorTest, StreamOperator) {
+ auto Tokens = annotate("\"foo\\n\" << aux << \"foo\\n\" << \"foo\";");
+ ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+ EXPECT_FALSE(Tokens[1]->MustBreakBefore);
+ EXPECT_FALSE(Tokens[3]->MustBreakBefore);
+ // Only break between string literals if the former ends with \n.
+ EXPECT_TRUE(Tokens[5]->MustBreakBefore);
+}
+
} // namespace
} // namespace format
} // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] unexpected break after binOp '<<' (PR #69859)
kadircet wrote: sent out https://github.com/llvm/llvm-project/pull/76795 for implementation https://github.com/llvm/llvm-project/pull/69859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Format] Fix isStartOfName to recognize attributes (PR #76804)
https://github.com/kadircet approved this pull request. can you also add a test to clang/unittests/Format/TokenAnnotatorTest.cpp that ensures trailing attribute-like macros receive `StartOfName` annotation to make sure we don't regress the signal in the future? https://github.com/llvm/llvm-project/pull/76804 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Tooling] Print the progress when there are multiple files to process (PR #75904)
https://github.com/kadircet approved this pull request. thanks, lgtm! https://github.com/llvm/llvm-project/pull/75904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Track IWYU pragmas for non-preamble includes (PR #75612)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/75612
From 041a3396cd62773bac985a6ca0b453b2f14635b3 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 15 Dec 2023 15:14:05 +0100
Subject: [PATCH 1/2] [clangd] Track IWYU pragmas for non-preamble includes
---
clang-tools-extra/clangd/Hover.cpp| 4 ++--
clang-tools-extra/clangd/IncludeCleaner.cpp | 4 ++--
clang-tools-extra/clangd/ParsedAST.cpp| 21 +++
clang-tools-extra/clangd/ParsedAST.h | 9
clang-tools-extra/clangd/XRefs.cpp| 2 +-
clang-tools-extra/clangd/index/FileIndex.cpp | 2 +-
.../clangd/unittests/FileIndexTests.cpp | 12 +--
.../clangd/unittests/IncludeCleanerTests.cpp | 2 ++
clang-tools-extra/clangd/unittests/TestTU.cpp | 4 ++--
.../include/clang-include-cleaner/Record.h| 2 +-
.../include-cleaner/lib/Record.cpp| 5 +++--
11 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/clang-tools-extra/clangd/Hover.cpp
b/clang-tools-extra/clangd/Hover.cpp
index 82323fe16c82b6..06b949bc4a2b55 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -1194,7 +1194,7 @@ void maybeAddSymbolProviders(ParsedAST &AST, HoverInfo
&HI,
const SourceManager &SM = AST.getSourceManager();
llvm::SmallVector RankedProviders =
- include_cleaner::headersForSymbol(Sym, SM,
AST.getPragmaIncludes().get());
+ include_cleaner::headersForSymbol(Sym, SM, &AST.getPragmaIncludes());
if (RankedProviders.empty())
return;
@@ -1254,7 +1254,7 @@ void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI,
const Inclusion &Inc) {
llvm::DenseSet UsedSymbols;
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), collectMacroReferences(AST),
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef Providers) {
if (Ref.RT != include_cleaner::RefType::Explicit ||
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp
b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 2f34c949349200..563a1f5d22f5b5 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -311,7 +311,7 @@ getUnused(ParsedAST &AST,
auto IncludeID = static_cast(*MFI.HeaderID);
if (ReferencedFiles.contains(IncludeID))
continue;
-if (!mayConsiderUnused(MFI, AST, AST.getPragmaIncludes().get())) {
+if (!mayConsiderUnused(MFI, AST, &AST.getPragmaIncludes())) {
dlog("{0} was not used, but is not eligible to be diagnosed as unused",
MFI.Written);
continue;
@@ -403,7 +403,7 @@ IncludeCleanerFindings
computeIncludeCleanerFindings(ParsedAST &AST) {
.getBuiltinDir();
include_cleaner::walkUsed(
AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
- AST.getPragmaIncludes().get(), AST.getPreprocessor(),
+ &AST.getPragmaIncludes(), AST.getPreprocessor(),
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef Providers) {
bool Satisfied = false;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp
b/clang-tools-extra/clangd/ParsedAST.cpp
index d91ce7283ecee4..14a91797f4d2ea 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -653,6 +653,7 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
}
IncludeStructure Includes;
+ include_cleaner::PragmaIncludes PI;
// If we are using a preamble, copy existing includes.
if (Preamble) {
Includes = Preamble->Includes;
@@ -660,11 +661,15 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
// Replay the preamble includes so that clang-tidy checks can see them.
ReplayPreamble::attach(Patch->preambleIncludes(), *Clang,
Patch->modifiedBounds());
+PI = *Preamble->Pragmas;
}
// Important: collectIncludeStructure is registered *after* ReplayPreamble!
// Otherwise we would collect the replayed includes again...
// (We can't *just* use the replayed includes, they don't have Resolved
path).
Includes.collect(*Clang);
+ // Same for pragma-includes, we're already inheriting preamble includes, so
we
+ // should only receive callbacks for non-preamble mainfile includes.
+ PI.record(*Clang);
// Copy over the macros in the preamble region of the main file, and combine
// with non-preamble macros below.
MainFileMacros Macros;
@@ -735,7 +740,7 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
ParsedAST Result(Filename, Inputs.Version, std::move(Preamble),
std::move(Clang), std::move(Action), std::move(Tokens),
std::move(Macros), std::move(Marks), std::move(ParsedDecls),
- std
[clang-tools-extra] [clangd] Track IWYU pragmas for non-preamble includes (PR #75612)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/75612 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Dont require confirmation for include-cleaner batch-fixes (PR #76826)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/76826
False negative/positive rate has decreased to the degree that these
extra confirmations no longer provide any value, but only create
friction in the happy case.
When we have false analysis, people usually need to apply the fixes and
run the builds to discover the failure. At that point they can add
relevant IWYU pragmas to guide analysis, and will be more likely to
create bug reports due to extra annoyance :)
From 5e9a851584dcfd730fda7d85b54101ebea89f8a3 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 3 Jan 2024 16:06:13 +0100
Subject: [PATCH] [clangd] Dont require confirmation for include-cleaner
batch-fixes
False negative/positive rate has decreased to the degree that these
extra confirmations no longer provide any value, but only create
friction in the happy case.
When we have false analysis, people usually need to apply the fixes and
run the builds to discover the failure. At that point they can add
relevant IWYU pragmas to guide analysis, and will be more likely to
create bug reports due to extra annoyance :)
---
clang-tools-extra/clangd/IncludeCleaner.cpp | 27 +---
.../test/include-cleaner-batch-fix.test | 68 ---
2 files changed, 1 insertion(+), 94 deletions(-)
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp
b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 563a1f5d22f5b5..672140a6f2b4d8 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -48,7 +48,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -237,18 +236,6 @@ removeAllUnusedIncludes(llvm::ArrayRef
UnusedIncludes) {
Diag.Fixes.front().Edits.begin(),
Diag.Fixes.front().Edits.end());
}
-
- // TODO(hokein): emit a suitable text for the label.
- ChangeAnnotation Annotation = {/*label=*/"",
- /*needsConfirmation=*/true,
- /*description=*/""};
- static const ChangeAnnotationIdentifier RemoveAllUnusedID =
- "RemoveAllUnusedIncludes";
- for (unsigned I = 0; I < RemoveAll.Edits.size(); ++I) {
-ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I);
-RemoveAll.Edits[I].annotationId = ID;
-RemoveAll.Annotations.push_back({ID, Annotation});
- }
return RemoveAll;
}
@@ -268,20 +255,8 @@ addAllMissingIncludes(llvm::ArrayRef
MissingIncludeDiags) {
Edits.try_emplace(Edit.newText, Edit);
}
}
- // FIXME(hokein): emit used symbol reference in the annotation.
- ChangeAnnotation Annotation = {/*label=*/"",
- /*needsConfirmation=*/true,
- /*description=*/""};
- static const ChangeAnnotationIdentifier AddAllMissingID =
- "AddAllMissingIncludes";
- unsigned I = 0;
- for (auto &It : Edits) {
-ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
+ for (auto &It : Edits)
AddAllMissing.Edits.push_back(std::move(It.second));
-AddAllMissing.Edits.back().annotationId = ID;
-
-AddAllMissing.Annotations.push_back({ID, Annotation});
- }
return AddAllMissing;
}
Fix fixAll(const Fix &RemoveAllUnused, const Fix &AddAllMissing) {
diff --git a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
index af7cdba5b05f43..07ebe1009a78f6 100644
--- a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
+++ b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
@@ -157,21 +157,10 @@
# CHECK-NEXT:{
# CHECK-NEXT: "arguments": [
# CHECK-NEXT:{
-# CHECK-NEXT: "changeAnnotations": {
-# CHECK-NEXT:"AddAllMissingIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT:},
-# CHECK-NEXT:"AddAllMissingIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT:}
-# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT:{
# CHECK-NEXT: "edits": [
# CHECK-NEXT:{
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
@@ -185,7 +174,6 @@
# CHECK-NEXT: }
# CHECK-NEXT:},
# CHECK-NEXT:{
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
@@ -213,29 +201,10 @@
# CHECK-NEXT:{
# CHECK-NEXT: "arguments": [
# C
[clang] [Tooling] Fix FixedCompilationDatabase with header compile flags (PR #73913)
https://github.com/kadircet approved this pull request. thanks! https://github.com/llvm/llvm-project/pull/73913 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Break after string literals with trailing line breaks (PR #76795)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/76795
From 18ef1d8901835f5129f775d292425b808f42fe85 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 3 Jan 2024 09:28:35 +0100
Subject: [PATCH 1/2] [clang-format] Break after string literals with trailing
line breaks
This restores a subset of functionality that was forego in
d68826dfbd987377ef6771d40c1d984f09ee3b9e.
Streaming multiple string literals is rare enough in practice, hence
that change makes sense in general. But it seems people were
incidentally relying on this for having line breaks after string
literals that ended with `\n`.
This patch tries to restore that behavior to prevent regressions in the
upcoming LLVM release, until we can implement some configuration based
approach as proposed in https://github.com/llvm/llvm-project/pull/69859.
---
clang/lib/Format/TokenAnnotator.cpp | 8
clang/unittests/Format/TokenAnnotatorTest.cpp | 10 ++
2 files changed, 18 insertions(+)
diff --git a/clang/lib/Format/TokenAnnotator.cpp
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..27c6bb0ef6fe4f 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5151,6 +5151,14 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine
&Line,
return true;
if (Left.IsUnterminatedLiteral)
return true;
+ if (Right.is(tok::lessless) && Right.Next && Left.is(tok::string_literal) &&
+ // FIXME: Breaking after newlines seems useful in general. Turn this into
+ // an option and Recognize more cases like endl etc, and break
independent
+ // of what comes after operator lessless.
+ Right.Next->is(tok::string_literal) &&
+ Left.TokenText.ends_with("\\n\"")) {
+return true;
+ }
if (Right.is(TT_RequiresClause)) {
switch (Style.RequiresClausePosition) {
case FormatStyle::RCPS_OwnLine:
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp
b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 2cafc0438ffb46..cd3ffb611839d2 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -10,6 +10,7 @@
#include "FormatTestUtils.h"
#include "TestLexer.h"
+#include "clang/Basic/TokenKinds.h"
#include "gtest/gtest.h"
namespace clang {
@@ -2499,6 +2500,15 @@ TEST_F(TokenAnnotatorTest, BraceKind) {
EXPECT_BRACE_KIND(Tokens[6], BK_Block);
}
+TEST_F(TokenAnnotatorTest, StreamOperator) {
+ auto Tokens = annotate("\"foo\\n\" << aux << \"foo\\n\" << \"foo\";");
+ ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+ EXPECT_FALSE(Tokens[1]->MustBreakBefore);
+ EXPECT_FALSE(Tokens[3]->MustBreakBefore);
+ // Only break between string literals if the former ends with \n.
+ EXPECT_TRUE(Tokens[5]->MustBreakBefore);
+}
+
} // namespace
} // namespace format
} // namespace clang
From ecc4284ce3d174944a09189d7ff3570df4ccbe70 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Thu, 4 Jan 2024 08:16:17 +0100
Subject: [PATCH 2/2] Add formatting test & drop extra include
---
clang/unittests/Format/FormatTest.cpp | 2 ++
clang/unittests/Format/TokenAnnotatorTest.cpp | 1 -
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/unittests/Format/FormatTest.cpp
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..25ef5c680af862 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -26708,6 +26708,8 @@ TEST_F(FormatTest, PPBranchesInBracedInit) {
TEST_F(FormatTest, StreamOutputOperator) {
verifyFormat("std::cout << \"foo\" << \"bar\" << baz;");
+ verifyFormat("std::cout << \"foo\\n\"\n"
+ " << \"bar\";");
}
TEST_F(FormatTest, BreakAdjacentStringLiterals) {
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp
b/clang/unittests/Format/TokenAnnotatorTest.cpp
index cd3ffb611839d2..decc0785c5cde7 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -10,7 +10,6 @@
#include "FormatTestUtils.h"
#include "TestLexer.h"
-#include "clang/Basic/TokenKinds.h"
#include "gtest/gtest.h"
namespace clang {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Break after string literals with trailing line breaks (PR #76795)
kadircet wrote: > Can you also add a formatting test? done https://github.com/llvm/llvm-project/pull/76795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Do not offer extraction to variable for decl init expression (PR #69477)
@@ -504,68 +502,6 @@ TEST_F(ExtractVariableTest, Test) {
int main() {
auto placeholder = []() { return 42; }(); return placeholder ;
})cpp"},
- {R"cpp(
-template
-void foo(Ts ...args) {
- auto x = [[ [&args...]() {} ]];
-}
- )cpp",
- R"cpp(
-template
-void foo(Ts ...args) {
- auto placeholder = [&args...]() {}; auto x = placeholder ;
-}
- )cpp"},
- {R"cpp(
kadircet wrote:
for the following two can you change them to:
```
...
auto f = [[ lambda_expr ]]();
...
```
that way they should turn into
```
...
placeholder = lambda_expr;
auto f = placeholder();
...
```
https://github.com/llvm/llvm-project/pull/69477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Do not offer extraction to variable for decl init expression (PR #69477)
@@ -422,8 +422,6 @@ TEST_F(ExtractVariableTest, Test) {
int member = 42;
};
)cpp"},
- {R"cpp(void f() { auto x = [[ [](){ return 42; }]]; })cpp",
kadircet wrote:
let's change this to `auto x = +[[ ... ]]`, to make sure we preserve the test
for moving around a lambda
https://github.com/llvm/llvm-project/pull/69477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Do not offer extraction to variable for decl init expression (PR #69477)
@@ -490,6 +491,13 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
BO->getRHS() == OuterImplicit.ASTNode.get())
return false;
}
+ if (const auto *Decl = Parent->ASTNode.get()) {
+if (!Decl->isInitCapture() &&
kadircet wrote:
it's not obvious to me why we check for anything but initializer being the
outerimplict node itself. can you add comments and test cases?
https://github.com/llvm/llvm-project/pull/69477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Do not offer extraction to variable for decl init expression (PR #69477)
@@ -27,10 +27,10 @@ TEST_F(ExtractVariableTest, Test) {
return t.b[[a]]r]]([[t.z]])]];
}
void f() {
- int a = [[5 +]] [[4 * xyz]]();
+ int a = 5 + [[4 * xyz]]();
// multivariable initialization
if(1)
-int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
+int x = [[1]] + 1, y = [[a + 1]], a = [[1]] + 2, z = a + 1;
kadircet wrote:
hmm, it's surprising that this is still available for `y = [[a + 1]]`
https://github.com/llvm/llvm-project/pull/69477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Do not offer extraction to variable for decl init expression (PR #69477)
@@ -504,68 +502,6 @@ TEST_F(ExtractVariableTest, Test) {
int main() {
auto placeholder = []() { return 42; }(); return placeholder ;
})cpp"},
- {R"cpp(
kadircet wrote:
again you can keep this test around by prepending a `+` to the lambda
https://github.com/llvm/llvm-project/pull/69477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Dont require confirmation for include-cleaner batch-fixes (PR #76826)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/76826
From 5e9a851584dcfd730fda7d85b54101ebea89f8a3 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 3 Jan 2024 16:06:13 +0100
Subject: [PATCH 1/2] [clangd] Dont require confirmation for include-cleaner
batch-fixes
False negative/positive rate has decreased to the degree that these
extra confirmations no longer provide any value, but only create
friction in the happy case.
When we have false analysis, people usually need to apply the fixes and
run the builds to discover the failure. At that point they can add
relevant IWYU pragmas to guide analysis, and will be more likely to
create bug reports due to extra annoyance :)
---
clang-tools-extra/clangd/IncludeCleaner.cpp | 27 +---
.../test/include-cleaner-batch-fix.test | 68 ---
2 files changed, 1 insertion(+), 94 deletions(-)
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp
b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 563a1f5d22f5b5..672140a6f2b4d8 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -48,7 +48,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -237,18 +236,6 @@ removeAllUnusedIncludes(llvm::ArrayRef
UnusedIncludes) {
Diag.Fixes.front().Edits.begin(),
Diag.Fixes.front().Edits.end());
}
-
- // TODO(hokein): emit a suitable text for the label.
- ChangeAnnotation Annotation = {/*label=*/"",
- /*needsConfirmation=*/true,
- /*description=*/""};
- static const ChangeAnnotationIdentifier RemoveAllUnusedID =
- "RemoveAllUnusedIncludes";
- for (unsigned I = 0; I < RemoveAll.Edits.size(); ++I) {
-ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I);
-RemoveAll.Edits[I].annotationId = ID;
-RemoveAll.Annotations.push_back({ID, Annotation});
- }
return RemoveAll;
}
@@ -268,20 +255,8 @@ addAllMissingIncludes(llvm::ArrayRef
MissingIncludeDiags) {
Edits.try_emplace(Edit.newText, Edit);
}
}
- // FIXME(hokein): emit used symbol reference in the annotation.
- ChangeAnnotation Annotation = {/*label=*/"",
- /*needsConfirmation=*/true,
- /*description=*/""};
- static const ChangeAnnotationIdentifier AddAllMissingID =
- "AddAllMissingIncludes";
- unsigned I = 0;
- for (auto &It : Edits) {
-ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
+ for (auto &It : Edits)
AddAllMissing.Edits.push_back(std::move(It.second));
-AddAllMissing.Edits.back().annotationId = ID;
-
-AddAllMissing.Annotations.push_back({ID, Annotation});
- }
return AddAllMissing;
}
Fix fixAll(const Fix &RemoveAllUnused, const Fix &AddAllMissing) {
diff --git a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
index af7cdba5b05f43..07ebe1009a78f6 100644
--- a/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
+++ b/clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
@@ -157,21 +157,10 @@
# CHECK-NEXT:{
# CHECK-NEXT: "arguments": [
# CHECK-NEXT:{
-# CHECK-NEXT: "changeAnnotations": {
-# CHECK-NEXT:"AddAllMissingIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT:},
-# CHECK-NEXT:"AddAllMissingIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT:}
-# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT:{
# CHECK-NEXT: "edits": [
# CHECK-NEXT:{
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
@@ -185,7 +174,6 @@
# CHECK-NEXT: }
# CHECK-NEXT:},
# CHECK-NEXT:{
-# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
@@ -213,29 +201,10 @@
# CHECK-NEXT:{
# CHECK-NEXT: "arguments": [
# CHECK-NEXT:{
-# CHECK-NEXT: "changeAnnotations": {
-# CHECK-NEXT:"AddAllMissingIncludes0": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT:},
-# CHECK-NEXT:"AddAllMissingIncludes1": {
-# CHECK-NEXT: "label": "",
-# CHECK-NEXT: "needsConfirmation": true
-# CHECK-NEXT:
[clang-tools-extra] [clangd] Dont require confirmation for include-cleaner batch-fixes (PR #76826)
@@ -237,18 +236,6 @@ removeAllUnusedIncludes(llvm::ArrayRef
UnusedIncludes) {
Diag.Fixes.front().Edits.begin(),
Diag.Fixes.front().Edits.end());
}
-
- // TODO(hokein): emit a suitable text for the label.
- ChangeAnnotation Annotation = {/*label=*/"",
kadircet wrote:
thanks!
https://github.com/llvm/llvm-project/pull/76826
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Dont require confirmation for include-cleaner batch-fixes (PR #76826)
kadircet wrote: > This change makes sense for removing all unused includes (as all these unused > includes are visible in the editor). For missing includes, they are less > obvious, but it is probably fine. The rationale for missing includes was the same, when things go wrong it's usually obscure enough that problems only raise with build systems that have visibility restrictions. Hence I am not sure if this extra step provides enough value. > (I think it would be great to have a way to view all missing includes, VSCode > has a feature to preview code action changes, but it seems > [broken](https://github.com/clangd/vscode-clangd/issues/376) with clangd.) Agreed, making alt+enter work in these cases would be useful, and last time I checked it is actually working. I tried with this patch, and it still gives people a way to preview and selectively apply these edits. So we're actually not taking away the functionality completely, but rather putting this behind an extra shortcut :) Can you check if it's still broken for you ? https://github.com/llvm/llvm-project/assets/897555/afe76b42-1ed4-4612-9d83-5499baf90119";> https://github.com/llvm/llvm-project/pull/76826 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Dont require confirmation for include-cleaner batch-fixes (PR #76826)
kadircet wrote: oh sorry, this should ofc work with include-cleaner fixes, as we actually provide these as edits, instead of commands, as we do with our usual tweaks. https://github.com/llvm/llvm-project/pull/76826 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Dont require confirmation for include-cleaner batch-fixes (PR #76826)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/76826 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Break after string literals with trailing line breaks (PR #76795)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/76795
From 18ef1d8901835f5129f775d292425b808f42fe85 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 3 Jan 2024 09:28:35 +0100
Subject: [PATCH 1/3] [clang-format] Break after string literals with trailing
line breaks
This restores a subset of functionality that was forego in
d68826dfbd987377ef6771d40c1d984f09ee3b9e.
Streaming multiple string literals is rare enough in practice, hence
that change makes sense in general. But it seems people were
incidentally relying on this for having line breaks after string
literals that ended with `\n`.
This patch tries to restore that behavior to prevent regressions in the
upcoming LLVM release, until we can implement some configuration based
approach as proposed in https://github.com/llvm/llvm-project/pull/69859.
---
clang/lib/Format/TokenAnnotator.cpp | 8
clang/unittests/Format/TokenAnnotatorTest.cpp | 10 ++
2 files changed, 18 insertions(+)
diff --git a/clang/lib/Format/TokenAnnotator.cpp
b/clang/lib/Format/TokenAnnotator.cpp
index 3ac3aa3c5e3a22..27c6bb0ef6fe4f 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5151,6 +5151,14 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine
&Line,
return true;
if (Left.IsUnterminatedLiteral)
return true;
+ if (Right.is(tok::lessless) && Right.Next && Left.is(tok::string_literal) &&
+ // FIXME: Breaking after newlines seems useful in general. Turn this into
+ // an option and Recognize more cases like endl etc, and break
independent
+ // of what comes after operator lessless.
+ Right.Next->is(tok::string_literal) &&
+ Left.TokenText.ends_with("\\n\"")) {
+return true;
+ }
if (Right.is(TT_RequiresClause)) {
switch (Style.RequiresClausePosition) {
case FormatStyle::RCPS_OwnLine:
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp
b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 2cafc0438ffb46..cd3ffb611839d2 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -10,6 +10,7 @@
#include "FormatTestUtils.h"
#include "TestLexer.h"
+#include "clang/Basic/TokenKinds.h"
#include "gtest/gtest.h"
namespace clang {
@@ -2499,6 +2500,15 @@ TEST_F(TokenAnnotatorTest, BraceKind) {
EXPECT_BRACE_KIND(Tokens[6], BK_Block);
}
+TEST_F(TokenAnnotatorTest, StreamOperator) {
+ auto Tokens = annotate("\"foo\\n\" << aux << \"foo\\n\" << \"foo\";");
+ ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+ EXPECT_FALSE(Tokens[1]->MustBreakBefore);
+ EXPECT_FALSE(Tokens[3]->MustBreakBefore);
+ // Only break between string literals if the former ends with \n.
+ EXPECT_TRUE(Tokens[5]->MustBreakBefore);
+}
+
} // namespace
} // namespace format
} // namespace clang
From ecc4284ce3d174944a09189d7ff3570df4ccbe70 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Thu, 4 Jan 2024 08:16:17 +0100
Subject: [PATCH 2/3] Add formatting test & drop extra include
---
clang/unittests/Format/FormatTest.cpp | 2 ++
clang/unittests/Format/TokenAnnotatorTest.cpp | 1 -
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/unittests/Format/FormatTest.cpp
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..25ef5c680af862 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -26708,6 +26708,8 @@ TEST_F(FormatTest, PPBranchesInBracedInit) {
TEST_F(FormatTest, StreamOutputOperator) {
verifyFormat("std::cout << \"foo\" << \"bar\" << baz;");
+ verifyFormat("std::cout << \"foo\\n\"\n"
+ " << \"bar\";");
}
TEST_F(FormatTest, BreakAdjacentStringLiterals) {
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp
b/clang/unittests/Format/TokenAnnotatorTest.cpp
index cd3ffb611839d2..decc0785c5cde7 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -10,7 +10,6 @@
#include "FormatTestUtils.h"
#include "TestLexer.h"
-#include "clang/Basic/TokenKinds.h"
#include "gtest/gtest.h"
namespace clang {
From d77d94f28c7553834c6372aee0a43946b53dbcf2 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 5 Jan 2024 08:48:35 +0100
Subject: [PATCH 3/3] Move comment around
---
clang/lib/Format/TokenAnnotator.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Format/TokenAnnotator.cpp
b/clang/lib/Format/TokenAnnotator.cpp
index 27c6bb0ef6fe4f..695eed7412d877 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5151,10 +5151,10 @@ bool TokenAnnotator::mustBreakBefore(const
AnnotatedLine &Line,
return true;
if (Left.IsUnterminatedLiteral)
return true;
+ // FIXME: Breaking after newlines seems useful in general. Turn this into an
+ // option and Recognize more cases like endl etc, and break independent of
+
[clang] [clang-format] Break after string literals with trailing line breaks (PR #76795)
kadircet wrote: ping @HazardyKnusperkeks @owenca this is blocking releases on our side, anything concerning here? https://github.com/llvm/llvm-project/pull/76795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Fix a race issue when editing multiple files. (PR #76960)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/76960 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Fix a race issue when editing multiple files. (PR #76960)
https://github.com/kadircet approved this pull request. thanks, lgtm! https://github.com/llvm/llvm-project/pull/76960 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Fix a race issue when editing multiple files. (PR #76960)
@@ -215,11 +208,15 @@ class ActionFactory : public
tooling::FrontendActionFactory {
: HeaderFilter(HeaderFilter) {}
std::unique_ptr create() override {
-return std::make_unique(HeaderFilter);
+return std::make_unique(HeaderFilter, EditFiles);
}
+ const llvm::StringMap &getEditFiles() const { return EditFiles;
}
kadircet wrote:
s/getEditFiles/editedFiles
https://github.com/llvm/llvm-project/pull/76960
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/76466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
https://github.com/kadircet commented: thanks, this LG in direction. only reviewed the pieces leading to in-file rename so far. https://github.com/llvm/llvm-project/pull/76466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -1187,6 +1187,16 @@ bool fromJSON(const llvm::json::Value &Params,
RenameParams &R,
O.map("position", R.position) && O.map("newName", R.newName);
}
+llvm::json::Value toJSON(const PrepareRenameResult &PRR) {
+ if (!PRR.placeholder) {
kadircet wrote:
nit: braces
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -1436,6 +1436,14 @@ struct RenameParams {
};
bool fromJSON(const llvm::json::Value &, RenameParams &, llvm::json::Path);
+struct PrepareRenameResult {
+ /// Range of the string to rename.
+ Range range;
+ /// Placeholder text to use in the editor, if set.
+ std::optional placeholder;
kadircet wrote:
let's drop `optional` to prevent confusion here, there isn't any difference
between an empty placeholder and nullopt, right?
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -53,13 +55,38 @@ struct RenameInputs {
struct RenameResult {
// The range of the symbol that the user can attempt to rename.
Range Target;
+ // Placeholder text for the rename operation, if set.
+ std::optional Placeholder;
// Rename occurrences for the current main file.
std::vector LocalChanges;
// Complete edits for the rename, including LocalChanges.
// If the full set of changes is unknown, this field is empty.
FileEdits GlobalChanges;
};
+/// Represents a symbol range where the symbol can potentially have multiple
+/// tokens.
+struct SymbolRange {
+ /// All ranges. If multiple, corresponds to an ObjC selector.
kadircet wrote:
maybe also talk about the general case?
```
Ranges for the tokens that make up the symbol's name.
Usually a single range, there can be multiple ranges if the tokens for symbol
are split, e.g. ObjC selectors.
```
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -183,6 +185,8 @@ bool isSpelled(SourceLocation Loc, const NamedDecl &ND) {
if (clang::Lexer::getRawToken(Loc, Tok, SM, LO))
return false;
auto StrName = Name.getAsString();
+ if (const auto *MD = dyn_cast(&ND))
+StrName = MD->getSelector().getNameForSlot(0).str();
kadircet wrote:
nit: we can rewrite as:
```
auto TokSpelling = clang::Lexer::getSpelling(Tok, SM, LO);
if (const auto *MD = dyn_cast(&ND))
return TokSpelling == MD->getSelector().getNameForSlot(0);
return TokSpelling == Name.getAsString();
```
(to prevent a string construction)
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -569,8 +571,43 @@ renameWithinFile(ParsedAST &AST, const NamedDecl
&RenameDecl,
// }
if (!isInsideMainFile(RenameLoc, SM))
continue;
+Locs.push_back(RenameLoc);
+ }
+ if (const auto *MD = dyn_cast(&RenameDecl)) {
kadircet wrote:
let's extract this branch into a separate function?
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -134,7 +134,7 @@ class ClangdLSPServer : private ClangdServer::Callbacks, void onWorkspaceSymbol(const WorkspaceSymbolParams &, Callback>); void onPrepareRename(const TextDocumentPositionParams &, - Callback>); + Callback>); kadircet wrote: you can drop the optional now, we either return an Error or PrepareRenameResult https://github.com/llvm/llvm-project/pull/76466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -508,7 +508,8 @@ static bool mayBeValidIdentifier(llvm::StringRef Ident) {
!isAsciiIdentifierStart(Ident.front(), AllowDollar))
return false;
for (char C : Ident) {
-if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar))
+if (llvm::isASCII(C) && !isAsciiIdentifierContinue(C, AllowDollar) &&
kadircet wrote:
nit:
```cpp
if (AllowColon && C == ':')
continue;
if (llvm::isASCII ...)
```
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -338,6 +338,10 @@ inline bool isReservedName(llvm::StringRef Name) {
SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
const SourceManager &SM);
+clangd::Range tokenRangeForLoc(SourceLocation TokLoc,
kadircet wrote:
this isn't used outside rename.cpp, let's move it there?
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -53,13 +55,38 @@ struct RenameInputs {
struct RenameResult {
// The range of the symbol that the user can attempt to rename.
Range Target;
+ // Placeholder text for the rename operation, if set.
+ std::optional Placeholder;
kadircet wrote:
same here, let's drop optional
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -71,8 +98,8 @@ llvm::Expected rename(const RenameInputs &RInputs); /// REQUIRED: Occurrences is sorted and doesn't have duplicated ranges. llvm::Expected buildRenameEdit(llvm::StringRef AbsFilePath, llvm::StringRef InitialCode, - std::vector Occurrences, - llvm::StringRef NewName); + std::vector Occurrences, + llvm::SmallVectorImpl &NewNames); kadircet wrote: why not `llvm::ArrayRef NewNames` ? https://github.com/llvm/llvm-project/pull/76466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -569,8 +571,43 @@ renameWithinFile(ParsedAST &AST, const NamedDecl
&RenameDecl,
// }
if (!isInsideMainFile(RenameLoc, SM))
continue;
+Locs.push_back(RenameLoc);
+ }
+ if (const auto *MD = dyn_cast(&RenameDecl)) {
+auto Code = SM.getBufferData(SM.getMainFileID());
+auto RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
+llvm::SmallVector NewNames;
+NewName.split(NewNames, ":");
+if (NewNames.empty())
+ NewNames.push_back(NewName);
+std::vector Ranges;
+const auto &LangOpts = RenameDecl.getASTContext().getLangOpts();
+for (const auto &Loc : Locs)
+ Ranges.push_back(tokenRangeForLoc(Loc, SM, LangOpts));
+auto FilePath = AST.tuPath();
+auto RenameRanges =
+adjustRenameRanges(Code, RenameIdentifier,
kadircet wrote:
we don't need to adjust ranges again in here. we're already using ranges from
the AST (not from index)
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -893,22 +964,36 @@ llvm::Expected buildRenameEdit(llvm::StringRef
AbsFilePath,
return LastOffset;
};
- std::vector> OccurrencesOffsets;
- for (const auto &R : Occurrences) {
-auto StartOffset = Offset(R.start);
-if (!StartOffset)
- return StartOffset.takeError();
-auto EndOffset = Offset(R.end);
-if (!EndOffset)
- return EndOffset.takeError();
-OccurrencesOffsets.push_back({*StartOffset, *EndOffset});
+ struct OccurrenceOffset {
+size_t Start;
+size_t End;
+llvm::StringRef NewName;
+
+OccurrenceOffset(size_t Start, size_t End, llvm::StringRef NewName) :
+ Start(Start), End(End), NewName(NewName) {}
+ };
+
+ std::vector OccurrencesOffsets;
+ for (const auto &SR : Occurrences) {
+for (auto It = SR.Ranges.begin(); It != SR.Ranges.end(); ++It) {
kadircet wrote:
nit: `for (auto [Range, NewName] : llvm::zip(SR.Ranges, NewNames))`
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -53,13 +55,38 @@ struct RenameInputs {
struct RenameResult {
// The range of the symbol that the user can attempt to rename.
Range Target;
+ // Placeholder text for the rename operation, if set.
+ std::optional Placeholder;
// Rename occurrences for the current main file.
std::vector LocalChanges;
// Complete edits for the rename, including LocalChanges.
// If the full set of changes is unknown, this field is empty.
FileEdits GlobalChanges;
};
+/// Represents a symbol range where the symbol can potentially have multiple
+/// tokens.
+struct SymbolRange {
+ /// All ranges. If multiple, corresponds to an ObjC selector.
+ std::vector Ranges;
+
+ /// Returns the first range.
+ Range range() const { return Ranges.front(); }
+
+ SymbolRange(Range R) : Ranges({R}) {}
+ SymbolRange(std::vector Ranges) : Ranges(std::move(Ranges)) {}
+
+ friend bool operator==(const SymbolRange &LHS, const SymbolRange &RHS) {
+return LHS.Ranges == RHS.Ranges;
+ }
+ friend bool operator!=(const SymbolRange &LHS, const SymbolRange &RHS) {
+return !(LHS == RHS);
+ }
+ friend bool operator<(const SymbolRange &LHS, const SymbolRange &RHS) {
+return LHS.range() < RHS.range();
+ }
+};
kadircet wrote:
can you move function definitions to implementation file?
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] Add support for renaming objc methods, even those with multiple selector pieces (PR #76466)
@@ -569,8 +571,43 @@ renameWithinFile(ParsedAST &AST, const NamedDecl
&RenameDecl,
// }
if (!isInsideMainFile(RenameLoc, SM))
continue;
+Locs.push_back(RenameLoc);
+ }
+ if (const auto *MD = dyn_cast(&RenameDecl)) {
+auto Code = SM.getBufferData(SM.getMainFileID());
+auto RenameIdentifier = MD->getSelector().getNameForSlot(0).str();
+llvm::SmallVector NewNames;
+NewName.split(NewNames, ":");
+if (NewNames.empty())
+ NewNames.push_back(NewName);
+std::vector Ranges;
+const auto &LangOpts = RenameDecl.getASTContext().getLangOpts();
+for (const auto &Loc : Locs)
+ Ranges.push_back(tokenRangeForLoc(Loc, SM, LangOpts));
kadircet wrote:
instead of re-lexing, you can use tokenbuffer inside AST and call
`spelledTokenAt`, then convert it into a range
https://github.com/llvm/llvm-project/pull/76466
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
