[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
https://github.com/kadircet approved this pull request. thanks for bearing with me! https://github.com/llvm/llvm-project/pull/111723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
@@ -54,7 +54,9 @@ std::unique_ptr HeaderMap::Create(FileEntryRef FE,
FileManager &FM) {
unsigned FileSize = FE.getSize();
if (FileSize <= sizeof(HMapHeader)) return nullptr;
- auto FileBuffer = FM.getBufferForFile(FE);
+ auto FileBuffer =
+ FM.getBufferForFile(FE, /*IsVolatile=*/false,
+ /*RequiresNullTerminator=*/true, /*IsText=*/false);
kadircet wrote:
i think it'd be great to have a test case for this that actually fails (even if
only in zOS).
https://github.com/llvm/llvm-project/pull/111723
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
@@ -302,6 +305,17 @@ class RealFileSystem : public FileSystem {
return Storage;
}
+ ErrorOr> openFileForRead(const Twine &Name,
kadircet wrote:
it was quite confusing to see this as an overload, can you call it
`openFileForReadWithFlags` ?
https://github.com/llvm/llvm-project/pull/111723
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
@@ -292,21 +292,21 @@ class FileManager : public RefCountedBase {
/// MemoryBuffer if successful, otherwise returning null.
llvm::ErrorOr>
getBufferForFile(FileEntryRef Entry, bool isVolatile = false,
- bool RequiresNullTerminator = true,
+ bool RequiresNullTerminator = true, bool IsText = true,
kadircet wrote:
can you add some comments explaining what `IsText` is about and when it should
be set to `false`?
also can you make it the last parameter ?
https://github.com/llvm/llvm-project/pull/111723
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
https://github.com/kadircet requested changes to this pull request. thanks! I think this LG at a high-level, added some comments about the details. I think it'd be worthwhile to add some tests (even if not now, probably going forward). as otherwise such rare use cases might regress quite frequently. https://github.com/llvm/llvm-project/pull/111723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
@@ -271,15 +271,24 @@ class FileSystem : public llvm::ThreadSafeRefCountedBase, /// Get the status of the entry at \p Path, if one exists. virtual llvm::ErrorOr status(const Twine &Path) = 0; - /// Get a \p File object for the file at \p Path, if one exists. + /// Get a \p File object for the text file at \p Path, if one exists. virtual llvm::ErrorOr> openFileForRead(const Twine &Path) = 0; + /// Get a \p File objct for the binary file at \p Path, if one exists. + /// This function should be called instead of openFileForRead if the file + /// should be opened as a binary file. kadircet wrote: can you rather explain the motiviating use case? e.g. "Some filesystems can perform modifications to bytes on-disk when reading as test. Reading as binary ensures contents are received as-is in such cases, on most file systems this is same as `openFileForRead`". https://github.com/llvm/llvm-project/pull/111723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
@@ -271,15 +271,24 @@ class FileSystem : public
llvm::ThreadSafeRefCountedBase,
/// Get the status of the entry at \p Path, if one exists.
virtual llvm::ErrorOr status(const Twine &Path) = 0;
- /// Get a \p File object for the file at \p Path, if one exists.
+ /// Get a \p File object for the text file at \p Path, if one exists.
virtual llvm::ErrorOr>
openFileForRead(const Twine &Path) = 0;
+ /// Get a \p File objct for the binary file at \p Path, if one exists.
+ /// This function should be called instead of openFileForRead if the file
+ /// should be opened as a binary file.
+ virtual llvm::ErrorOr>
+ openFileForReadBinary(const Twine &Path) {
+return openFileForRead(Path);
+ }
+
/// This is a convenience method that opens a file, gets its content and then
/// closes the file.
llvm::ErrorOr>
getBufferForFile(const Twine &Name, int64_t FileSize = -1,
- bool RequiresNullTerminator = true, bool IsVolatile =
false);
+ bool RequiresNullTerminator = true, bool IsVolatile = false,
+ bool IsText = true);
kadircet wrote:
can you also add some comments about what `IsText` parameter does?
https://github.com/llvm/llvm-project/pull/111723
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile (PR #111723)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/111723 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-include-cleaner] Fix incorrect directory issue for writing files (PR #111375)
https://github.com/kadircet commented: thanks a lot for the revision! One concern is, we can't really use `getAllCompileCommands()` reliably either, not all compilation databases implement that, and even if they do this is gonna be an overkill. For example for chromium we'll iterate over tens of thousands of compile flags, just to modify a handful of files. I've made some suggestions in line. Also I think it's easier if we put absolute-paths from CDB into the `EditedFiles` and just remap them to absolute paths per tools working directory afterwards when we can. https://github.com/llvm/llvm-project/pull/111375 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-include-cleaner] Fix incorrect directory issue for writing files (PR #111375)
@@ -173,6 +173,7 @@ class Action : public clang::ASTFrontendAction {
if (!HTMLReportPath.empty())
writeHTML();
+// Source File's path relative to compilation database.
llvm::StringRef Path =
kadircet wrote:
can we keep reporting with abs-paths here, i.e:
```
llvm::SmallString<256>
AbsPath(SM.getFileEntryRefForID(SM.getMainFileID())->getName());
SM.getFileManager().makeAbsolutePath(AbsPath);
...
```
This way we can recognize these paths in main and perform the mapping
accordingly.
https://github.com/llvm/llvm-project/pull/111375
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-include-cleaner] Fix incorrect directory issue for writing files (PR #111375)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/111375 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-include-cleaner] Fix incorrect directory issue for writing files (PR #111375)
@@ -305,7 +342,32 @@ int main(int argc, const char **argv) {
}
}
- clang::tooling::ClangTool Tool(OptionsParser->getCompilations(),
+ auto &CompilationDatabase = OptionsParser->getCompilations();
kadircet wrote:
i think this can be simplified with something like:
```
auto VFS = llvm::vfs::getRealFileSystem();
auto &CDB = OptionsParser->getCompilations();
std::map CDBToAbsPaths;
for (auto &Source : OptionsParser->getSourcePathList()) {
llvm::SmallString<256> AbsPath(Source);
if (auto Err = VFS.makeAbsolute(AbsPath)) {
llvm::errs() << "Failed to get absolute path for " << Source << " : " <<
Err.message() << '\n';
return 1;
}
for(auto Cmd : CDB.getCompilecommands(AbsPath)) {
llvm::SmallString<256> CDBPath(Cmd.Filename);
llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath);
CDBToAbsPaths[CDBPath] = AbsPath;
}
}
if (Edit) {
for (const auto &NameAndContent : Factory.editedFiles()) {
llvm::StringRef FileName = NameAndContent.first();
if (auto It = CDBToAbsPaths.find(FileName); It != CDBToAbsPaths.end())
FileName = It->second;
...
}
}
```
https://github.com/llvm/llvm-project/pull/111375
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-include-cleaner] Fix incorrect directory issue for writing files (PR #111375)
kadircet wrote: > so clang-include-cleaner correctly adjusted the path to > /home/leebc/.cache/bazel/_bazel_leebc/5ea2deb937e75f0bf29ba4c49931e67d/execroot/_main/api/main.cc, > which was correctly symlinked. in our case this is symlinked to an immutable copy of the source file (to make sure builds don't act weird if you edit some sources while the build is running). hence it'll be a regression. What's the concern about not performing this directly as relative to the process' working directory? That approach doesn't rely on any assumptions about the build system, or filesystem layout hence feels much more robust. https://github.com/llvm/llvm-project/pull/111375 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-include-cleaner] Fix incorrect directory issue for writing files (PR #111375)
https://github.com/kadircet requested changes to this pull request. thanks a lot for the patch, and sorry for regressing this. I think this is also going to regress behavior in some cases (e.g. working directory in compile commands might be virtual for some build systems like bazel). When include-cleaner is invoked as `clang-include-cleaner foo.cc` the user is definitely trying to run this on a file relative to directory in which we invoked include-cleaner, hence we should make sure it's adressed through that path going forward. Can we instead update logic near the following in `main`: ```cpp clang::tooling::ClangTool Tool(OptionsParser->getCompilations(), OptionsParser->getSourcePathList()); ``` instead of passing `OptionsParser->getSourcePathList())`, we can turn all of these source paths into absolue paths, using the current-working-dir of the process, and then we don't need to worry about how they make their way into our ast-consumer. https://github.com/llvm/llvm-project/pull/111375 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] Propagate IsText parameter to openFileForRead function (PR #110661)
kadircet wrote: Sorry for not responding here for a while. > These change mirror interface to getFileOrSTDIN() which has a IsText > parameter. This does touch a number of places but I feel the changes are in > line with the rest of the file I/O functions in llvm. I think there's a huge difference between `getFileOrSTDIN` and `llvm::vfs::FileSystem` interfaces. The former has a single implementation that's meant to always work with a physical filesytem. The latter has many implementations and is meant to decouple physical filesystem, we're now leaking that abstraction solely because physical system is trying to receive some information. > Thanks, I've tested the #embed lit tests with this fix and did not see any > regressions I am still wary of this. Logic that handles #embed directives lives in https://github.com/llvm/llvm-project/blob/main/clang/lib/Lex/PPDirectives.cpp#L3964-L3990. As you can see, all of them use the default filemanager/filesystem interfaces, which will read this file with `IsText = true` after this patch. --- All of that being said; If you folks are in alignment with this being required, I think we should still make it as least intrusive to rest of the users of this interface as possible. Looks like this only has affects on `RealFileSystem` implementation (and probably isn't really useful for the rest anyways). Can we at least change the implementation here to: - Introduce a new virtual methods called `openFileForReadBinary` and `getBufferForFileBinary` into `llvm::vfs::Filesystem`. Make default implementations just delegate to `openFileForRead` and `getBufferForFile`. Explain when the binary specific overloads should be preferred and how the default versions assume operating on text files in the comments for these methods. - Override these new methods in `RealFileSystem` to pass different flags to underlying calls - Make sure place like #embed handling and astreader calls the `read-binary` versions of these methods. https://github.com/llvm/llvm-project/pull/110661 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] Propagate IsText parameter to openFileForRead function (PR #110661)
https://github.com/kadircet requested changes to this pull request. sorry this is same as https://github.com/llvm/llvm-project/pull/107906 (with a bigger impact radius, as you're also changing getBufferForFile) and doesn't address any of the issues mention about explaining the semantics of `IsText` or justification for changing the core VFS interfaces, for an operation that's can solely be performed on physical fileystem. @perry-ca raised some concerns in https://github.com/llvm/llvm-project/pull/109664 about this functionality requiring some context awareness, I don't think any of those is addressed by this patch either. Pretty much all of the callers apart from ASTReader is just using `IsText = true`. https://github.com/llvm/llvm-project/pull/110661 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve filtering logic for undesired proto symbols (PR #110091)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/110091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve filtering logic for undesired proto symbols (PR #110091)
@@ -75,18 +76,61 @@ bool isPrivateProtoDecl(const NamedDecl &ND) {
if (ND.getIdentifier() == nullptr)
return false;
auto Name = ND.getIdentifier()->getName();
- if (!Name.contains('_'))
-return false;
- // Nested proto entities (e.g. Message::Nested) have top-level decls
- // that shouldn't be used (Message_Nested). Ignore them completely.
- // The nested entities are dangling type aliases, we may want to reconsider
- // including them in the future.
- // For enum constants, SOME_ENUM_CONSTANT is not private and should be
- // indexed. Outer_INNER is private. This heuristic relies on naming style, it
- // will include OUTER_INNER and exclude some_enum_constant.
- // FIXME: the heuristic relies on naming style (i.e. no underscore in
- // user-defined names) and can be improved.
- return (ND.getKind() != Decl::EnumConstant) || llvm::any_of(Name, islower);
+ // There are some internal helpers like _internal_set_foo();
kadircet wrote:
I feel like that FIXME might still apply. I think it's rather about passing in
a custom filter, similar to `FileFilter`, so I am more inclined towards keeping
it
https://github.com/llvm/llvm-project/pull/110091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve filtering logic for undesired proto symbols (PR #110091)
@@ -75,18 +76,61 @@ bool isPrivateProtoDecl(const NamedDecl &ND) {
if (ND.getIdentifier() == nullptr)
return false;
auto Name = ND.getIdentifier()->getName();
- if (!Name.contains('_'))
-return false;
- // Nested proto entities (e.g. Message::Nested) have top-level decls
- // that shouldn't be used (Message_Nested). Ignore them completely.
- // The nested entities are dangling type aliases, we may want to reconsider
- // including them in the future.
- // For enum constants, SOME_ENUM_CONSTANT is not private and should be
- // indexed. Outer_INNER is private. This heuristic relies on naming style, it
- // will include OUTER_INNER and exclude some_enum_constant.
- // FIXME: the heuristic relies on naming style (i.e. no underscore in
- // user-defined names) and can be improved.
- return (ND.getKind() != Decl::EnumConstant) || llvm::any_of(Name, islower);
+ // There are some internal helpers like _internal_set_foo();
+ if (Name.contains("_internal_"))
+return true;
+
+ // https://protobuf.dev/reference/cpp/cpp-generated/#nested-types
+ // Nested entities (messages/enums) has two names, one at the top-level
scope,
+ // with a mangled name created by prepending all the outer types. These names
+ // are almost never preferred by the developers, so exclude them from index.
+ // e.g.
+ // message Foo {
+ // message Bar {}
+ // enum E { A }
+ // }
+ // yields:
+ // class Foo_Bar {};
+ // enum Foo_E { Foo_E_A };
+ // class Foo {
+ // using Bar = Foo_Bar;
+ // static constexpr Foo_E A = Foo_E_A;
+ // };
+
+ // We get rid of Foo_Bar and Foo_E by discarding any top-level entries with
+ // `_` in the name. This relies on original message/enum not having `_` in
the
+ // name. Hence might go wrong in certain cases.
+ if (ND.getDeclContext()->isNamespace()) {
+// Strip off some known public suffix helpers for enums, rest of the
helpers
+// are generated inside record decls so we don't care.
+// https://protobuf.dev/reference/cpp/cpp-generated/#enum
+Name.consume_back("_descriptor");
+Name.consume_back("_IsValid");
+Name.consume_back("_Name");
+Name.consume_back("_Parse");
+Name.consume_back("_MIN");
+Name.consume_back("_MAX");
+Name.consume_back("_ARRAYSIZE");
+return Name.contains('_');
+ }
+
+ // EnumConstantDecls need some special attention, despite being nested in a
+ // TagDecl, they might still have mangled names. We filter those by checking
+ // if it has parent's name as a prefix.
+ // This might go wrong if a nested entity has a name that starts with
parent's
+ // name, e.g: enum Foo { Foo_X }.
+ if (llvm::isa(&ND)) {
+auto *DC = llvm::cast(ND.getDeclContext());
+if (!DC || !DC->getIdentifier())
+ return false;
+auto CtxName = DC->getIdentifier()->getName();
+return !CtxName.empty() && Name.consume_front(CtxName) &&
+ Name.consume_front("_");
kadircet wrote:
i'd rather keep it as-is, since `CtxName + "_"` might trigger an extra string
allocation
https://github.com/llvm/llvm-project/pull/110091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve filtering logic for undesired proto symbols (PR #110091)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/110091
From 02e1677ed40ef23792943d9b2cb895ba80312ee9 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Thu, 26 Sep 2024 10:55:49 +0200
Subject: [PATCH] [clangd] Improve filtering logic for undesired proto symbols
This used to filter any names with `_` in them, apart from
enum-constants. Resulting in discrepancies in behavior when we had
fields that have `_` in the name, or for accessors like `set_`, `has_`.
The logic seems to be trying to filter mangled names for nested entries,
so adjusted logic to only do so for top-level decls, while still
preserving some public top-level helpers.
Heuristics are still leaning towards false-negatives, e.g. if a
top-level entity has `_` in its name (`message Foo_Bar {}`), it'll be
filtered, or an enum that prefixes its type name to constants
(`enum Foo { Foo_OK }`).
---
.../clangd/index/SymbolCollector.cpp | 69 +++
.../clangd/unittests/SymbolCollectorTests.cpp | 64 ++---
2 files changed, 111 insertions(+), 22 deletions(-)
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp
b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index a76894cf0855f3..d1d744a21cfd55 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -41,6 +41,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
@@ -75,18 +76,62 @@ bool isPrivateProtoDecl(const NamedDecl &ND) {
if (ND.getIdentifier() == nullptr)
return false;
auto Name = ND.getIdentifier()->getName();
- if (!Name.contains('_'))
-return false;
- // Nested proto entities (e.g. Message::Nested) have top-level decls
- // that shouldn't be used (Message_Nested). Ignore them completely.
- // The nested entities are dangling type aliases, we may want to reconsider
- // including them in the future.
- // For enum constants, SOME_ENUM_CONSTANT is not private and should be
- // indexed. Outer_INNER is private. This heuristic relies on naming style, it
- // will include OUTER_INNER and exclude some_enum_constant.
- // FIXME: the heuristic relies on naming style (i.e. no underscore in
- // user-defined names) and can be improved.
- return (ND.getKind() != Decl::EnumConstant) || llvm::any_of(Name, islower);
+ // There are some internal helpers like _internal_set_foo();
+ if (Name.contains("_internal_"))
+return true;
+
+ // https://protobuf.dev/reference/cpp/cpp-generated/#nested-types
+ // Nested entities (messages/enums) has two names, one at the top-level
scope,
+ // with a mangled name created by prepending all the outer types. These names
+ // are almost never preferred by the developers, so exclude them from index.
+ // e.g.
+ // message Foo {
+ //message Bar {}
+ //enum E { A }
+ // }
+ //
+ // yields:
+ // class Foo_Bar {};
+ // enum Foo_E { Foo_E_A };
+ // class Foo {
+ //using Bar = Foo_Bar;
+ //static constexpr Foo_E A = Foo_E_A;
+ // };
+
+ // We get rid of Foo_Bar and Foo_E by discarding any top-level entries with
+ // `_` in the name. This relies on original message/enum not having `_` in
the
+ // name. Hence might go wrong in certain cases.
+ if (ND.getDeclContext()->isNamespace()) {
+// Strip off some known public suffix helpers for enums, rest of the
helpers
+// are generated inside record decls so we don't care.
+// https://protobuf.dev/reference/cpp/cpp-generated/#enum
+Name.consume_back("_descriptor");
+Name.consume_back("_IsValid");
+Name.consume_back("_Name");
+Name.consume_back("_Parse");
+Name.consume_back("_MIN");
+Name.consume_back("_MAX");
+Name.consume_back("_ARRAYSIZE");
+return Name.contains('_');
+ }
+
+ // EnumConstantDecls need some special attention, despite being nested in a
+ // TagDecl, they might still have mangled names. We filter those by checking
+ // if it has parent's name as a prefix.
+ // This might go wrong if a nested entity has a name that starts with
parent's
+ // name, e.g: enum Foo { Foo_X }.
+ if (llvm::isa(&ND)) {
+auto *DC = llvm::cast(ND.getDeclContext());
+if (!DC || !DC->getIdentifier())
+ return false;
+auto CtxName = DC->getIdentifier()->getName();
+return !CtxName.empty() && Name.consume_front(CtxName) &&
+ Name.consume_front("_");
+ }
+
+ // Now we're only left with fields/methods without an `_internal_` in the
+ // name, they're intended for public use.
+ return false;
}
// We only collect #include paths for symbols that are suitable for global code
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 0666be95b6b9ee..e8088cb37fa51c
[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/108671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
@@ -52,6 +52,11 @@ struct CodeCompleteOptions {
/// For example, private members are usually inaccessible.
bool IncludeIneligibleResults = false;
+ /// Force sema to load decls from preamble even if an index is provided.
+ /// This is helpful for cases the index can't provide symbols, e.g., the
+ /// unsupported yet modules.
kadircet wrote:
`e.g. with experimental c++20 modules`
https://github.com/llvm/llvm-project/pull/110083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
https://github.com/kadircet approved this pull request. thanks! https://github.com/llvm/llvm-project/pull/110083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/110083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)
@@ -243,14 +244,16 @@ class AnnotatingParser {
// operator that was misinterpreted because we are parsing template
// parameters.
// FIXME: This is getting out of hand, write a decent parser.
- if (InExpr && !Line.startsWith(tok::kw_template) &&
+ if (InExpr && !SeenFatArrow && !Line.startsWith(tok::kw_template) &&
Prev.is(TT_BinaryOperator)) {
const auto Precedence = Prev.getPrecedence();
if (Precedence > prec::Conditional && Precedence < prec::Relational)
return false;
}
if (Prev.isOneOf(tok::question, tok::colon) && !Style.isProto())
SeenTernaryOperator = true;
+ else if (Prev.is(TT_FatArrow))
kadircet wrote:
> The GitHub examples you listed here have zero arguments and thus are
> unambiguous to clang-format. (See
> https://github.com/llvm/llvm-project/pull/110408.) As I mentioned or implied
> above, a pair of empty parentheses after the > would help. We can add another
> patch (if needed) for multiple arguments such as return FixedInt(foo,
> bar); , which is also unambiguous.
Sorry I wasn't paying attention to such details, as I was rather trying to
explain the current logic and the previous one has different characteristics
around `<>`. Trying to fix it for certain special cases won't change that fact
and we'll let regressions around cases that we haven't discovered to slip.
Hence, I totally agree with your approach on only changing behavior in special
cases, rather than changing overall behavior. The point that's puzzling me is,
why don't we treat 834ac2e205dd8e492d6084a7952e68e19a1f54db and
73c961a3345c697f40e2148318f34f5f347701c1 similarly. If they were to just change
handling of ternary expressions, but they changed behavior for expressions that
don't have `?` in them, it sounds like an unintended change.
> However, the initial version of your C++ example return FixedInt(foo);
> has exactly one argument and can be also interpreted as a non-template
> expression similar to Foo < A || B > (C ^ D);.
I completely agree with that, and that's the regression I am trying to express
as well. Previously clang-format was erring towards recognizing these as
template arguments, now it's erring towards recognizing them as comparisons. I
am trying to figure out if we share the same understanding on this topic. If we
both agree on this one, but you folks believe that's a regression we're willing
to eat, I am totally fine with that. In that case I think
https://github.com/llvm/llvm-project/pull/109916 LGTM!
https://github.com/llvm/llvm-project/pull/108671
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
@@ -52,6 +52,9 @@ struct CodeCompleteOptions {
/// For example, private members are usually inaccessible.
bool IncludeIneligibleResults = false;
+ /// Whether the experimental modules support are enabled.
+ bool ExperimentalModulesSupport = false;
kadircet wrote:
can you rename this to `ForceLoadPreamble`? with a comment explaining what it
does
https://github.com/llvm/llvm-project/pull/110083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
@@ -2122,7 +2125,10 @@ clang::CodeCompleteOptions
CodeCompleteOptions::getClangCompleteOpts() const {
// When an is used, Sema is responsible for completing the main file,
// the index can provide results from the preamble.
// Tell Sema not to deserialize the preamble to look for results.
- Result.LoadExternal = !Index;
+ //
+ // FIXME: If we're using C++20 modules, force the lookup process to load
external decls,
+ // since currently the index doesn't support C++20 modules.
kadircet wrote:
i think this comment belongs to `ClangdMain.cpp` instead.
https://github.com/llvm/llvm-project/pull/110083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/110083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
https://github.com/kadircet commented: thanks! https://github.com/llvm/llvm-project/pull/110083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Attach Header to AnalysisResults for misisng headers (PR #110272)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/110272 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Attach Header to AnalysisResults for misisng headers (PR #110272)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/110272
Currently callers of analyze can't get detailed information about a missing
header, e.g. resolve path. Only way to get at this is to use low level walkUsed
funciton, which is way more complicated than just calling analyze.
This enables further analysis, e.g. when includes are spelled relative to inner
directories, caller can still know their path relative to repository root.
From 8f46353bf7ae1befecb6bd5f8c197a069a1a598c Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 27 Sep 2024 15:51:39 +0200
Subject: [PATCH] [include-cleaner] Attach Header to AnalysisResults for
misisng headers
Currently callers of analyze can't get detailed information about a
missing header, e.g. resolve path. Only way to get at this is to use low
level walkUsed funciton, which is way more complicated than just calling
analyze.
This enables further analysis, e.g. when includes are spelled relative
to inner directories, caller can still know their path relative to
repository root.
---
.../include/clang-include-cleaner/Analysis.h | 4 +++-
.../include-cleaner/lib/Analysis.cpp | 16 +++
.../include-cleaner/tool/IncludeCleaner.cpp | 2 +-
.../unittests/AnalysisTest.cpp| 20 +++
4 files changed, 24 insertions(+), 18 deletions(-)
diff --git
a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
index cd2111cf72abf2..46ca3c9d080740 100644
--- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
+++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
@@ -21,6 +21,7 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include
+#include
namespace clang {
class SourceLocation;
@@ -62,7 +63,8 @@ void walkUsed(llvm::ArrayRef ASTRoots,
struct AnalysisResults {
std::vector Unused;
- std::vector Missing; // Spellings, like ""
+ // Spellings, like "" paired with the Header that generated it.
+ std::vector> Missing;
};
/// Determine which headers should be inserted or removed from the main file.
diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
index 05e9d14734a95f..16013f53894e8d 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -26,8 +26,8 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include
@@ -84,7 +84,7 @@ analyze(llvm::ArrayRef ASTRoots,
auto &SM = PP.getSourceManager();
const auto MainFile = *SM.getFileEntryRefForID(SM.getMainFileID());
llvm::DenseSet Used;
- llvm::StringSet<> Missing;
+ llvm::StringMap Missing;
if (!HeaderFilter)
HeaderFilter = [](llvm::StringRef) { return false; };
OptionalDirectoryEntryRef ResourceDir =
@@ -119,7 +119,7 @@ analyze(llvm::ArrayRef ASTRoots,
Satisfied = true;
}
if (!Satisfied)
- Missing.insert(std::move(Spelling));
+ Missing.try_emplace(std::move(Spelling), Providers.front());
});
AnalysisResults Results;
@@ -144,8 +144,8 @@ analyze(llvm::ArrayRef ASTRoots,
}
Results.Unused.push_back(&I);
}
- for (llvm::StringRef S : Missing.keys())
-Results.Missing.push_back(S.str());
+ for (auto &E : Missing)
+Results.Missing.emplace_back(E.first().str(), E.second);
llvm::sort(Results.Missing);
return Results;
}
@@ -158,9 +158,9 @@ std::string fixIncludes(const AnalysisResults &Results,
// Encode insertions/deletions in the magic way clang-format understands.
for (const Include *I : Results.Unused)
cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 1, I->quote(;
- for (llvm::StringRef Spelled : Results.Missing)
-cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 0,
-("#include " + Spelled).str(;
+ for (auto &[Spelled, _] : Results.Missing)
+cantFail(R.add(
+tooling::Replacement(FileName, UINT_MAX, 0, "#include " + Spelled)));
// "cleanup" actually turns the UINT_MAX replacements into concrete edits.
auto Positioned = cantFail(format::cleanupAroundReplacements(Code, R,
Style));
return cantFail(tooling::applyAllReplacements(Code, Positioned));
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index afae4365587aea..080099adc9a07a 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -192,7 +192,7 @@ class
[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)
@@ -79,8 +79,24 @@ enum class StringLiteralKind; // AST classes for statements. //===--===// -/// Stmt - This represents one statement. +/// A statement or expression in the program. /// +/// This is the base for the hierarchy of statements (ForStmt, ReturnStmt...) +/// as well as expressions (Expr, CastExpr, IntegerLiteral...). +/// Classing expressions as Stmt allows them to appear as statements without +/// needing an extra "expression-statement" node. +/// +/// Statements can have children and so form trees. e.g. `while (i>0) i--;`: +/// +/// WhileStmt +/// |-BinaryOperator > +/// | |-DeclRefExpr i +/// | `-IntegerLiteral 0 +/// `-UnaryOperator -- +/// `-DeclRefExpr i +/// +/// These trees are often rooted at function bodies, and attach to the rest +/// of the AST via FunctionDecls. kadircet wrote: i am not sure what's the value we get out of this paragraph. maybe just emphasize they usually attach the AST via Decls? https://github.com/llvm/llvm-project/pull/109795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)
@@ -1802,32 +1802,32 @@ enum class ArraySizeModifier; enum class ElaboratedTypeKeyword; enum class VectorKind; -/// The base class of the type hierarchy. +/// A type in the program, such as `int` or `vector`. +/// This the base class for a hierarchy: PointerType, BuiltinType etc. /// -/// A central concept with types is that each type always has a canonical -/// type. A canonical type is the type with any typedef names stripped out -/// of it or the types it references. For example, consider: +/// Types appear throughout the AST: expressions and (some) declarations have +/// types; casts, new-expressions, and template-arguments refer to types, etc. +/// A Type is not tied to a specific place where it was written (see TypeLoc). /// -/// typedef int foo; -/// typedef foo* bar; -///'int *''foo *''bar' +/// For each distinct type (per language rules) there is one canonical Type. +/// Compound types are formed as trees of simpler types. +/// e.g `int*`: a PointerType(pointee = BuiltinType(kind = Int)). +/// Types are interned: you can compare canonical type equality by pointer. /// -/// There will be a Type object created for 'int'. Since int is canonical, its -/// CanonicalType pointer points to itself. There is also a Type for 'foo' (a -/// TypedefType). Its CanonicalType pointer points to the 'int' Type. Next -/// there is a PointerType that represents 'int*', which, like 'int', is -/// canonical. Finally, there is a PointerType type for 'foo*' whose canonical -/// type is 'int*', and there is a TypedefType for 'bar', whose canonical type -/// is also 'int*'. +/// There are also non-canonical "sugar" types, which also describe e.g. what +/// typedef was used. For example: /// -/// Non-canonical types are useful for emitting diagnostics, without losing -/// information about typedefs being used. Canonical types are useful for type -/// comparisons (they allow by-pointer equality tests) and useful for reasoning -/// about whether something has a particular form (e.g. is a function type), -/// because they implicitly, recursively, strip all typedefs out of a type. +/// using Integer = int; // BuiltinType(kind = int) +/// Integer* x; // PointerType(pointee = TypedefType(decl=Integer)) /// -/// Types, once created, are immutable. +/// The Type obtained from the VarDecl for x reflects how that declaration was +/// written. This is useful for diagnostic messages, for example. +/// Each Type has a pointer to its canonical type, and these should be used for +/// semantic checks: are two types equal, is this a function type, etc. /// +/// All types are allocated within the ASTContext, interned, and immutable. kadircet wrote: i think we should preserve the previous comment that expands on `interned` by mentioning `pointer-equality`. https://github.com/llvm/llvm-project/pull/109795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/109795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang] Add/enhance documentation for some important classes. (PR #109795)
https://github.com/kadircet approved this pull request. thanks a lot for doing this! https://github.com/llvm/llvm-project/pull/109795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
@@ -2108,7 +2116,7 @@ class CodeCompleteFlow {
} // namespace
-clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
+clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts(bool
ForceLoadExternal) const {
kadircet wrote:
> we may need to update all the places using clangd::CodeCompleteOptions
i don't follow this one, i'd expect us to just set it once in ClangdMain.cpp,
with other CodeCompleteOpts fields we set based on command line flags/configs
and use it only once in `CodeCompleteOptions::getClangCompleteOpts` (modulo
tests that're specifically testing this behavior, not even all the tests).
What are the other places that need updating?
https://github.com/llvm/llvm-project/pull/110083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve filtering logic for undesired proto symbols (PR #110091)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/110091
From 08e2a37c8b028efcacd5ee8a269aed837ba16698 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Thu, 26 Sep 2024 10:55:49 +0200
Subject: [PATCH] [clangd] Improve filtering logic for undesired proto symbols
This used to filter any names with `_` in them, apart from
enum-constants. Resulting in discrepancies in behavior when we had
fields that have `_` in the name, or for accessors like `set_`, `has_`.
The logic seems to be trying to filter mangled names for nested entries,
so adjusted logic to only do so for top-level decls, while still
preserving some public top-level helpers.
Heuristics are still leaning towards false-negatives, e.g. if a
top-level entity has `_` in its name (`message Foo_Bar {}`), it'll be
filtered, or an enum that prefixes its type name to constants
(`enum Foo { Foo_OK }`).
---
.../clangd/index/SymbolCollector.cpp | 68 +++
.../clangd/unittests/SymbolCollectorTests.cpp | 64 ++---
2 files changed, 110 insertions(+), 22 deletions(-)
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp
b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index a76894cf0855f3..60739032eab003 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -41,6 +41,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
@@ -75,18 +76,61 @@ bool isPrivateProtoDecl(const NamedDecl &ND) {
if (ND.getIdentifier() == nullptr)
return false;
auto Name = ND.getIdentifier()->getName();
- if (!Name.contains('_'))
-return false;
- // Nested proto entities (e.g. Message::Nested) have top-level decls
- // that shouldn't be used (Message_Nested). Ignore them completely.
- // The nested entities are dangling type aliases, we may want to reconsider
- // including them in the future.
- // For enum constants, SOME_ENUM_CONSTANT is not private and should be
- // indexed. Outer_INNER is private. This heuristic relies on naming style, it
- // will include OUTER_INNER and exclude some_enum_constant.
- // FIXME: the heuristic relies on naming style (i.e. no underscore in
- // user-defined names) and can be improved.
- return (ND.getKind() != Decl::EnumConstant) || llvm::any_of(Name, islower);
+ // There are some internal helpers like _internal_set_foo();
+ if (Name.contains("_internal_"))
+return true;
+
+ // https://protobuf.dev/reference/cpp/cpp-generated/#nested-types
+ // Nested entities (messages/enums) has two names, one at the top-level
scope,
+ // with a mangled name created by prepending all the outer types. These names
+ // are almost never preferred by the developers, so exclude them from index.
+ // e.g.
+ // message Foo {
+ // message Bar {}
+ // enum E { A }
+ // }
+ // yields:
+ // class Foo_Bar {};
+ // enum Foo_E { Foo_E_A };
+ // class Foo {
+ // using Bar = Foo_Bar;
+ // static constexpr Foo_E A = Foo_E_A;
+ // };
+
+ // We get rid of Foo_Bar and Foo_E by discarding any top-level entries with
+ // `_` in the name. This relies on original message/enum not having `_` in
the
+ // name. Hence might go wrong in certain cases.
+ if (ND.getDeclContext()->isNamespace()) {
+// Strip off some known public suffix helpers for enums, rest of the
helpers
+// are generated inside record decls so we don't care.
+// https://protobuf.dev/reference/cpp/cpp-generated/#enum
+Name.consume_back("_descriptor");
+Name.consume_back("_IsValid");
+Name.consume_back("_Name");
+Name.consume_back("_Parse");
+Name.consume_back("_MIN");
+Name.consume_back("_MAX");
+Name.consume_back("_ARRAYSIZE");
+return Name.contains('_');
+ }
+
+ // EnumConstantDecls need some special attention, despite being nested in a
+ // TagDecl, they might still have mangled names. We filter those by checking
+ // if it has parent's name as a prefix.
+ // This might go wrong if a nested entity has a name that starts with
parent's
+ // name, e.g: enum Foo { Foo_X }.
+ if (llvm::isa(&ND)) {
+auto *DC = llvm::cast(ND.getDeclContext());
+if (!DC || !DC->getIdentifier())
+ return false;
+auto CtxName = DC->getIdentifier()->getName();
+return !CtxName.empty() && Name.consume_front(CtxName) &&
+ Name.consume_front("_");
+ }
+
+ // Now we're only left with fields/methods without an `_internal_` in the
+ // name, they're intended for public use.
+ return false;
}
// We only collect #include paths for symbols that are suitable for global code
diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
index 0666be95b6b9ee..e8088cb37fa51c 100644
--- a/clang-tools-e
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
@@ -402,6 +402,46 @@ import A;
EXPECT_TRUE(D.isFromASTFile());
}
+// An end to end test for code complete in modules
+TEST_F(PrerequisiteModulesTests, CodeCompleteTest) {
kadircet wrote:
can you also add a test for signature help?
https://github.com/llvm/llvm-project/pull/110083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
@@ -2108,7 +2116,7 @@ class CodeCompleteFlow {
} // namespace
-clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
+clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts(bool
ForceLoadExternal) const {
kadircet wrote:
instead of taking a parameter can you make it part of
`clangd::CodeCompleteOptions` and initialize it to true if we have
`EnableExperimentalModulesSupport` ?
https://github.com/llvm/llvm-project/pull/110083
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
https://github.com/kadircet requested changes to this pull request. thanks, mostly LG! https://github.com/llvm/llvm-project/pull/110083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] [C++20] [Modules] Support code complete for C++20 modules (PR #110083)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/110083 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Extend diagnose_if to accept more detailed warning information (PR #70976)
@@ -489,13 +485,7 @@ static DiagnosticIDs::Level toLevel(diag::Severity SV) {
DiagnosticIDs::Level
DiagnosticIDs::getDiagnosticLevel(unsigned DiagID, SourceLocation Loc,
const DiagnosticsEngine &Diag) const {
- // Handle custom diagnostics, which cannot be mapped.
kadircet wrote:
ok, putting together a revert for
https://github.com/llvm/llvm-project/commit/e39205654dc11c50bd117e8ccac243a641ebd71f
https://github.com/llvm/llvm-project/pull/70976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve filtering logic for undesired proto symbols (PR #110091)
kadircet wrote: https://reviews.llvm.org/D46751 for context from the original patch https://github.com/llvm/llvm-project/pull/110091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve filtering logic for undesired proto symbols (PR #110091)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/110091
This used to filter any names with `_` in them, apart from
enum-constants. Resulting in discrepancies in behavior when we had
fields that have `_` in the name, or for accessors like `set_`, `has_`.
The logic seems to be trying to filter mangled names for nested entries,
so adjusted logic to only do so for top-level decls, while still
preserving some public top-level helpers.
Heuristics are still leaning towards false-negatives, e.g. if a
top-level entity has `_` in its name (`message Foo_Bar {}`), it'll be
filtered, or an enum that prefixes its type name to constants
(`enum Foo { Foo_OK }`).
From 48e17de386d504f01cf89dba1f7bd445b4ce78e4 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Thu, 26 Sep 2024 10:55:49 +0200
Subject: [PATCH] [clangd] Improve filtering logic for undesired proto symbols
This used to filter any names with `_` in them, apart from
enum-constants. Resulting in discrepancies in behavior when we had
fields that have `_` in the name, or for accessors like `set_`, `has_`.
The logic seems to be trying to filter mangled names for nested entries,
so adjusted logic to only do so for top-level decls, while still
preserving some public top-level helpers.
Heuristics are still leaning towards false-negatives, e.g. if a
top-level entity has `_` in its name (`message Foo_Bar {}`), it'll be
filtered, or an enum that prefixes its type name to constants
(`enum Foo { Foo_OK }`).
---
.../clangd/index/SymbolCollector.cpp | 68 +++
.../clangd/unittests/SymbolCollectorTests.cpp | 65 +++---
2 files changed, 111 insertions(+), 22 deletions(-)
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp
b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index a76894cf0855f3..60739032eab003 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -41,6 +41,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
@@ -75,18 +76,61 @@ bool isPrivateProtoDecl(const NamedDecl &ND) {
if (ND.getIdentifier() == nullptr)
return false;
auto Name = ND.getIdentifier()->getName();
- if (!Name.contains('_'))
-return false;
- // Nested proto entities (e.g. Message::Nested) have top-level decls
- // that shouldn't be used (Message_Nested). Ignore them completely.
- // The nested entities are dangling type aliases, we may want to reconsider
- // including them in the future.
- // For enum constants, SOME_ENUM_CONSTANT is not private and should be
- // indexed. Outer_INNER is private. This heuristic relies on naming style, it
- // will include OUTER_INNER and exclude some_enum_constant.
- // FIXME: the heuristic relies on naming style (i.e. no underscore in
- // user-defined names) and can be improved.
- return (ND.getKind() != Decl::EnumConstant) || llvm::any_of(Name, islower);
+ // There are some internal helpers like _internal_set_foo();
+ if (Name.contains("_internal_"))
+return true;
+
+ // https://protobuf.dev/reference/cpp/cpp-generated/#nested-types
+ // Nested entities (messages/enums) has two names, one at the top-level
scope,
+ // with a mangled name created by prepending all the outer types. These names
+ // are almost never preferred by the developers, so exclude them from index.
+ // e.g.
+ // message Foo {
+ // message Bar {}
+ // enum E { A }
+ // }
+ // yields:
+ // class Foo_Bar {};
+ // enum Foo_E { Foo_E_A };
+ // class Foo {
+ // using Bar = Foo_Bar;
+ // static constexpr Foo_E A = Foo_E_A;
+ // };
+
+ // We get rid of Foo_Bar and Foo_E by discarding any top-level entries with
+ // `_` in the name. This relies on original message/enum not having `_` in
the
+ // name. Hence might go wrong in certain cases.
+ if (ND.getDeclContext()->isNamespace()) {
+// Strip off some known public suffix helpers for enums, rest of the
helpers
+// are generated inside record decls so we don't care.
+// https://protobuf.dev/reference/cpp/cpp-generated/#enum
+Name.consume_back("_descriptor");
+Name.consume_back("_IsValid");
+Name.consume_back("_Name");
+Name.consume_back("_Parse");
+Name.consume_back("_MIN");
+Name.consume_back("_MAX");
+Name.consume_back("_ARRAYSIZE");
+return Name.contains('_');
+ }
+
+ // EnumConstantDecls need some special attention, despite being nested in a
+ // TagDecl, they might still have mangled names. We filter those by checking
+ // if it has parent's name as a prefix.
+ // This might go wrong if a nested entity has a name that starts with
parent's
+ // name, e.g: enum Foo { Foo_X }.
+ if (llvm::isa(&ND)) {
+auto *DC = llvm::cast(ND.getDeclContext());
+if (!DC || !DC->getIdentifier())
[clang] [clang-tools-extra] [clang] Extend diagnose_if to accept more detailed warning information (PR #70976)
@@ -489,13 +485,7 @@ static DiagnosticIDs::Level toLevel(diag::Severity SV) {
DiagnosticIDs::Level
DiagnosticIDs::getDiagnosticLevel(unsigned DiagID, SourceLocation Loc,
const DiagnosticsEngine &Diag) const {
- // Handle custom diagnostics, which cannot be mapped.
kadircet wrote:
so despite this change making sense at a high level, this is a big behavior
change that isn't really obvious in context of this PR.
before this change, clang didn't propagate `-Werror` like mappings into custom
diagnostics. Hence some compilations that succeed now with custom warning
(clang-plugins etc.), will start failing after this change as those will now
turn into errors.
We're already seeing this internally and both
https://github.com/llvm/llvm-project/pull/70976#issuecomment-2357811301 and
https://github.com/llvm/llvm-project/pull/70976#issuecomment-2372463357 are
mentioning this as well.
Since this is triggering a backward incompatible behavior, can we have this bit
reverted and decide how/whether we're planning to move forward with this?
https://github.com/llvm/llvm-project/pull/70976
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)
@@ -243,14 +244,16 @@ class AnnotatingParser {
// operator that was misinterpreted because we are parsing template
// parameters.
// FIXME: This is getting out of hand, write a decent parser.
- if (InExpr && !Line.startsWith(tok::kw_template) &&
+ if (InExpr && !SeenFatArrow && !Line.startsWith(tok::kw_template) &&
Prev.is(TT_BinaryOperator)) {
const auto Precedence = Prev.getPrecedence();
if (Precedence > prec::Conditional && Precedence < prec::Relational)
return false;
}
if (Prev.isOneOf(tok::question, tok::colon) && !Style.isProto())
SeenTernaryOperator = true;
+ else if (Prev.is(TT_FatArrow))
kadircet wrote:
> This patch IMO is a narrowed down version of the other patch
> https://github.com/llvm/llvm-project/pull/100980, which fixed a real
> regression https://github.com/llvm/llvm-project/issues/100300 caused by yet
> another patch https://github.com/llvm/llvm-project/pull/96801, which in turn
> was an attempt at fixing a real bug
> https://github.com/llvm/llvm-project/issues/81385.
I totally get that. The point I am trying to make is, these sequences of
patches, fixed a real regression, at the cost of introducing another one.
> In your
> https://github.com/llvm/llvm-project/pull/108671#discussion_r1762725421
> above, it doesn't seem that your C++ example return FixedInt(foo); is
> from an existing codebase.
Sorry if I gave that impression, but I was delibaretly trying to make it more
concrete as you were asking for more specifics, here's some real world
examples:
https://github.com/search?q=lang%3Ac%2B%2B+%2FFixedInt%3C.*%5C%7C.*%3E%2F&type=code
> The fact that it happened to get formatted that way before doesn't
> necessarily mean that we should always format another similar pattern e.g.
> return Foo < A || B > (C ^ D); the same way, especially if the latter is much
> more likely.
No I totally get that. That's how clang-format evolves over time. But as I
explained above, my understanding is this is done in a way that it doesn't
regress behavior for already formatted code. As otherwise it becomes really
troublesome to adopt new versions of clang-format.
Hence the change in https://github.com/llvm/llvm-project/pull/100980, didn't
simply improve formatting for some mishandled pattern, it also regressed
handling of some existing pattern.
I am totally OK if your verdict here is this is a cost we're willing to pay, in
the end you're the maintainer. I am just trying to make sure this doesn't go
unnoticed and someone is making a call deliberately. Even if it isn't aligned
with the outcome that I might like.
> I've added an option in https://github.com/llvm/llvm-project/pull/109916 to
> help disambiguate the < in the C++ examples above.
It might be a nice addition, but it doesn't really change the fact that
clang-format-18 is WAI for such code patterns, and starting with
clang-format-19 this has regressed and users need to change their
configuration. Moreover it also means that patch now needs to be backported (or
clang-format-19 is just broken for certain codebases).
https://github.com/llvm/llvm-project/pull/108671
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)
@@ -243,14 +244,16 @@ class AnnotatingParser {
// operator that was misinterpreted because we are parsing template
// parameters.
// FIXME: This is getting out of hand, write a decent parser.
- if (InExpr && !Line.startsWith(tok::kw_template) &&
+ if (InExpr && !SeenFatArrow && !Line.startsWith(tok::kw_template) &&
Prev.is(TT_BinaryOperator)) {
const auto Precedence = Prev.getPrecedence();
if (Precedence > prec::Conditional && Precedence < prec::Relational)
return false;
}
if (Prev.isOneOf(tok::question, tok::colon) && !Style.isProto())
SeenTernaryOperator = true;
+ else if (Prev.is(TT_FatArrow))
kadircet wrote:
sorry I wasn't saying that `FixedInt(foo);` is clearly a template
argument, it's definitely ambiguous in absence of name-resolution and there
isn't much clang-format can do here.
this kind of backward incompatible changes are making it harder to update
clang-format to newer versions in large codebases. as clang-format right now is
trading off one set of false-formatting with other. hence my understanding for
the project has been to `stick with existing formatting, if we're heuristically
detecting some patterns. so that behavior on existing formatted-code doesn't
regress.` maybe that understanding has changed over time, or wasn't there at
all. But this bug and requested changes in your PR was solely made with that
assumption (as many others I've been filing over the past years).
hence I am not expecting a "fix" for the issue here, but I guess a narrowed
down version of [other
patch](https://github.com/llvm/llvm-project/commit/73c961a3345c697f40e2148318f34f5f347701c1),
where it actually preserves behavior with old clang-format in uncertain cases.
https://github.com/llvm/llvm-project/pull/108671
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)
kadircet wrote: i think limiting this to `RealFileSystem::openFileForRead` LG, but can we make sure `IsText` defaults to `false` and we can only set it in `#ifdef __MVS__` block to make sure this is really a no-op for other platforms. e.g.: ```cppp auto OpenFlags = sys::fs::OF_None; #ifdef __MVS__ OpenFlags |= sys::fs::OF_Text; #endif Expected FDOrErr = sys::fs::openNativeFileForRead( adjustPath(Name, Storage), OpenFlags, &RealName); ``` I'd still prefer doing this inside https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/Unix/Path.inc#L972-L1118 as we have other utility functions that create file-descriptors and all of them end up using these base functions. By having your logic at a layer above these, you risk having subtle discrepancies. But I don't know much about zOS or why this is needed, so I'll leave management of that risk to you. https://github.com/llvm/llvm-project/pull/107906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)
kadircet wrote: thanks a lot for the swift response! I can see how none of these implementations are not using those flags _today_, but we're changing the observable behavior for them as well, and if some of those implementations decides to give meaning to these flags, it might be impossible to keep it working. Especially because we're not clearly defining the semantics around how callers are going to figure those bits out. So I am wondering: - if we can make these changes in a very limited way, e.g. just in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/Unix/Path.inc#L972-L1118. We already have lots of `__MVS__` specific regions in there. that's also the "right" layer if you want to perform syscalls. - if we need to make those changes in the VFS interfaces indeed, can you provide the rationale explaining that need and what the new semantics mean both for implementors of the interface and the callers of it? https://github.com/llvm/llvm-project/pull/107906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Improve robustness when clang-tidy check names contain leading spaces. (PR #109421)
https://github.com/kadircet approved this pull request. thanks! https://github.com/llvm/llvm-project/pull/109421 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)
kadircet wrote: i've also just noticed https://github.com/llvm/llvm-project/commit/3b3accb598ec87a6a30b0e18ded06071030bb78f, which seem to be pushed without review and any tests, changing behavior more in a non-obvious way. Please not that logic you have in: ``` Expected FDOrErr = sys::fs::openNativeFileForRead( adjustPath(Name, Storage), IsText ? sys::fs::OF_Text : sys::fs::OF_None, &RealName); ``` is affecting all, not just zOS, and by fiddling with the value passed by default, you're in turn changing the flags used by all the platforms. https://github.com/llvm/llvm-project/pull/107906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)
@@ -121,8 +121,18 @@ ContentCache::getBufferOrNone(DiagnosticsEngine &Diag, FileManager &FM, // Start with the assumption that the buffer is invalid to simplify early // return paths. IsBufferInvalid = true; - - auto BufferOrError = FM.getBufferForFile(*ContentsEntry, IsFileVolatile); + bool IsText = false; kadircet wrote: in all other code paths, `IsText` always defaults to true, but you're making it default to `false` here, is this intentional ? https://github.com/llvm/llvm-project/pull/107906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)
@@ -323,10 +325,11 @@ ErrorOr RealFileSystem::status(const Twine &Path)
{
}
ErrorOr>
-RealFileSystem::openFileForRead(const Twine &Name) {
+RealFileSystem::openFileForRead(const Twine &Name, bool IsText) {
SmallString<256> RealName, Storage;
Expected FDOrErr = sys::fs::openNativeFileForRead(
- adjustPath(Name, Storage), sys::fs::OF_None, &RealName);
+ adjustPath(Name, Storage), IsText ? sys::fs::OF_Text : sys::fs::OF_None,
kadircet wrote:
this seems like the only place that uses `IsText`. is there any reason why you
can't deduce that signal from `Name` here directly? rather than propagating it
all the way from the caller?
https://github.com/llvm/llvm-project/pull/107906
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)
https://github.com/kadircet commented: hi sorry for missing this during the review time, i believe this change is changing some of the core support library interfaces in a way that's not justified. this is an interface implemented by quite a lot of both upstream and downstream clients, but this change doesn't describe what the semantics around `IsText` is. so none of these implementations have a way to sustainably understand and maintain this new semantic. Moreover reading the commit description, I also cannot see how callers can figure out what to pass into this parameter. You were probably fine with this during the implementation as they don't need to care, but this will create an unreasonable maintenance burden for the contributors in the future as no one can reason about semantics here. Even further, the way this functionality is implemented today, relies on doing system calls to figure out `IsText`ness. This is against the very nature of a VFS, the behavior you're implementing here will not compose well with rest of the contractual guarantees we have via the VFS abstractions (surely there're cases where we violate that already today, but that isn't a reason to add more). --- So in the light of all of these, I believe this change should've gotten approvals from an LLVM code-owner before submission. In that regard, I can see that you've been trying for the past 2 weeks to get attention, unfortunately community can be slow to respond at times, I'd like to ask you to a little bit more patient, or even start an RFC in discourse to get more visibility (as PRs are quite easy to miss). I was also making some recommendations in `RealFileSystem::openFileForRead` about how this change might be contained. I think it'd be a lot better if you can keep the interfaces clean and introduce the change only into the parts in which these semantics are meaningful. --- Up until these questions are settled (and I am more than happy to follow up on the discussion on my behalf), may I ask you to revert this change? As it's going to create some confusion both in the upstream and downstream users the more it stays around. https://github.com/llvm/llvm-project/pull/107906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [lldb] [llvm] [SystemZ][z/OS] Propagate IsText parameter to open text files as text (PR #107906)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/107906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Suppress all clang warnings (PR #109099)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/109099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] Remove clang-pseudo (PR #109154)
kadircet wrote: thanks a lot for taking care of this @AaronBallman, I think the only concern here is not regressing clangd functionality. We can figure out how we should trim down the bits that're moving into clangd later if need be. I think this LG, but cc @hokein as well. https://github.com/llvm/llvm-project/pull/109154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Suppress all clang warnings (PR #109099)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/109099
This patch disables all clang warnings when running include-cleaner, as
users aren't interested in other findings and in-development code might
have them temporarily. This ensures tool can keep working even in
presence of such issues.
From 5eb5a0cebc789ca0b26f9212af5ae3695274ca38 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 18 Sep 2024 10:38:55 +0200
Subject: [PATCH] [include-cleaner] Suppress all clang warnings
This patch disables all clang warnings when running include-cleaner, as
users aren't interested in other findings and in-development code might
have them temporarily. This ensures tool can keep working even in
presence of such issues.
---
.../include-cleaner/test/tool-ignores-warnings.cpp | 5 +
.../include-cleaner/tool/IncludeCleaner.cpp | 12 +++-
2 files changed, 16 insertions(+), 1 deletion(-)
create mode 100644
clang-tools-extra/include-cleaner/test/tool-ignores-warnings.cpp
diff --git a/clang-tools-extra/include-cleaner/test/tool-ignores-warnings.cpp
b/clang-tools-extra/include-cleaner/test/tool-ignores-warnings.cpp
new file mode 100644
index 00..e207a32c950d5d
--- /dev/null
+++ b/clang-tools-extra/include-cleaner/test/tool-ignores-warnings.cpp
@@ -0,0 +1,5 @@
+// RUN: clang-include-cleaner %s -- -Wunused 2>&1 | FileCheck --allow-empty %s
+static void foo() {}
+
+// Make sure that we don't get an unused warning
+// CHECK-NOT: unused function
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index d8a44ab9b6e12e..afae4365587aea 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -139,7 +139,17 @@ class Action : public clang::ASTFrontendAction {
}
void ExecuteAction() override {
-auto &P = getCompilerInstance().getPreprocessor();
+const auto &CI = getCompilerInstance();
+
+// Disable all warnings when running include-cleaner, as we are only
+// interested in include-cleaner related findings. This makes the tool both
+// more resilient around in-development code, and possibly faster as we
+// skip some extra analysis.
+auto &Diags = CI.getDiagnostics();
+Diags.setEnableAllWarnings(false);
+Diags.setSeverityForAll(clang::diag::Flavor::WarningOrError,
+clang::diag::Severity::Ignored);
+auto &P = CI.getPreprocessor();
P.addPPCallbacks(PP.record(P));
PI.record(getCompilerInstance());
ASTFrontendAction::ExecuteAction();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)
@@ -243,14 +244,16 @@ class AnnotatingParser {
// operator that was misinterpreted because we are parsing template
// parameters.
// FIXME: This is getting out of hand, write a decent parser.
- if (InExpr && !Line.startsWith(tok::kw_template) &&
+ if (InExpr && !SeenFatArrow && !Line.startsWith(tok::kw_template) &&
Prev.is(TT_BinaryOperator)) {
const auto Precedence = Prev.getPrecedence();
if (Precedence > prec::Conditional && Precedence < prec::Relational)
return false;
}
if (Prev.isOneOf(tok::question, tok::colon) && !Style.isProto())
SeenTernaryOperator = true;
+ else if (Prev.is(TT_FatArrow))
kadircet wrote:
i don't think new or empty parantheses are a must either, the logic around here
is just checking that `<>` are inside an expression, hence that kind of code
pattern is likely quite common as people tend to do arithmetic via
metaprogramming using these patterns. e.g:
```cpp
constexpr auto foo() { return FixedInt(foo); }
```
this will be formatted as the following now:
```cpp
constexpr auto foo() { return FixedInt < N | M > (foo); }
```
https://github.com/llvm/llvm-project/pull/108671
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Format] Dont treat LBrace after extends/implements as initializer list (PR #108524)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/108524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)
https://github.com/kadircet requested changes to this pull request. https://github.com/llvm/llvm-project/pull/108671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)
@@ -243,14 +244,16 @@ class AnnotatingParser {
// operator that was misinterpreted because we are parsing template
// parameters.
// FIXME: This is getting out of hand, write a decent parser.
- if (InExpr && !Line.startsWith(tok::kw_template) &&
+ if (InExpr && !SeenFatArrow && !Line.startsWith(tok::kw_template) &&
Prev.is(TT_BinaryOperator)) {
const auto Precedence = Prev.getPrecedence();
if (Precedence > prec::Conditional && Precedence < prec::Relational)
return false;
}
if (Prev.isOneOf(tok::question, tok::colon) && !Style.isProto())
SeenTernaryOperator = true;
+ else if (Prev.is(TT_FatArrow))
kadircet wrote:
i am not sure if the regression is specific to fat-arrow here (sorry for not
putting much effort into the reproducer). the original change seem to changed
behavior for handling of any bitwise operators. so I think something like:
```cpp
auto foo = new Bar();
```
would also be formatted differently
https://github.com/llvm/llvm-project/pull/108671
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Format] Dont treat LBrace after extends/implements as initializer list (PR #108524)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/108524
From e63f2acd673183d15420b41ebe50a1c061de0905 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 13 Sep 2024 11:31:43 +0200
Subject: [PATCH] [Format] Dont treat LBrace after extends/implements as
initializer list
This extends the fix in https://github.com/llvm/llvm-project/pull/106242
for other derived class types.
---
clang/lib/Format/UnwrappedLineParser.cpp | 2 +-
clang/unittests/Format/FormatTestJS.cpp | 7 ++-
clang/unittests/Format/TokenAnnotatorTest.cpp | 8
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp
b/clang/lib/Format/UnwrappedLineParser.cpp
index 1727ed93822b1b..40f77266fabdca 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -4042,7 +4042,7 @@ void UnwrappedLineParser::parseRecord(bool ParseAsExpr) {
}
auto IsListInitialization = [&] {
-if (!ClassName || IsDerived)
+if (!ClassName || IsDerived || JSPastExtendsOrImplements)
return false;
assert(FormatTok->is(tok::l_brace));
const auto *Prev = FormatTok->getPreviousNonComment();
diff --git a/clang/unittests/Format/FormatTestJS.cpp
b/clang/unittests/Format/FormatTestJS.cpp
index c25228a69a748f..57c021c76867f7 100644
--- a/clang/unittests/Format/FormatTestJS.cpp
+++ b/clang/unittests/Format/FormatTestJS.cpp
@@ -579,12 +579,17 @@ TEST_F(FormatTestJS, GoogScopes) {
"});");
}
-TEST_F(FormatTestJS, GoogAnonymousClass) {
+TEST_F(FormatTestJS, ClassExtends) {
verifyFormat("a = class extends goog.structs.a {\n"
" a() {\n"
"return 0;\n"
" }\n"
"};");
+ verifyFormat("a = class Foo extends goog.structs.a {\n"
+ " a() {\n"
+ "return 0;\n"
+ " }\n"
+ "};");
}
TEST_F(FormatTestJS, IIFEs) {
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp
b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 5c28e3a4ea5a1f..baa5ab0ac5e456 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -3277,6 +3277,14 @@ TEST_F(TokenAnnotatorTest, BraceKind) {
EXPECT_TOKEN(Tokens[8], tok::r_brace, TT_ClassRBrace);
EXPECT_BRACE_KIND(Tokens[8], BK_Block);
+ Tokens = annotate("a = class Foo extends goog.a {};",
+getGoogleStyle(FormatStyle::LK_JavaScript));
+ ASSERT_EQ(Tokens.size(), 12u) << Tokens;
+ EXPECT_TOKEN(Tokens[8], tok::l_brace, TT_ClassLBrace);
+ EXPECT_BRACE_KIND(Tokens[8], BK_Block);
+ EXPECT_TOKEN(Tokens[9], tok::r_brace, TT_ClassRBrace);
+ EXPECT_BRACE_KIND(Tokens[9], BK_Block);
+
Tokens = annotate("#define FOO(X) \\\n"
" struct X##_tag_ {};");
ASSERT_EQ(Tokens.size(), 14u) << Tokens;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Format] Dont treat LBrace after extends/implements as initializer list (PR #108524)
kadircet wrote: added, ptal https://github.com/llvm/llvm-project/pull/108524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a regression on BAS_AlwaysBreak (PR #107506)
kadircet wrote: i've also found 3 more cases that point back to same patch in https://github.com/llvm/llvm-project/issues/107574, in case you want to address all of them with this change https://github.com/llvm/llvm-project/pull/107506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a regression on BAS_AlwaysBreak (PR #107506)
https://github.com/kadircet requested changes to this pull request. https://github.com/llvm/llvm-project/pull/107506 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a regression on BAS_AlwaysBreak (PR #107506)
@@ -861,7 +861,7 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState
&State, bool DryRun,
// or
// cal(
// new SomethingElseee());
- !IsSimpleFunction(Current)) {
+ Current.isNot(tok::comment) && !IsSimpleFunction(Current)) {
kadircet wrote:
would it make sense to introduce this into `IsSimpleFunction` ? similar to
handling of `new` keyword in there? possibly just skipping over all of the
comments. because we can also have `foo(/*comments*/ new X())`, and i think we
still want that inner expression to be treated simple
https://github.com/llvm/llvm-project/pull/107506
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix a bug in annotating CastRParen (PR #102261)
kadircet wrote: there still seems to be a regression when the operator is `-`, filed https://github.com/llvm/llvm-project/issues/107568 https://github.com/llvm/llvm-project/pull/102261 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [clang-format] Add support for BasedOnStyle referencing an arbitrary file (PR #107312)
kadircet wrote: I am not sure if it's best to push a new command line flag to all other tools that use clang-format as a library. Have you considered any other alternatives that can self-contain this in clang-format? e.g can we just treat paths as relative to current `.clang-format` file ? https://github.com/llvm/llvm-project/pull/107312 ___ 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 collect macros when clang-tidy checks call into the preprocessor (PR #106329)
@@ -702,6 +704,10 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
log("Execute() failed when building AST for {0}: {1}", MainInput.getFile(),
toString(std::move(Err)));
+ // Disable the macro collector for the remainder of this function, e.g.
+ // clang-tidy checkers.
+ MacroCollectorPtr->doneParse();
kadircet wrote:
> add a new method to PPCallbacks, e.g. BuildASTDone() or such
I'd first see if we can extend existing `(Lexed)FileChanged` callbacks to fit
this use case without breaking any users.
but if that doesn't work, yes, a new callback would be needed.
https://github.com/llvm/llvm-project/pull/106329
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Update TidyFastChecks for release/19.x (PR #106354)
kadircet wrote: /cherry-pick b47d7ce8121b1cb1923e879d58eaa1d63aeaaae2 https://github.com/llvm/llvm-project/pull/106354 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Update TidyFastChecks for release/19.x (PR #106354)
https://github.com/kadircet milestoned https://github.com/llvm/llvm-project/pull/106354 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Update TidyFastChecks for release/19.x (PR #106354)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/106354 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/106706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/106706
From 53b34977daa00abdad9bb6b3a351b05ae59cb2df Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 30 Aug 2024 12:28:02 +0200
Subject: [PATCH] [include-cleaner] Report refs for enum constants used through
namespace aliases
---
.../include-cleaner/lib/WalkAST.cpp | 27 ---
.../include-cleaner/unittests/WalkASTTest.cpp | 3 +++
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index f7a2ebd5260681..aae3eda519ffdc 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -15,6 +15,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/NestedNameSpecifier.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/TemplateBase.h"
#include "clang/AST/TemplateName.h"
@@ -23,9 +24,11 @@
#include "clang/Basic/IdentifierTable.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/ErrorHandling.h"
namespace clang::include_cleaner {
namespace {
@@ -125,6 +128,24 @@ class ASTWalker : public RecursiveASTVisitor {
return true;
}
+ bool qualifierIsNamespaceOrNone(DeclRefExpr *DRE) {
+const auto *Qual = DRE->getQualifier();
+if (!Qual)
+ return true;
+switch (Qual->getKind()) {
+case NestedNameSpecifier::Namespace:
+case NestedNameSpecifier::NamespaceAlias:
+case NestedNameSpecifier::Global:
+ return true;
+case NestedNameSpecifier::TypeSpec:
+case NestedNameSpecifier::TypeSpecWithTemplate:
+case NestedNameSpecifier::Super:
+case NestedNameSpecifier::Identifier:
+ return false;
+}
+llvm_unreachable("Unknown value for NestedNameSpecifierKind");
+ }
+
bool VisitDeclRefExpr(DeclRefExpr *DRE) {
auto *FD = DRE->getFoundDecl();
// Prefer the underlying decl if FoundDecl isn't a shadow decl, e.g:
@@ -146,10 +167,8 @@ class ASTWalker : public RecursiveASTVisitor {
//
// If it's an enum constant, it must be due to prior decl. Report
references
// to it when qualifier isn't a type.
-if (llvm::isa(FD)) {
- if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace())
-report(DRE->getLocation(), FD);
-}
+if (llvm::isa(FD) && qualifierIsNamespaceOrNone(DRE))
+ report(DRE->getLocation(), FD);
return true;
}
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 9286758cab081c..e45ea36f7938ea 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -534,6 +534,9 @@ TEST(WalkAST, Enums) {
testWalk(R"(namespace ns { enum E { A = 42 }; }
struct S { using ns::E::A; };)",
"int e = S::^A;");
+ testWalk(R"(namespace ns { enum E { $explicit^A = 42 }; })",
+ "namespace z = ns; int e = z::^A;");
+ testWalk(R"(enum E { $explicit^A = 42 };)", "int e = ::^A;");
}
TEST(WalkAST, InitializerList) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
@@ -146,9 +148,24 @@ class ASTWalker : public RecursiveASTVisitor {
//
// If it's an enum constant, it must be due to prior decl. Report
references
// to it when qualifier isn't a type.
-if (llvm::isa(FD)) {
- if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace())
-report(DRE->getLocation(), FD);
+auto QualifierIsNamepsaceOrNone = [&DRE]() {
+ const auto *Qual = DRE->getQualifier();
+ if (!Qual)
+return true;
+ switch (Qual->getKind()) {
+ case NestedNameSpecifier::Namespace:
+ case NestedNameSpecifier::NamespaceAlias:
+ case NestedNameSpecifier::Global:
+return true;
+ case NestedNameSpecifier::TypeSpec:
+ case NestedNameSpecifier::TypeSpecWithTemplate:
+ case NestedNameSpecifier::Super:
+ case NestedNameSpecifier::Identifier:
+return false;
+ }
kadircet wrote:
we already have `Wswitch` in LLVM so there should be warnings (unless we
actually add a default clause). added llvm_unreachable to have some more useful
error messages at runtime though.
https://github.com/llvm/llvm-project/pull/106706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Hide the `TargetOptions` pointer from `CompilerInvocation` (PR #106271)
@@ -216,7 +216,7 @@ enum OpenCLTypeKind : uint8_t {
///
class TargetInfo : public TransferrableTargetInfo,
public RefCountedBase {
- std::shared_ptr TargetOpts;
+ TargetOptions *TargetOpts;
kadircet wrote:
this is making me a little anxious. previously a `TargetInfo` was valid even if
`TargetOptions` used during construction went away.
now we're breaking that contract, any code pattern that keeps `TargetInfo`s
alive, now needs to make sure underlying options are outliving it. AFAICT, you
ensured usages upstream are updated accordingly (and are safe), but I am afraid
this might break downstream projects. This is fine, but it would be nice to
prevent it if we can.
I do understand we need to hide `TargetOptions` from `CompilerInvocation`, but
do we have a reason to make this reference non-owning ? (this should also
ensure we don't create a new unique_ptr for AuxTargetOpts in
`CompilerInstance.h`.
https://github.com/llvm/llvm-project/pull/106271
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/106706
From 55be096324da41a17612fb67bfa521a8f42c7874 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 30 Aug 2024 12:28:02 +0200
Subject: [PATCH] [include-cleaner] Report refs for enum constants used through
namespace aliases
---
.../include-cleaner/lib/WalkAST.cpp | 23 ---
.../include-cleaner/unittests/WalkASTTest.cpp | 3 +++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index f7a2ebd5260681..8475577c565ad5 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -15,6 +15,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/NestedNameSpecifier.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/TemplateBase.h"
#include "clang/AST/TemplateName.h"
@@ -23,6 +24,7 @@
#include "clang/Basic/IdentifierTable.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/Specifiers.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
@@ -146,9 +148,24 @@ class ASTWalker : public RecursiveASTVisitor {
//
// If it's an enum constant, it must be due to prior decl. Report
references
// to it when qualifier isn't a type.
-if (llvm::isa(FD)) {
- if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace())
-report(DRE->getLocation(), FD);
+auto QualifierIsNamepsaceOrNone = [&DRE]() {
+ const auto *Qual = DRE->getQualifier();
+ if (!Qual)
+return true;
+ switch (Qual->getKind()) {
+ case NestedNameSpecifier::Namespace:
+ case NestedNameSpecifier::NamespaceAlias:
+ case NestedNameSpecifier::Global:
+return true;
+ case NestedNameSpecifier::TypeSpec:
+ case NestedNameSpecifier::TypeSpecWithTemplate:
+ case NestedNameSpecifier::Super:
+ case NestedNameSpecifier::Identifier:
+return false;
+ }
+};
+if (llvm::isa(FD) && QualifierIsNamepsaceOrNone()) {
+ report(DRE->getLocation(), FD);
}
return true;
}
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 9286758cab081c..e45ea36f7938ea 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -534,6 +534,9 @@ TEST(WalkAST, Enums) {
testWalk(R"(namespace ns { enum E { A = 42 }; }
struct S { using ns::E::A; };)",
"int e = S::^A;");
+ testWalk(R"(namespace ns { enum E { $explicit^A = 42 }; })",
+ "namespace z = ns; int e = z::^A;");
+ testWalk(R"(enum E { $explicit^A = 42 };)", "int e = ::^A;");
}
TEST(WalkAST, InitializerList) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
@@ -534,6 +534,8 @@ TEST(WalkAST, Enums) {
testWalk(R"(namespace ns { enum E { A = 42 }; }
struct S { using ns::E::A; };)",
"int e = S::^A;");
+ testWalk(R"(namespace ns { enum E { $explicit^A = 42 }; })",
+ "namespace z = ns; int e = z::^A;");
kadircet wrote:
ugh, apparently for global namespaces, we don't store any decls. hence we were
failing. tweaked the code a little.
https://github.com/llvm/llvm-project/pull/106706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Correctly annotate braces in ObjC square brackets (PR #106654)
https://github.com/kadircet approved this pull request. thanks! https://github.com/llvm/llvm-project/pull/106654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/106706
None
From 82ac332a3b94f6f753b706077b13f405ff776882 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Fri, 30 Aug 2024 12:28:02 +0200
Subject: [PATCH] [include-cleaner] Report refs for enum constants used through
namespace aliases
---
clang-tools-extra/include-cleaner/lib/WalkAST.cpp | 3 ++-
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index f7a2ebd5260681..9663c9916740eb 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -147,7 +147,8 @@ class ASTWalker : public RecursiveASTVisitor {
// If it's an enum constant, it must be due to prior decl. Report
references
// to it when qualifier isn't a type.
if (llvm::isa(FD)) {
- if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace())
+ if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace() ||
+ DRE->getQualifier()->getAsNamespaceAlias())
report(DRE->getLocation(), FD);
}
return true;
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 9286758cab081c..9ae9f9a5ddb829 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -534,6 +534,8 @@ TEST(WalkAST, Enums) {
testWalk(R"(namespace ns { enum E { A = 42 }; }
struct S { using ns::E::A; };)",
"int e = S::^A;");
+ testWalk(R"(namespace ns { enum E { $explicit^A = 42 }; })",
+ "namespace z = ns; int e = z::^A;");
}
TEST(WalkAST, InitializerList) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/106241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/106241
From c8c29eae625991ee1ea7dfbfcda4a31f0b19b2e3 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Tue, 27 Aug 2024 17:56:47 +0200
Subject: [PATCH] [clang] Cleanup IncludeLocMap
CompilerInstance can re-use same SourceManager across multiple
frontendactions. During this process it calls
`SourceManager::clearIDTables` to reset any caches based on FileIDs.
It didn't reset IncludeLocMap, resulting in wrong include locations for
workflows that triggered multiple frontend-actions through same
CompilerInstance.
---
clang/lib/Basic/SourceManager.cpp | 1 +
clang/unittests/Basic/SourceManagerTest.cpp | 60 +
2 files changed, 61 insertions(+)
diff --git a/clang/lib/Basic/SourceManager.cpp
b/clang/lib/Basic/SourceManager.cpp
index b0256a8ce9ed04..d6ec26af80aadd 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -350,6 +350,7 @@ void SourceManager::clearIDTables() {
LastLineNoContentCache = nullptr;
LastFileIDLookup = FileID();
+ IncludedLocMap.clear();
if (LineTable)
LineTable->clear();
diff --git a/clang/unittests/Basic/SourceManagerTest.cpp
b/clang/unittests/Basic/SourceManagerTest.cpp
index 45840f5188cdcd..0f2476bd8b0612 100644
--- a/clang/unittests/Basic/SourceManagerTest.cpp
+++ b/clang/unittests/Basic/SourceManagerTest.cpp
@@ -20,6 +20,7 @@
#include "clang/Lex/PreprocessorOptions.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Process.h"
#include "gtest/gtest.h"
#include
@@ -453,6 +454,65 @@ TEST_F(SourceManagerTest,
loadedSLocEntryIsInTheSameTranslationUnit) {
#if defined(LLVM_ON_UNIX)
+// A single SourceManager instance is sometimes reused across multiple
+// compilations. This test makes sure we're resetting caches built for tracking
+// include locations that are based on FileIDs, to make sure we don't report
+// wrong include locations when FileIDs coincide between two different runs.
+TEST_F(SourceManagerTest, ResetsIncludeLocMap) {
+ auto ParseFile = [&] {
+TrivialModuleLoader ModLoader;
+HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+Diags, LangOpts, &*Target);
+Preprocessor PP(std::make_shared(), Diags, LangOpts,
+SourceMgr, HeaderInfo, ModLoader,
+/*IILookup =*/nullptr,
+/*OwnsHeaderSearch =*/false);
+PP.Initialize(*Target);
+PP.EnterMainSourceFile();
+PP.LexTokensUntilEOF();
+EXPECT_FALSE(Diags.hasErrorOccurred());
+ };
+
+ auto Buf = llvm::MemoryBuffer::getMemBuffer("");
+ FileEntryRef HeaderFile =
+ FileMgr.getVirtualFileRef("/foo.h", Buf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(HeaderFile, std::move(Buf));
+
+ Buf = llvm::MemoryBuffer::getMemBuffer(R"cpp(#include "/foo.h")cpp");
+ FileEntryRef BarFile =
+ FileMgr.getVirtualFileRef("/bar.h", Buf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(BarFile, std::move(Buf));
+ SourceMgr.createFileID(BarFile, {}, clang::SrcMgr::C_User);
+
+ Buf = llvm::MemoryBuffer::getMemBuffer(R"cpp(#include "/foo.h")cpp");
+ FileID MFID = SourceMgr.createFileID(std::move(Buf));
+ SourceMgr.setMainFileID(MFID);
+
+ ParseFile();
+ auto FooFID = SourceMgr.getOrCreateFileID(HeaderFile, clang::SrcMgr::C_User);
+ auto IncFID = SourceMgr.getDecomposedIncludedLoc(FooFID).first;
+ EXPECT_EQ(IncFID, MFID);
+
+ // Clean up source-manager state before we start next parse.
+ SourceMgr.clearIDTables();
+
+ // Set up a new main file.
+ Buf = llvm::MemoryBuffer::getMemBuffer(R"cpp(
+ // silly comment 42
+ #include "/bar.h")cpp");
+ MFID = SourceMgr.createFileID(std::move(Buf));
+ SourceMgr.setMainFileID(MFID);
+
+ ParseFile();
+ // Make sure foo.h got the same file-id in both runs.
+ EXPECT_EQ(FooFID,
+SourceMgr.getOrCreateFileID(HeaderFile, clang::SrcMgr::C_User));
+ auto BarFID = SourceMgr.getOrCreateFileID(BarFile, clang::SrcMgr::C_User);
+ IncFID = SourceMgr.getDecomposedIncludedLoc(FooFID).first;
+ // Check that includer is bar.h during this run.
+ EXPECT_EQ(IncFID, BarFID);
+}
+
TEST_F(SourceManagerTest, getMacroArgExpandedLocation) {
const char *header =
"#define FM(x,y) x\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Correctly annotate braces in ObjC square brackets (PR #106654)
@@ -3286,6 +3286,14 @@ TEST_F(TokenAnnotatorTest, BlockLBrace) {
EXPECT_BRACE_KIND(Tokens[4], BK_Block);
EXPECT_TOKEN(Tokens[5], tok::l_brace, TT_BlockLBrace);
EXPECT_BRACE_KIND(Tokens[5], BK_Block);
+
+ Tokens = annotate("[foo bar:{{0, 1}}];", getLLVMStyle(FormatStyle::LK_ObjC));
kadircet wrote:
i believe this case was already WAI (somehow), can you add another selector
after `}}`. e.g: `[foo bar:{{0, 1}} baz: baz];`
https://github.com/llvm/llvm-project/pull/106654
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Mark RecordDecls referenced in UsingDecls as explicit (PR #106430)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/106430 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Hide the `DiagnosticOptions` pointer from `CompilerInvocation` (PR #106274)
kadircet wrote: yes turning diagnosticoptions to be a ThreadSafeRefCountedBase pointer should ensure we have similar guarantees. so LG from clangd side, but I am not sure about implications on using thread-aware structs in core parts of clang. so it might be worthwhile to get some opinions from clang maintainers as well. https://github.com/llvm/llvm-project/pull/106274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
@@ -350,6 +350,7 @@ void SourceManager::clearIDTables() {
LastLineNoContentCache = nullptr;
LastFileIDLookup = FileID();
+ IncludedLocMap.clear();
kadircet wrote:
done
https://github.com/llvm/llvm-project/pull/106241
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/106241
From f8fb04379255a783f1fbdcd07cfff5846d253d32 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Tue, 27 Aug 2024 17:56:47 +0200
Subject: [PATCH] [clang] Cleanup IncludeLocMap
CompilerInstance can re-use same SourceManager across multiple
frontendactions. During this process it calls
`SourceManager::clearIDTables` to reset any caches based on FileIDs.
It didn't reset IncludeLocMap, resulting in wrong include locations for
workflows that triggered multiple frontend-actions through same
CompilerInstance.
---
clang/lib/Basic/SourceManager.cpp | 1 +
clang/unittests/Basic/SourceManagerTest.cpp | 56 +
2 files changed, 57 insertions(+)
diff --git a/clang/lib/Basic/SourceManager.cpp
b/clang/lib/Basic/SourceManager.cpp
index b0256a8ce9ed04..d6ec26af80aadd 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -350,6 +350,7 @@ void SourceManager::clearIDTables() {
LastLineNoContentCache = nullptr;
LastFileIDLookup = FileID();
+ IncludedLocMap.clear();
if (LineTable)
LineTable->clear();
diff --git a/clang/unittests/Basic/SourceManagerTest.cpp
b/clang/unittests/Basic/SourceManagerTest.cpp
index 45840f5188cdcd..8d6e6e789cc057 100644
--- a/clang/unittests/Basic/SourceManagerTest.cpp
+++ b/clang/unittests/Basic/SourceManagerTest.cpp
@@ -20,6 +20,7 @@
#include "clang/Lex/PreprocessorOptions.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Process.h"
#include "gtest/gtest.h"
#include
@@ -453,6 +454,61 @@ TEST_F(SourceManagerTest,
loadedSLocEntryIsInTheSameTranslationUnit) {
#if defined(LLVM_ON_UNIX)
+TEST_F(SourceManagerTest, ResetsIncludeLocMap) {
+ auto ParseFile = [&] {
+TrivialModuleLoader ModLoader;
+HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+Diags, LangOpts, &*Target);
+Preprocessor PP(std::make_shared(), Diags, LangOpts,
+SourceMgr, HeaderInfo, ModLoader,
+/*IILookup =*/nullptr,
+/*OwnsHeaderSearch =*/false);
+PP.Initialize(*Target);
+PP.EnterMainSourceFile();
+PP.LexTokensUntilEOF();
+EXPECT_FALSE(Diags.hasErrorOccurred());
+ };
+
+ auto Buf = llvm::MemoryBuffer::getMemBuffer("");
+ FileEntryRef HeaderFile = FileMgr.getVirtualFileRef(
+ "/foo.h", Buf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(HeaderFile, std::move(Buf));
+
+ Buf = llvm::MemoryBuffer::getMemBuffer(R"cpp(#include "/foo.h")cpp");
+ FileEntryRef BarFile = FileMgr.getVirtualFileRef(
+ "/bar.h", Buf->getBufferSize(), 0);
+ SourceMgr.overrideFileContents(BarFile, std::move(Buf));
+ SourceMgr.createFileID(BarFile, {}, clang::SrcMgr::C_User);
+
+ Buf = llvm::MemoryBuffer::getMemBuffer(R"cpp(#include "/foo.h")cpp");
+ FileID MFID = SourceMgr.createFileID(std::move(Buf));
+ SourceMgr.setMainFileID(MFID);
+
+ ParseFile();
+ auto FooFID = SourceMgr.getOrCreateFileID(HeaderFile, clang::SrcMgr::C_User);
+ auto IncFID = SourceMgr.getDecomposedIncludedLoc(FooFID).first;
+ EXPECT_EQ(IncFID, MFID);
+
+ // Clean up source-manager state before we start next parse.
+ SourceMgr.clearIDTables();
+
+ // Set up a new main file.
+ Buf = llvm::MemoryBuffer::getMemBuffer(R"cpp(
+ // silly comment 42
+ #include "/bar.h")cpp");
+ MFID = SourceMgr.createFileID(std::move(Buf));
+ SourceMgr.setMainFileID(MFID);
+
+ ParseFile();
+ // Make sure foo.h got the same file-id in both runs.
+ EXPECT_EQ(FooFID,
+SourceMgr.getOrCreateFileID(HeaderFile, clang::SrcMgr::C_User));
+ auto BarFID = SourceMgr.getOrCreateFileID(BarFile, clang::SrcMgr::C_User);
+ IncFID = SourceMgr.getDecomposedIncludedLoc(FooFID).first;
+ // Check that includer is bar.h during this run.
+ EXPECT_EQ(IncFID, BarFID);
+}
+
TEST_F(SourceManagerTest, getMacroArgExpandedLocation) {
const char *header =
"#define FM(x,y) x\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Mark RecordDecls referenced in UsingDecls as explicit (PR #106430)
https://github.com/kadircet updated
https://github.com/llvm/llvm-project/pull/106430
From 9a96724fba63c91eefca804112c8e862e5427c10 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 28 Aug 2024 20:30:08 +0200
Subject: [PATCH] [include-cleaner] Mark RecordDecls referenced in UsingDecls
as explicit
We were reporting ambigious references from using declarations as user
can be depending on different overloads of a function just because they
are visible in the TU.
This doesn't apply to records, or primary templates as declaration being
referenced in such cases is unambigious, the ambiguity applies to
specializations though.
Hence this patch returns an explicit reference to record decls and
primary templates of those.
---
clang-tools-extra/include-cleaner/lib/WalkAST.cpp | 7 ++-
.../include-cleaner/unittests/WalkASTTest.cpp | 11 ---
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index 598484d09712e5..f7a2ebd5260681 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -203,7 +203,12 @@ class ASTWalker : public RecursiveASTVisitor {
bool VisitUsingDecl(UsingDecl *UD) {
for (const auto *Shadow : UD->shadows()) {
auto *TD = Shadow->getTargetDecl();
- auto IsUsed = TD->isUsed() || TD->isReferenced();
+ // For function-decls, we might have overloads brought in due to
+ // transitive dependencies. Hence we only want to report explicit
+ // references for those if they're used.
+ // But for record decls, spelling of the type always refers to primary
+ // decl non-ambiguously. Hence spelling is already a use.
+ auto IsUsed = TD->isUsed() || TD->isReferenced() || !TD->getAsFunction();
report(UD->getLocation(), TD,
IsUsed ? RefType::Explicit : RefType::Ambiguous);
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 6c8eacbff1cea3..9286758cab081c 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -255,7 +255,7 @@ TEST(WalkAST, TemplateSpecializationsFromUsingDecl) {
// Class templates
testWalk(R"cpp(
namespace ns {
-template class $ambiguous^Z {}; // primary template
+template class $explicit^Z {}; // primary template
template class $ambiguous^Z {}; // partial specialization
template<> class $ambiguous^Z {};// full specialization
}
@@ -265,7 +265,7 @@ template<> class $ambiguous^Z {};// full
specialization
// Var templates
testWalk(R"cpp(
namespace ns {
-template T $ambiguous^foo; // primary template
+template T $explicit^foo; // primary template
template T $ambiguous^foo; // partial specialization
template<> int* $ambiguous^foo; // full specialization
}
@@ -335,7 +335,12 @@ TEST(WalkAST, Using) {
testWalk(R"cpp(
namespace ns {
template
- class $ambiguous^Y {};
+ class $explicit^Y {};
+})cpp",
+ "using ns::^Y;");
+ testWalk(R"cpp(
+namespace ns {
+ class $explicit^Y {};
})cpp",
"using ns::^Y;");
testWalk(R"cpp(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Mark RecordDecls referenced in UsingDecls as explicit (PR #106430)
@@ -203,7 +203,7 @@ class ASTWalker : public RecursiveASTVisitor {
bool VisitUsingDecl(UsingDecl *UD) {
for (const auto *Shadow : UD->shadows()) {
auto *TD = Shadow->getTargetDecl();
- auto IsUsed = TD->isUsed() || TD->isReferenced();
+ auto IsUsed = TD->isUsed() || TD->isReferenced() || !TD->getAsFunction();
kadircet wrote:
Well I was actually thinking that `usedness` is still the right concept here,
spelling of the type name in the using declaration is enough to trigger that
use for records.
I added comments also along those lines, LMK if it still doesn't resonate with
you.
https://github.com/llvm/llvm-project/pull/106430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't merge a short block for SBS_Never (PR #88238)
kadircet wrote:
This seems to be recognizing some initliazer list statements as blocks as well,
e.g:
```objc
$ cat a.m
[bar bat:{{0, 1, 2, 3}} qq: qq];
```
this seems to be formatted as:
```objc
$ ~/repos/llvm/build/bin/clang-format -style='{AllowShortBlocksOnASingleLine:
Never}' a.m
[bar bat:{
{
0, 1, 2, 3
}
}
qq:qq];
```
https://github.com/llvm/llvm-project/pull/88238
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Correctly identifies token-pasted record names (PR #106484)
https://github.com/kadircet approved this pull request. thanks! https://github.com/llvm/llvm-project/pull/106484 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Mark RecordDecls referenced in UsingDecls as explicit (PR #106430)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/106430
We were reporting ambigious references from using declarations as user
can be depending on different overloads of a function just because they
are visible in the TU.
This doesn't apply to records, or primary templates as declaration being
referenced in such cases is unambigious, the ambiguity applies to
specializations though.
Hence this patch returns an explicit reference to record decls and
primary templates of those.
From 89694986accf28acb6605214daa72dfe313017dd Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Wed, 28 Aug 2024 20:30:08 +0200
Subject: [PATCH] [include-cleaner] Mark RecordDecls referenced in UsingDecls
as explicit
We were reporting ambigious references from using declarations as user
can be depending on different overloads of a function just because they
are visible in the TU.
This doesn't apply to records, or primary templates as declaration being
referenced in such cases is unambigious, the ambiguity applies to
specializations though.
Hence this patch returns an explicit reference to record decls and
primary templates of those.
---
clang-tools-extra/include-cleaner/lib/WalkAST.cpp | 2 +-
.../include-cleaner/unittests/WalkASTTest.cpp | 11 ---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index 598484d09712e5..5e6b862caf0d6c 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -203,7 +203,7 @@ class ASTWalker : public RecursiveASTVisitor {
bool VisitUsingDecl(UsingDecl *UD) {
for (const auto *Shadow : UD->shadows()) {
auto *TD = Shadow->getTargetDecl();
- auto IsUsed = TD->isUsed() || TD->isReferenced();
+ auto IsUsed = TD->isUsed() || TD->isReferenced() || !TD->getAsFunction();
report(UD->getLocation(), TD,
IsUsed ? RefType::Explicit : RefType::Ambiguous);
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 6c8eacbff1cea3..9286758cab081c 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -255,7 +255,7 @@ TEST(WalkAST, TemplateSpecializationsFromUsingDecl) {
// Class templates
testWalk(R"cpp(
namespace ns {
-template class $ambiguous^Z {}; // primary template
+template class $explicit^Z {}; // primary template
template class $ambiguous^Z {}; // partial specialization
template<> class $ambiguous^Z {};// full specialization
}
@@ -265,7 +265,7 @@ template<> class $ambiguous^Z {};// full
specialization
// Var templates
testWalk(R"cpp(
namespace ns {
-template T $ambiguous^foo; // primary template
+template T $explicit^foo; // primary template
template T $ambiguous^foo; // partial specialization
template<> int* $ambiguous^foo; // full specialization
}
@@ -335,7 +335,12 @@ TEST(WalkAST, Using) {
testWalk(R"cpp(
namespace ns {
template
- class $ambiguous^Y {};
+ class $explicit^Y {};
+})cpp",
+ "using ns::^Y;");
+ testWalk(R"cpp(
+namespace ns {
+ class $explicit^Y {};
})cpp",
"using ns::^Y;");
testWalk(R"cpp(
___
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 collect macros when clang-tidy checks call into the preprocessor (PR #106329)
https://github.com/kadircet edited https://github.com/llvm/llvm-project/pull/106329 ___ 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 collect macros when clang-tidy checks call into the preprocessor (PR #106329)
@@ -702,6 +704,10 @@ ParsedAST::build(llvm::StringRef Filename, const
ParseInputs &Inputs,
log("Execute() failed when building AST for {0}: {1}", MainInput.getFile(),
toString(std::move(Err)));
+ // Disable the macro collector for the remainder of this function, e.g.
+ // clang-tidy checkers.
+ MacroCollectorPtr->doneParse();
kadircet wrote:
thanks it's unfortunate but i guess makes sense.
in theory, we can have similar issues with all the others PPCallbcks that we
installed. Moreover if others were to use clang-tidy as a library, they'll run
into similar pitfalls. As the flow is:
- Inject PPCallbacks
- Build AST
- Pass AST to clang-ast-consumer (in this case clang-tidy)
- Invoke EndOfMainFile
So there might be some value in injecting an extra callback between `Build AST`
and `Pass AST to consumer`. We could properly reset our PPCallbacks to
recognize leaving main file for such situations. It's unfortunate that both
`FileChanged` and `LexedFileChanged` are designed to operate with a contract
that hints "new file/location" will be valid. It makes such a semantic possibly
breaking. Leaving that idea here in case you want to follow up on that (I'd be
happy to review), but I can see that it's much more involved, possibly without
anything breaking (and if it does we can always ask people to turn that check
off until we fix the issue).
https://github.com/llvm/llvm-project/pull/106329
___
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 collect macros when clang-tidy checks call into the preprocessor (PR #106329)
https://github.com/kadircet approved this pull request. thanks, lgtm! https://github.com/llvm/llvm-project/pull/106329 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Update TidyFastChecks for release/19.x (PR #106354)
https://github.com/kadircet updated https://github.com/llvm/llvm-project/pull/106354 From c2248b8f0b6255774c7cf2aa80e7330696fd9c40 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Wed, 28 Aug 2024 11:27:47 +0200 Subject: [PATCH] [clangd] Update TidyFastChecks for release/19.x Run for clang-tidy checks available in release/19.x branch. Some notable findings: - altera-id-dependent-backward-branch, stays slow with 13%. - misc-const-correctness become faster, going from 261% to 67%, but still above 8% threshold. - misc-header-include-cycle is a new SLOW check with 10% runtime implications - readability-container-size-empty went from 16% to 13%, still SLOW. --- clang-tools-extra/clangd/TidyFastChecks.inc | 669 +++- 1 file changed, 367 insertions(+), 302 deletions(-) diff --git a/clang-tools-extra/clangd/TidyFastChecks.inc b/clang-tools-extra/clangd/TidyFastChecks.inc index 9050ce16127ff4..de1a025602fa9c 100644 --- a/clang-tools-extra/clangd/TidyFastChecks.inc +++ b/clang-tools-extra/clangd/TidyFastChecks.inc @@ -7,370 +7,435 @@ #define SLOW(CHECK, DELTA) #endif -FAST(abseil-cleanup-ctad, -1.0) +FAST(abseil-cleanup-ctad, -2.0) FAST(abseil-duration-addition, 0.0) -FAST(abseil-duration-comparison, 1.0) -FAST(abseil-duration-conversion-cast, 3.0) -FAST(abseil-duration-division, -0.0) -FAST(abseil-duration-factory-float, 1.0) -FAST(abseil-duration-factory-scale, -0.0) -FAST(abseil-duration-subtraction, 1.0) -FAST(abseil-duration-unnecessary-conversion, 4.0) -FAST(abseil-faster-strsplit-delimiter, 2.0) -FAST(abseil-no-internal-dependencies, -1.0) -FAST(abseil-no-namespace, -1.0) -FAST(abseil-redundant-strcat-calls, 2.0) -FAST(abseil-str-cat-append, 1.0) -FAST(abseil-string-find-startswith, 1.0) -FAST(abseil-string-find-str-contains, 1.0) -FAST(abseil-time-comparison, -0.0) -FAST(abseil-time-subtraction, 0.0) +FAST(abseil-duration-comparison, -1.0) +FAST(abseil-duration-conversion-cast, -1.0) +FAST(abseil-duration-division, 0.0) +FAST(abseil-duration-factory-float, 2.0) +FAST(abseil-duration-factory-scale, 1.0) +FAST(abseil-duration-subtraction, -1.0) +FAST(abseil-duration-unnecessary-conversion, -0.0) +FAST(abseil-faster-strsplit-delimiter, 3.0) +FAST(abseil-no-internal-dependencies, 1.0) +FAST(abseil-no-namespace, -0.0) +FAST(abseil-redundant-strcat-calls, 1.0) +FAST(abseil-str-cat-append, -0.0) +FAST(abseil-string-find-startswith, -1.0) +FAST(abseil-string-find-str-contains, 4.0) +FAST(abseil-time-comparison, -1.0) +FAST(abseil-time-subtraction, 1.0) FAST(abseil-upgrade-duration-conversions, 2.0) SLOW(altera-id-dependent-backward-branch, 13.0) -FAST(altera-kernel-name-restriction, -1.0) -FAST(altera-single-work-item-barrier, -1.0) -FAST(altera-struct-pack-align, -1.0) +FAST(altera-kernel-name-restriction, 4.0) +FAST(altera-single-work-item-barrier, 1.0) +FAST(altera-struct-pack-align, -0.0) FAST(altera-unroll-loops, 2.0) -FAST(android-cloexec-accept, -1.0) -FAST(android-cloexec-accept4, 3.0) -FAST(android-cloexec-creat, 0.0) -FAST(android-cloexec-dup, 3.0) -FAST(android-cloexec-epoll-create, -2.0) -FAST(android-cloexec-epoll-create1, -1.0) -FAST(android-cloexec-fopen, -0.0) -FAST(android-cloexec-inotify-init, 1.0) -FAST(android-cloexec-inotify-init1, 2.0) -FAST(android-cloexec-memfd-create, 2.0) -FAST(android-cloexec-open, -1.0) -FAST(android-cloexec-pipe, -1.0) +FAST(android-cloexec-accept, 0.0) +FAST(android-cloexec-accept4, 1.0) +FAST(android-cloexec-creat, 1.0) +FAST(android-cloexec-dup, 0.0) +FAST(android-cloexec-epoll-create, 2.0) +FAST(android-cloexec-epoll-create1, 0.0) +FAST(android-cloexec-fopen, -1.0) +FAST(android-cloexec-inotify-init, 2.0) +FAST(android-cloexec-inotify-init1, -0.0) +FAST(android-cloexec-memfd-create, -1.0) +FAST(android-cloexec-open, 1.0) +FAST(android-cloexec-pipe, -0.0) FAST(android-cloexec-pipe2, 0.0) FAST(android-cloexec-socket, 1.0) -FAST(android-comparison-in-temp-failure-retry, 0.0) -FAST(boost-use-to-string, 1.0) -FAST(bugprone-argument-comment, 2.0) +FAST(android-comparison-in-temp-failure-retry, 1.0) +FAST(boost-use-ranges, 2.0) +FAST(boost-use-to-string, 2.0) +FAST(bugprone-argument-comment, 4.0) FAST(bugprone-assert-side-effect, 1.0) -FAST(bugprone-assignment-in-if-condition, -0.0) -FAST(bugprone-bad-signal-to-kill-thread, -1.0) +FAST(bugprone-assignment-in-if-condition, 2.0) +FAST(bugprone-bad-signal-to-kill-thread, 1.0) FAST(bugprone-bool-pointer-implicit-conversion, 0.0) -FAST(bugprone-branch-clone, -0.0) +FAST(bugprone-branch-clone, 1.0) +FAST(bugprone-casting-through-void, 1.0) +FAST(bugprone-chained-comparison, 1.0) +FAST(bugprone-compare-pointer-to-member-virtual-function, -0.0) FAST(bugprone-copy-constructor-init, 1.0) -FAST(bugprone-dangling-handle, 0.0) -FAST(bugprone-dynamic-static-initializers, 1.0) +FAST(bugprone-crtp-constructor-accessibility, 0.0) +FAST(bugprone-dangling-handle, -0.0) +FAST(bugprone-dynamic-static-initializers, 0.0) FAST(bugprone-easily-swappable-parameters, 2.0) -FAST(bugprone-exception-escape, 1
[clang-tools-extra] [clangd] Update TidyFastChecks for release/19.x (PR #106354)
https://github.com/kadircet created https://github.com/llvm/llvm-project/pull/106354 Run for clang-tidy checks available in release/19.x branch. Some notable findings: - altera-id-dependent-backward-branch, stays slow with 13%. - misc-const-correctness become faster, going from 261% to 67%, but still above 8% threshold. - misc-header-include-cycle is a new SLOW check with 10% runtime implications - readability-container-size-empty went from 16% to 13%, still SLOW. From 38df7489ff2fa15a5bc7fc66d18ef78b489cd9d8 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Wed, 28 Aug 2024 11:27:47 +0200 Subject: [PATCH] [clangd] Update TidyFastChecks for release/19.x --- clang-tools-extra/clangd/TidyFastChecks.inc | 670 +++- 1 file changed, 368 insertions(+), 302 deletions(-) diff --git a/clang-tools-extra/clangd/TidyFastChecks.inc b/clang-tools-extra/clangd/TidyFastChecks.inc index 9050ce16127ff4..b1e1bf05172daa 100644 --- a/clang-tools-extra/clangd/TidyFastChecks.inc +++ b/clang-tools-extra/clangd/TidyFastChecks.inc @@ -7,371 +7,437 @@ #define SLOW(CHECK, DELTA) #endif -FAST(abseil-cleanup-ctad, -1.0) +FAST(abseil-cleanup-ctad, -2.0) FAST(abseil-duration-addition, 0.0) -FAST(abseil-duration-comparison, 1.0) -FAST(abseil-duration-conversion-cast, 3.0) -FAST(abseil-duration-division, -0.0) -FAST(abseil-duration-factory-float, 1.0) -FAST(abseil-duration-factory-scale, -0.0) -FAST(abseil-duration-subtraction, 1.0) -FAST(abseil-duration-unnecessary-conversion, 4.0) -FAST(abseil-faster-strsplit-delimiter, 2.0) -FAST(abseil-no-internal-dependencies, -1.0) -FAST(abseil-no-namespace, -1.0) -FAST(abseil-redundant-strcat-calls, 2.0) -FAST(abseil-str-cat-append, 1.0) -FAST(abseil-string-find-startswith, 1.0) -FAST(abseil-string-find-str-contains, 1.0) -FAST(abseil-time-comparison, -0.0) -FAST(abseil-time-subtraction, 0.0) +FAST(abseil-duration-comparison, -1.0) +FAST(abseil-duration-conversion-cast, -1.0) +FAST(abseil-duration-division, 0.0) +FAST(abseil-duration-factory-float, 2.0) +FAST(abseil-duration-factory-scale, 1.0) +FAST(abseil-duration-subtraction, -1.0) +FAST(abseil-duration-unnecessary-conversion, -0.0) +FAST(abseil-faster-strsplit-delimiter, 3.0) +FAST(abseil-no-internal-dependencies, 1.0) +FAST(abseil-no-namespace, -0.0) +FAST(abseil-redundant-strcat-calls, 1.0) +FAST(abseil-str-cat-append, -0.0) +FAST(abseil-string-find-startswith, -1.0) +FAST(abseil-string-find-str-contains, 4.0) +FAST(abseil-time-comparison, -1.0) +FAST(abseil-time-subtraction, 1.0) FAST(abseil-upgrade-duration-conversions, 2.0) SLOW(altera-id-dependent-backward-branch, 13.0) -FAST(altera-kernel-name-restriction, -1.0) -FAST(altera-single-work-item-barrier, -1.0) -FAST(altera-struct-pack-align, -1.0) +FAST(altera-kernel-name-restriction, 4.0) +FAST(altera-single-work-item-barrier, 1.0) +FAST(altera-struct-pack-align, -0.0) FAST(altera-unroll-loops, 2.0) -FAST(android-cloexec-accept, -1.0) -FAST(android-cloexec-accept4, 3.0) -FAST(android-cloexec-creat, 0.0) -FAST(android-cloexec-dup, 3.0) -FAST(android-cloexec-epoll-create, -2.0) -FAST(android-cloexec-epoll-create1, -1.0) -FAST(android-cloexec-fopen, -0.0) -FAST(android-cloexec-inotify-init, 1.0) -FAST(android-cloexec-inotify-init1, 2.0) -FAST(android-cloexec-memfd-create, 2.0) -FAST(android-cloexec-open, -1.0) -FAST(android-cloexec-pipe, -1.0) +FAST(android-cloexec-accept, 0.0) +FAST(android-cloexec-accept4, 1.0) +FAST(android-cloexec-creat, 1.0) +FAST(android-cloexec-dup, 0.0) +FAST(android-cloexec-epoll-create, 2.0) +FAST(android-cloexec-epoll-create1, 0.0) +FAST(android-cloexec-fopen, -1.0) +FAST(android-cloexec-inotify-init, 2.0) +FAST(android-cloexec-inotify-init1, -0.0) +FAST(android-cloexec-memfd-create, -1.0) +FAST(android-cloexec-open, 1.0) +FAST(android-cloexec-pipe, -0.0) FAST(android-cloexec-pipe2, 0.0) FAST(android-cloexec-socket, 1.0) -FAST(android-comparison-in-temp-failure-retry, 0.0) -FAST(boost-use-to-string, 1.0) -FAST(bugprone-argument-comment, 2.0) +FAST(android-comparison-in-temp-failure-retry, 1.0) +FAST(boost-use-ranges, 2.0) +FAST(boost-use-to-string, 2.0) +FAST(bugprone-argument-comment, 4.0) FAST(bugprone-assert-side-effect, 1.0) -FAST(bugprone-assignment-in-if-condition, -0.0) -FAST(bugprone-bad-signal-to-kill-thread, -1.0) +FAST(bugprone-assignment-in-if-condition, 2.0) +FAST(bugprone-bad-signal-to-kill-thread, 1.0) FAST(bugprone-bool-pointer-implicit-conversion, 0.0) -FAST(bugprone-branch-clone, -0.0) +FAST(bugprone-branch-clone, 1.0) +FAST(bugprone-casting-through-void, 1.0) +FAST(bugprone-chained-comparison, 1.0) +FAST(bugprone-compare-pointer-to-member-virtual-function, -0.0) FAST(bugprone-copy-constructor-init, 1.0) -FAST(bugprone-dangling-handle, 0.0) -FAST(bugprone-dynamic-static-initializers, 1.0) +FAST(bugprone-crtp-constructor-accessibility, 0.0) +FAST(bugprone-dangling-handle, -0.0) +FAST(bugprone-dynamic-static-initializers, 0.0) FAST(bugprone-easily-swappable-parameters, 2.0) -FAST(bugprone-exception-escape,
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/106241
CompilerInstance can re-use same SourceManager across multiple
frontendactions. During this process it calls
`SourceManager::clearIDTables` to reset any caches based on FileIDs.
It didn't reset IncludeLocMap, resulting in wrong include locations for
workflows that triggered multiple frontend-actions through same
CompilerInstance.
From ab986c7d33e45638db8d53ef128e617ce244f322 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya
Date: Tue, 27 Aug 2024 17:56:47 +0200
Subject: [PATCH] [clang] Cleanup IncludeLocMap
CompilerInstance can re-use same SourceManager across multiple
frontendactions. During this process it calls
`SourceManager::clearIDTables` to reset any caches based on FileIDs.
It didn't reset IncludeLocMap, resulting in wrong include locations for
workflows that triggered multiple frontend-actions through same
CompilerInstance.
---
clang/lib/Basic/SourceManager.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/clang/lib/Basic/SourceManager.cpp
b/clang/lib/Basic/SourceManager.cpp
index b0256a8ce9ed04..d6ec26af80aadd 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -350,6 +350,7 @@ void SourceManager::clearIDTables() {
LastLineNoContentCache = nullptr;
LastFileIDLookup = FileID();
+ IncludedLocMap.clear();
if (LineTable)
LineTable->clear();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Treat new expressions as simple functions (PR #105168)
https://github.com/kadircet closed https://github.com/llvm/llvm-project/pull/105168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
