[PATCH] D66751: [clangd] Add targetDecl(), which determines what declaration an AST node refers to.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

> Concretely, can you give an example of which node and what you'd want it to 
> point to? If it's just the single unqualified-name token, I agree and would 
> like to add it (as a separate patch). If it's the full range, I think that's 
> getSourceRange(). Is it something else?

Yes I was talking about for the single unqualified name.




Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:49
+EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty()) << Code;
+llvm::Annotations::Range R = A.llvm::Annotations::range();
+SelectionTree Selection(AST.getASTContext(), AST.getTokens(), R.Begin,

sammccall wrote:
> kadircet wrote:
> > can't you just call `A.range()` ? :D
> > I hope it wasn't clangd that somehow code-completed this.
> A.range() is an LSP Location (line/col pairs), offsets seem slightly more 
> convenient here.
> 
> This is ugly though, using Annotations with SourceLocations seems 
> unneccesarily clunky. Ideas?
> 
> (this reminds me, these test helpers need comments, added some)
ah sorry somehow thought this was already an `llvm::Annotations`. You seem to 
be only using `Code` and `Base::range` and they are both coming from 
`llvm::Annotations` already. Any reason for not using it directly instead of 
`clangd::Annotations` ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66751/new/

https://reviews.llvm.org/D66751



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66751: [clangd] Add targetDecl(), which determines what declaration an AST node refers to.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 217576.
sammccall marked an inline comment as done.
sammccall added a comment.

use llvm::Annotations


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66751/new/

https://reviews.llvm.org/D66751

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/FindTarget.h
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -0,0 +1,468 @@
+//===-- FindSymbolsTests.cpp -*- C++ -*===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "FindTarget.h"
+
+#include "Selection.h"
+#include "TestTU.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Testing/Support/Annotations.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+
+// A referenced Decl together with its DeclRelationSet, for assertions.
+//
+// There's no great way to assert on the "content" of a Decl in the general case
+// that's both expressive and unambiguous (e.g. clearly distinguishes between
+// templated decls and their specializations).
+//
+// We use the result of pretty-printing the decl, with the {body} truncated.
+struct PrintedDecl {
+  PrintedDecl(const char *Name, DeclRelationSet Relations = {})
+  : Name(Name), Relations(Relations) {}
+  PrintedDecl(const Decl *D, DeclRelationSet Relations = {})
+  : Relations(Relations) {
+std::string S;
+llvm::raw_string_ostream OS(S);
+D->print(OS);
+llvm::StringRef FirstLine =
+llvm::StringRef(OS.str()).take_until([](char C) { return C == '\n'; });
+FirstLine = FirstLine.rtrim(" {");
+Name = FirstLine.rtrim(" {");
+  }
+
+  std::string Name;
+  DeclRelationSet Relations;
+};
+bool operator==(const PrintedDecl &L, const PrintedDecl &R) {
+  return std::tie(L.Name, L.Relations) == std::tie(R.Name, R.Relations);
+}
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PrintedDecl &D) {
+  return OS << D.Name << " Rel=" << D.Relations;
+}
+
+// The test cases in for targetDecl() take the form
+//  - a piece of code (Code = "...")
+//  - Code should have a single AST node marked as a [[range]]
+//  - an EXPECT_DECLS() assertion that verify the type of node selected, and
+//all the decls that targetDecl() considers it to reference
+// Despite the name, these cases actually test allTargetDecls() for brevity.
+class TargetDeclTest : public ::testing::Test {
+protected:
+  using Rel = DeclRelation;
+  std::string Code;
+  std::vector Flags;
+
+  // Asserts that `Code` has a marked selection of a node `NodeType`,
+  // and returns allTargetDecls() as PrintedDecl structs.
+  // Use via EXPECT_DECLS().
+  std::vector assertNodeAndPrintDecls(const char *NodeType) {
+llvm::Annotations A(Code);
+auto TU = TestTU::withCode(A.code());
+TU.ExtraArgs = Flags;
+auto AST = TU.build();
+EXPECT_THAT(AST.getDiagnostics(), ::testing::IsEmpty()) << Code;
+llvm::Annotations::Range R = A.range();
+SelectionTree Selection(AST.getASTContext(), AST.getTokens(), R.Begin,
+R.End);
+const SelectionTree::Node *N = Selection.commonAncestor();
+if (!N) {
+  ADD_FAILURE() << "No node selected!\n" << Code;
+  return {};
+}
+EXPECT_EQ(N->kind(), NodeType) << Selection;
+
+std::vector ActualDecls;
+for (const auto &Entry : allTargetDecls(N->ASTNode))
+  ActualDecls.emplace_back(Entry.first, Entry.second);
+return ActualDecls;
+  }
+};
+
+// This is a macro to preserve line numbers in assertion failures.
+// It takes the expected decls as varargs to work around comma-in-macro issues.
+#define EXPECT_DECLS(NodeType, ...)\
+  EXPECT_THAT(assertNodeAndPrintDecls(NodeType),   \
+  ::testing::UnorderedElementsAreArray(\
+  std::vector({__VA_ARGS__})))\
+  << Code
+using ExpectedDecls = std::vector;
+
+TEST_F(TargetDeclTest, Exprs) {
+  Code = R"cpp(
+int f();
+int x = [[f]]();
+  )cpp";
+  EXPECT_DECLS("DeclRefExpr", "int f()");
+
+  Code = R"cpp(
+struct S { S operator+(S) const; };
+auto X = S() [[+]] S();
+  )cpp";
+  EXPECT_DECLS("DeclRefExpr", "S operator+(S) const");
+}
+
+TEST_F(TargetDeclTest, UsingDecl) {
+  Code = R"cpp(
+

[PATCH] D66765: [analyzer] (Urgent!) Add 9.0.0. release notes.

[Hans Wennborg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370166: [analyzer] Add 9.0.0. release notes. (authored by 
hans, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66765?vs=217564&id=217577#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66765/new/

https://reviews.llvm.org/D66765

Files:
  cfe/branches/release_90/docs/ClangStaticAnalyzer.rst
  cfe/branches/release_90/docs/ReleaseNotes.rst


Index: cfe/branches/release_90/docs/ReleaseNotes.rst
===
--- cfe/branches/release_90/docs/ReleaseNotes.rst
+++ cfe/branches/release_90/docs/ReleaseNotes.rst
@@ -325,10 +325,57 @@
 Static Analyzer
 ---
 
+- Fixed a bug where an incorrect checker name would be displayed for a bug
+  report.`
+
+- New checker: 'security.insecureAPI.DeprecatedOrUnsafeBufferHandling' to 
detect
+  uses of unsafe/deprecated buffer handling functions for C code using the C11
+  standard or newer.
+
+- New checker: 'osx.MIGChecker' to find violations of the Mach Interface
+  Generator calling convention
+
+- New checker: 'optin.osx.OSObjectCStyleCast' to find C-style casts of of XNU
+  libkern OSObjects
+
+- New package: 'apiModeling.llvm' contains modeling checkers to improve the
+  accuracy of reports on LLVM's own codebase.
+
+- The Static Analyzer received a
+  :ref:`developer documentation `.
+
 - The UninitializedObject checker is now considered as stable.
   (moved from the 'alpha.cplusplus' to the 'optin.cplusplus' package)
 
-...
+- New frontend flags: The list of available checkers are now split into 3
+  different frontend flags:
+
+  - ``-analyzer-checker-help``: The list of user-facing, stable checkers.
+
+  - ``-analyzer-checker-help-alpha``: The list of in-development
+checkers not yet advised to be turned on.
+
+  - ``-analyzer-checker-help-developer``: Checkers never meant to be
+enabled/disabled by hand + development checkers.
+
+- New frontend flags: While they have always been around, for the first time,
+  checker and package options are listable:
+
+  - ``-analyzer-checker-option-help``: The list of user-facing, stable checker
+and package options.
+
+  - ``-analyzer-checker-option-help-alpha``: The list of in-development checker
+options not yet advised to be used.
+
+  - ``-analyzer-checker-option-help-developer``: Options never meant to be
+enabled/disabled by hand + development options.
+
+- New frontend flag: ``-analyzer-werror`` to turn analyzer warnings into 
errors.
+
+- Numerous fixes to increase the stability of the experimental cross 
translation
+  unit analysis (CTU).
+
+- CTU now handles virtual functions as well.
 
 .. _release-notes-ubsan:
 
Index: cfe/branches/release_90/docs/ClangStaticAnalyzer.rst
===
--- cfe/branches/release_90/docs/ClangStaticAnalyzer.rst
+++ cfe/branches/release_90/docs/ClangStaticAnalyzer.rst
@@ -2,6 +2,8 @@
 Clang Static Analyzer
 =
 
+.. _clang-static-analyzer-docs:
+
 The Clang Static Analyzer is a source code analysis tool that finds bugs in C, 
C++, and Objective-C programs.
 It implements *path-sensitive*, *inter-procedural analysis* based on *symbolic 
execution* technique.
 


Index: cfe/branches/release_90/docs/ReleaseNotes.rst
===
--- cfe/branches/release_90/docs/ReleaseNotes.rst
+++ cfe/branches/release_90/docs/ReleaseNotes.rst
@@ -325,10 +325,57 @@
 Static Analyzer
 ---
 
+- Fixed a bug where an incorrect checker name would be displayed for a bug
+  report.`
+
+- New checker: 'security.insecureAPI.DeprecatedOrUnsafeBufferHandling' to detect
+  uses of unsafe/deprecated buffer handling functions for C code using the C11
+  standard or newer.
+
+- New checker: 'osx.MIGChecker' to find violations of the Mach Interface
+  Generator calling convention
+
+- New checker: 'optin.osx.OSObjectCStyleCast' to find C-style casts of of XNU
+  libkern OSObjects
+
+- New package: 'apiModeling.llvm' contains modeling checkers to improve the
+  accuracy of reports on LLVM's own codebase.
+
+- The Static Analyzer received a
+  :ref:`developer documentation `.
+
 - The UninitializedObject checker is now considered as stable.
   (moved from the 'alpha.cplusplus' to the 'optin.cplusplus' package)
 
-...
+- New frontend flags: The list of available checkers are now split into 3
+  different frontend flags:
+
+  - ``-analyzer-checker-help``: The list of user-facing, stable checkers.
+
+  - ``-analyzer-checker-help-alpha``: The list of in-development
+checkers not yet advised to be turned on.
+
+  - ``-analyzer-checker-help-developer``: Checkers never meant to be
+enabled/disabled by hand + development checkers.
+
+- New frontend flags: While they have always been around

[PATCH] D66627: [clang-tidy] add checks to bugprone-posix-return

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:70
 
+- New :doc:`bugprone-posix-return
+  ` check.

jcai19 wrote:
> Eugene.Zelenko wrote:
> > Check is not new, just modified. Such check should be after new checks.
> Is Modified the right keyword for modification? Thanks
I'd say "improved" instead of modified.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66627/new/

https://reviews.llvm.org/D66627



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66759: [clangd] Surface errors from command-line parsing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 217579.
ilya-biryukov marked 2 inline comments as done and an inline comment as not 
done.
ilya-biryukov added a comment.

- Do not flushDiag() on EndSourceFile
- Do not reset WasAdjusted outside flusLastDiag()
- Add a test that unknown warnings do not produce diagnostics


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66759/new/

https://reviews.llvm.org/D66759

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/ClangdUnit.h
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Compiler.cpp
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c

Index: clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
===
--- clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
+++ clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
@@ -1,16 +1,16 @@
-// Test CF+LF are properly handled along with quoted, multi-line #error
-// RUN: %clang_cc1 -DOTHER -print-dependency-directives-minimized-source %s 2>&1 | FileCheck %s
-
-#ifndef TEST
-#error "message \
-   more message \
-   even more"
-#endif
-
-#ifdef OTHER
-#include 
-#endif
-
-// CHECK:  #ifdef OTHER
-// CHECK-NEXT: #include 
-// CHECK-NEXT: #endif
+// Test CF+LF are properly handled along with quoted, multi-line #error
+// RUN: %clang_cc1 -DOTHER -print-dependency-directives-minimized-source %s 2>&1 | FileCheck %s
+
+#ifndef TEST
+#error "message \
+   more message \
+   even more"
+#endif
+
+#ifdef OTHER
+#include 
+#endif
+
+// CHECK:  #ifdef OTHER
+// CHECK-NEXT: #include 
+// CHECK-NEXT: #endif
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -7,6 +7,8 @@
 //===--===//
 
 #include "TestTU.h"
+#include "Compiler.h"
+#include "Diagnostics.h"
 #include "TestFS.h"
 #include "index/FileIndex.h"
 #include "index/MemIndex.h"
@@ -59,14 +61,16 @@
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)
 Inputs.Opts.SuggestMissingIncludes = true;
-  auto CI = buildCompilerInvocation(Inputs);
+  StoreDiags Diags;
+  auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
   buildPreamble(FullFilename, *CI,
 /*OldPreamble=*/nullptr,
 /*OldCompileCommand=*/Inputs.CompileCommand, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
-  auto AST = buildAST(FullFilename, std::move(CI), Inputs, Preamble);
+  auto AST =
+  buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
   if (!AST.hasValue()) {
 ADD_FAILURE() << "Failed to build code:\n" << Code;
 llvm_unreachable("Failed to build TestTU!");
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -14,6 +14,9 @@
 #include "Path.h"
 #include "TUScheduler.h"
 #include "TestFS.h"
+#include "Threading.h"
+#include "clang/Basic/DiagnosticDriver.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "gmock/gmock.h"
@@ -28,6 +31,9 @@
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
+using ::testing::Eq;
+using ::testing::Field;
+using ::testing::IsEmpty;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
 
@@ -60,12 +66,22 @@
   /// in updateWithDiags.
   static std::unique_ptr captureDiags() {
 class CaptureDiags : public ParsingCallbacks {
+public:
   void onMainAST(PathRef File, ParsedAST &AST, PublishFn Publish) override {
-auto Diags = AST.getDiagnostics();
+reportDiagnostics(File, AST.getDiagnostics(), Publish);
+  }
+
+  void onFailedAST(PathRef File, std::vector Diags,
+   PublishFn Publish) override {
+reportDiagnostics(File, Diags, Publish);
+  }
+
+private:
+  void reportDiagnostics(PathRef File, llvm::ArrayRef Diags,
+  

[PATCH] D66759: [clangd] Surface errors from command-line parsing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:465
+Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end());
+  // Finally, add diagnostics coming from the AST.
+  {

kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > could you preserve the previous order by inserting 
> > > `CompilerInvocationDiags` here instead of the ones in `AST` ?
> > Do you think we should add `CompilerInvocatioDiags` at the end, rather than 
> > at the start of the diagnostics list?
> > Why would that be better?
> > 
> > Mostly trying to keep the chronological order here: command-line 
> > diagnostics came first, followed by preamble, followed by AST diagnostics.
> I totally agree that your current ordering makes more sense. I just wasn't 
> sure why it was done in the opposite way before(first AST, then preamble), if 
> it doesn't cause any test breakages we should be good though.
The intention was to do less copies: normally there are less diagnostics in 
preamble than in the main file, so we would add the main file diagnostics first 
and later **prepend** the preamble diagnostics.

Performance-wise it does not matter and the new version reads better as it only 
appends and the order of items is natural.



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:399
+  // Do not forget to emit a pending diagnostic if there is one.
+  flushLastDiag();
+

kadircet wrote:
> I suppose we can get rid of the one in `StoreDiags::EndSourceFile` now ?
I've removed the call to `flushDiag()` from the function, but kept the function 
to reset the `LangOpts` to `None` for symmetry with setting them on 
`BeginSourceFile`.
Don't think this matters in practice, looks a bit better aesthetically (we 
"clean up" when the file goes out of scope).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66759/new/

https://reviews.llvm.org/D66759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66759: [clangd] Surface errors from command-line parsing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 217581.
ilya-biryukov added a comment.

- Remove accidental change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66759/new/

https://reviews.llvm.org/D66759

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/ClangdUnit.h
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Compiler.cpp
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -7,6 +7,8 @@
 //===--===//
 
 #include "TestTU.h"
+#include "Compiler.h"
+#include "Diagnostics.h"
 #include "TestFS.h"
 #include "index/FileIndex.h"
 #include "index/MemIndex.h"
@@ -59,14 +61,16 @@
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)
 Inputs.Opts.SuggestMissingIncludes = true;
-  auto CI = buildCompilerInvocation(Inputs);
+  StoreDiags Diags;
+  auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   auto Preamble =
   buildPreamble(FullFilename, *CI,
 /*OldPreamble=*/nullptr,
 /*OldCompileCommand=*/Inputs.CompileCommand, Inputs,
 /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr);
-  auto AST = buildAST(FullFilename, std::move(CI), Inputs, Preamble);
+  auto AST =
+  buildAST(FullFilename, std::move(CI), Diags.take(), Inputs, Preamble);
   if (!AST.hasValue()) {
 ADD_FAILURE() << "Failed to build code:\n" << Code;
 llvm_unreachable("Failed to build TestTU!");
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -14,6 +14,9 @@
 #include "Path.h"
 #include "TUScheduler.h"
 #include "TestFS.h"
+#include "Threading.h"
+#include "clang/Basic/DiagnosticDriver.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "gmock/gmock.h"
@@ -28,6 +31,9 @@
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
+using ::testing::Eq;
+using ::testing::Field;
+using ::testing::IsEmpty;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
 
@@ -60,12 +66,22 @@
   /// in updateWithDiags.
   static std::unique_ptr captureDiags() {
 class CaptureDiags : public ParsingCallbacks {
+public:
   void onMainAST(PathRef File, ParsedAST &AST, PublishFn Publish) override {
-auto Diags = AST.getDiagnostics();
+reportDiagnostics(File, AST.getDiagnostics(), Publish);
+  }
+
+  void onFailedAST(PathRef File, std::vector Diags,
+   PublishFn Publish) override {
+reportDiagnostics(File, Diags, Publish);
+  }
+
+private:
+  void reportDiagnostics(PathRef File, llvm::ArrayRef Diags,
+ PublishFn Publish) {
 auto D = Context::current().get(DiagsCallbackKey);
 if (!D)
   return;
-
 Publish([&]() {
   const_cast<
   llvm::unique_function)> &> (*D)(
@@ -720,6 +736,53 @@
   TUState(TUAction::Idle, /*No action*/ "")));
 }
 
+TEST_F(TUSchedulerTests, CommandLineErrors) {
+  // We should see errors from command-line parsing inside the main file.
+  CDB.ExtraClangFlags = {"-fsome-unknown-flag"};
+
+  TUScheduler S(CDB, /*AsyncThreadsCount=*/getDefaultAsyncThreadsCount(),
+/*StorePreambleInMemory=*/true, /*ASTCallbacks=*/captureDiags(),
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
+
+  Notification Ready;
+  std::vector Diagnostics;
+  updateWithDiags(S, testPath("foo.cpp"), "void test() {}",
+  WantDiagnostics::Yes, [&](std::vector D) {
+Diagnostics = std::move(D);
+Ready.notify();
+  });
+  Ready.wait();
+
+  EXPECT_THAT(
+  Diagnostics,
+  ElementsAre(AllOf(
+  Field(&Diag::ID, Eq(diag::err_drv_unknown_argument)),
+  Field(&Diag::Name, Eq("drv_unknown_argument")),
+  Field(&Diag::Message, "unknown argument: '-fsome-unknown

[PATCH] D66759: [clangd] Surface errors from command-line parsing

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66759/new/

https://reviews.llvm.org/D66759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: Avoid crash when dumping NULL Type as JSON

[Dmitry Sidorov via Phabricator via cfe-commits]

sidorovd added a comment.

LGTM. I'm not an expert in JSON, but may be it makes sense to move the change a 
line earlier before creation of pointer representation?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66864: [clang-tidy] readability-identifier-naming shouldn't complain about CRTP pseudo-overrides

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66864

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
  clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp


Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
@@ -262,6 +262,32 @@
   CMyWellNamedClass2 x5(42, nullptr);
 }
 
+class AOverridden {
+public:
+  virtual ~AOverridden() = default;
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual 
method 'BadBaseMethod'
+};
+
+class COverriding : public AOverridden {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+};
+
+template 
+class CRTPBase {
+public:
+  void BadBaseMethod(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 
'BadBaseMethod'
+};
+
+class CRTPDerived : CRTPBase {
+public:
+  // Hiding a badly-named base isn't a new violation.
+  double BadBaseMethod(double) { return 0; }
+};
+
 template
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type 
template parameter 'T'
 // CHECK-FIXES: {{^}}template{{$}}
Index: 
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -26,6 +26,10 @@
 different rules for different kinds of identifiers. In general, the rules are
 falling back to a more generic rule if the specific case is not configured.
 
+The naming of virtual methods is reported where they occur in the base class,
+but not where they are overridden, as it can't be fixed locally there.
+This also applies for pseudo-override patterns like CRTP.
+
 Options
 ---
 
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -10,6 +10,7 @@
 
 #include "../utils/ASTUtils.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
@@ -579,6 +580,15 @@
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 
+// If this method has the same name as any base method, this is likely
+// necessary even if it's not an override. e.g. CRTP.
+auto FindHidden = [&](const CXXBaseSpecifier *S, clang::CXXBasePath &P) {
+  return CXXRecordDecl::FindOrdinaryMember(S, P, Decl->getDeclName());
+};
+CXXBasePaths UnusedPaths;
+if (Decl->getParent()->lookupInBases(FindHidden, UnusedPaths))
+  return SK_Invalid;
+
 if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod])
   return SK_ConstexprMethod;
 


Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming.cpp
@@ -262,6 +262,32 @@
   CMyWellNamedClass2 x5(42, nullptr);
 }
 
+class AOverridden {
+public:
+  virtual ~AOverridden() = default;
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
+};
+
+class COverriding : public AOverridden {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+};
+
+template 
+class CRTPBase {
+public:
+  void BadBaseMethod(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 'BadBaseMethod'
+};
+
+class CRTPDerived : CRTPBase {
+public:
+  // Hiding a badly-named base isn't a new violation.
+  double BadBaseMethod(double) { return 0; }
+};
+
 template
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type template parameter 'T'
 // CHECK-FIXES: {{^}}template{{$}}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
+++ clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst
@@

[PATCH] D66828: [clangd] Add distinct highlightings for static fields and methods

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:208
 }
-if (isa(D)) {
-  addToken(Loc, HighlightingKind::Method);
+if (const CXXMethodDecl *MD = dyn_cast(D)) {
+  addToken(Loc, MD->isStatic() ? HighlightingKind::StaticMethod

nit: just use auto, the type is obvious from RHS


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66828/new/

https://reviews.llvm.org/D66828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Test coverage missing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r370175 - Delete minimize_source_to_dependency_directives_invalid_error.c

[Hans Wennborg via cfe-commits]

Author: hans
Date: Wed Aug 28 02:14:00 2019
New Revision: 370175

URL: http://llvm.org/viewvc/llvm-project?rev=370175&view=rev
Log:
Delete minimize_source_to_dependency_directives_invalid_error.c

It was added in r370129 with a .gitattributes file that means the file
always shows up as having a local diff in Git checkouts (at least on
Linux). Deleting it until we can figure out the right way to do this.

Removed:

cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
Modified:
cfe/trunk/.gitattributes

Modified: cfe/trunk/.gitattributes
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/.gitattributes?rev=370175&r1=370174&r2=370175&view=diff
==
--- cfe/trunk/.gitattributes (original)
+++ cfe/trunk/.gitattributes Wed Aug 28 02:14:00 2019
@@ -1,4 +1,3 @@
 # Windows line ending tests
 test/Lexer/minimize_source_to_dependency_directives_invalid_error.c text 
eol=crlf
 test/FixIt/fixit-newline-style.c text eol=crlf
-test/Frontend/system-header-line-directive-ms-lineendings.c text eol=crlf

Removed: 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c?rev=370174&view=auto
==
--- 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c 
(original)
+++ 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c 
(removed)
@@ -1,16 +0,0 @@
-// Test CF+LF are properly handled along with quoted, multi-line #error
-// RUN: %clang_cc1 -DOTHER -print-dependency-directives-minimized-source %s 
2>&1 | FileCheck %s
-
-#ifndef TEST
-#error "message \
-   more message \
-   even more"
-#endif
-
-#ifdef OTHER
-#include 
-#endif
-
-// CHECK:  #ifdef OTHER
-// CHECK-NEXT: #include 
-// CHECK-NEXT: #endif


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66556: [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D66556#1648118 , @dexonsmith wrote:

> In D66556#1648109 , @rnk wrote:
>
> > I'm not sure what happens, but I see you added .gitattributes. I'd commit 
> > it as is. Buildbots using svn will keep working. You can check that the 
> > monorepo has the right line endings afterwards, and try again if not.
>
>
> SGTM.


This broke users of the monorepo, where the file would show up as having 
changed locally, and with no way to reset it. I'm guessing that's because it 
was checked in with LF endings and then because of the .gitattributes file, it 
changes at checkout. I think the correct solution would be to check in the file 
with CRLF endings and not set any attributes or stuff. (Though this is all 
super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66556/new/

https://reviews.llvm.org/D66556



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r370177 - [clangd] Surface errors from command-line parsing

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Wed Aug 28 02:24:55 2019
New Revision: 370177

URL: http://llvm.org/viewvc/llvm-project?rev=370177&view=rev
Log:
[clangd] Surface errors from command-line parsing

Summary:
Those errors are exposed at the first character of a file,
for a lack of a better place.

Previously, all errors were stored inside the AST and report
accordingly. However, errors in command-line argument parsing could
result in failure to produce the AST, so we need an alternative ways to
report those errors.

We take the following approach in this patch:
  - buildCompilerInvocation() now requires an explicit DiagnosticConsumer.
  - TUScheduler and TestTU now collect the diagnostics produced when
parsing command line arguments.
If pasing of the AST failed, diagnostics are reported via a new
ParsingCallbacks::onFailedAST method.
If parsing of the AST succeeded, any errors produced during
command-line parsing are stored alongside the AST inside the
ParsedAST instance and reported as previously by calling the
ParsingCallbacks::onMainAST method;
  - The client code that uses ClangdServer's DiagnosticConsumer
does not need to change, it will receive new diagnostics in the
onDiagnosticsReady() callback

Errors produced when parsing command-line arguments are collected using
the same StoreDiags class that is used to collect all other errors. They
are recognized by their location being invalid. IIUC, the location is
invalid as there is no source manager at this point, it is created at a
later stage.

Although technically we might also get diagnostics that mention the
command-line arguments FileID with after the source manager was created
(and they have valid source locations), we choose to not handle those
and they are dropped as not coming from the main file. AFAICT, those
diagnostics should always be notes, therefore it's safe to drop them
without loosing too much information.

Reviewers: kadircet

Reviewed By: kadircet

Subscribers: nridge, javed.absar, MaskRay, jkorous, arphaman, cfe-commits, 
gribozavr

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66759

Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/Compiler.cpp
clang-tools-extra/trunk/clangd/Compiler.h
clang-tools-extra/trunk/clangd/Diagnostics.cpp
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
clang-tools-extra/trunk/clangd/unittests/FileIndexTests.cpp
clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp
clang-tools-extra/trunk/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/trunk/clangd/unittests/TestTU.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=370177&r1=370176&r2=370177&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Aug 28 02:24:55 2019
@@ -80,6 +80,11 @@ struct UpdateIndexCallbacks : public Par
 });
   }
 
+  void onFailedAST(PathRef Path, std::vector Diags,
+   PublishFn Publish) override {
+Publish([&]() { DiagConsumer.onDiagnosticsReady(Path, Diags); });
+  }
+
   void onFileUpdated(PathRef File, const TUStatus &Status) override {
 DiagConsumer.onFileUpdated(File, Status);
   }

Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=370177&r1=370176&r2=370177&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Wed Aug 28 02:24:55 2019
@@ -292,7 +292,8 @@ void dumpAST(ParsedAST &AST, llvm::raw_o
 }
 
 llvm::Optional
-ParsedAST::build(std::unique_ptr CI,
+ParsedAST::build(std::unique_ptr CI,
+ llvm::ArrayRef CompilerInvocationDiags,
  std::shared_ptr Preamble,
  std::unique_ptr Buffer,
  llvm::IntrusiveRefCntPtr VFS,
@@ -459,10 +460,15 @@ ParsedAST::build(std::unique_ptrgetPreprocessor().EndSourceFile();
 
-  std::vector Diags = ASTDiags.take(CTContext.getPointer());
+  std::vector Diags = CompilerInvocationDiags;
   // Add diagnostics from the preamble, if any.
   if (Preamble)
-Diags.insert(Diags.begin(), Preamble->Diags.begin(), 
Preamble->Diags.end());
+Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end());
+  // Finally, add diagnostics coming from the AST.
+  {
+std::

[PATCH] D66796: [clang] Loop pragma vectorize(disable)

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> Therefore, vectorize(disable) would also disable interleaving?

I don't have strong opinions on this. I think there's something to say for both 
options that we have (i.e. `vectorize(disable)` disables interleaving or 
enables it).
But I think it was @fhahn who mentioned that it would be possible to disable 
vectorisation, but still do interleaving. Perhaps bit of a edge use use, but 
again, it's possible currently. That would mean we don't need to disable 
interleaving here I think.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66796/new/

https://reviews.llvm.org/D66796



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66759: [clangd] Surface errors from command-line parsing

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370177: [clangd] Surface errors from command-line parsing 
(authored by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66759?vs=217581&id=217590#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66759/new/

https://reviews.llvm.org/D66759

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.h
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/Compiler.cpp
  clang-tools-extra/trunk/clangd/Compiler.h
  clang-tools-extra/trunk/clangd/Diagnostics.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.h
  clang-tools-extra/trunk/clangd/index/Background.cpp
  clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp
  clang-tools-extra/trunk/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/trunk/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/trunk/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp
===
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp
@@ -44,6 +44,7 @@
 #include "TUScheduler.h"
 #include "Cancellation.h"
 #include "Compiler.h"
+#include "Diagnostics.h"
 #include "GlobalCompilationDatabase.h"
 #include "Logger.h"
 #include "Trace.h"
@@ -365,6 +366,14 @@
 void ASTWorker::update(ParseInputs Inputs, WantDiagnostics WantDiags) {
   llvm::StringRef TaskName = "Update";
   auto Task = [=]() mutable {
+auto RunPublish = [&](llvm::function_ref Publish) {
+  // Ensure we only publish results from the worker if the file was not
+  // removed, making sure there are not race conditions.
+  std::lock_guard Lock(PublishMu);
+  if (CanPublishResults)
+Publish();
+};
+
 // Get the actual command as `Inputs` does not have a command.
 // FIXME: some build systems like Bazel will take time to preparing
 // environment to build the file, it would be nice if we could emit a
@@ -394,8 +403,11 @@
 Inputs.CompileCommand.Directory,
 llvm::join(Inputs.CompileCommand.CommandLine, " "));
 // Rebuild the preamble and the AST.
+StoreDiags CompilerInvocationDiagConsumer;
 std::unique_ptr Invocation =
-buildCompilerInvocation(Inputs);
+buildCompilerInvocation(Inputs, CompilerInvocationDiagConsumer);
+std::vector CompilerInvocationDiags =
+CompilerInvocationDiagConsumer.take();
 if (!Invocation) {
   elog("Could not build CompilerInvocation for file {0}", FileName);
   // Remove the old AST if it's still in cache.
@@ -403,6 +415,9 @@
   TUStatus::BuildDetails Details;
   Details.BuildFailed = true;
   emitTUStatus({TUAction::BuildingPreamble, TaskName}, &Details);
+  // Report the diagnostics we collected when parsing the command line.
+  Callbacks.onFailedAST(FileName, std::move(CompilerInvocationDiags),
+RunPublish);
   // Make sure anyone waiting for the preamble gets notified it could not
   // be built.
   PreambleWasBuilt.notify();
@@ -468,7 +483,8 @@
 llvm::Optional> AST = IdleASTs.take(this);
 if (!AST) {
   llvm::Optional NewAST =
-  buildAST(FileName, std::move(Invocation), Inputs, NewPreamble);
+  buildAST(FileName, std::move(Invocation), CompilerInvocationDiags,
+   Inputs, NewPreamble);
   AST = NewAST ? std::make_unique(std::move(*NewAST)) : nullptr;
   if (!(*AST)) { // buildAST fails.
 TUStatus::BuildDetails Details;
@@ -481,22 +497,22 @@
   Details.ReuseAST = true;
   emitTUStatus({TUAction::BuildingFile, TaskName}, &Details);
 }
+
 // We want to report the diagnostics even if this update was cancelled.
 // It seems more useful than making the clients wait indefinitely if they
 // spam us with updates.
 // Note *AST can still be null if buildAST fails.
 if (*AST) {
   trace::Span Span("Running main AST callback");
-  auto RunPublish = [&](llvm::function_ref Publish) {
-// Ensure we only publish results from the worker if the file was not
-// removed, making sure there are not race conditions.
-std::lock_guard Lock(PublishMu);
-if (CanPublishResults)
-  Publish();
-  };
 
   Callbacks.onMainAST(FileName, **AST, RunPublish);
   RanASTCallback = true;
+} else {
+  // Failed to build the AST, at least report diagnostics from the command
+  // line if there were any.
+  // FIXME: we might have got more errors while trying to build the AST,
+  //surface t

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177
   return;
+if (TP->isPointerType() || TP->isLValueReferenceType())
+  // When highlighting dependant template types the type can be a pointer 
or

ilya-biryukov wrote:
> jvikstrom wrote:
> > ilya-biryukov wrote:
> > > jvikstrom wrote:
> > > > ilya-biryukov wrote:
> > > > > jvikstrom wrote:
> > > > > > ilya-biryukov wrote:
> > > > > > > `RecursiveASTVisitor` also traverses the pointer and reference 
> > > > > > > types, why does it not reach the inner `TemplateTypeParmType` in 
> > > > > > > the cases you describe?
> > > > > > The D in `using D = ...` `typedef ... D` does not have a TypeLoc 
> > > > > > (at least not one that is visited). Therefore we use the 
> > > > > > VisitTypedefNameDecl (line 121) to get the location of `D` to be 
> > > > > > able to highlight it. And we just send the typeLocs typeptr to 
> > > > > > addType (which is a Pointer for `using D = T*;`)...
> > > > > > 
> > > > > > But maybe we should get the underlying type before we call addType 
> > > > > > with TypePtr? Just a while loop on line 123 basically (can we have 
> > > > > > multiple PointerTypes nested in each other actually?)
> > > > > > 
> > > > > > Even if we keep it in addType the comment is actually wrong, 
> > > > > > because it obviously works when for the actual "type occurrences" 
> > > > > > for `D` (so will fix that no matter what). This recursion will just 
> > > > > > make us add more duplicate tokens...
> > > > > Could we investigate why `RecursiveASTVisitor` does not visit the 
> > > > > `TypeLoc` of a corresponding decl?
> > > > > Here's the code from `RecursiveASTVisitor.h` that should do the trick:
> > > > > ```
> > > > > DEF_TRAVERSE_DECL(TypeAliasDecl, {
> > > > >   TRY_TO(TraverseTypeLoc(D->getTypeSourceInfo()->getTypeLoc()));
> > > > >   // We shouldn't traverse D->getTypeForDecl(); it's a result of
> > > > >   // declaring the type alias, not something that was written in the
> > > > >   // source.
> > > > > })
> > > > > ```
> > > > > 
> > > > > If it doesn't, we are probably holding it wrong.
> > > > There just doesn't seem to be a TypeLoc for the typedef'ed Decl.  We 
> > > > can get the `T*` TypeLoc (with `D->getTypeSourceInfo()->getTypeLoc()`). 
> > > > But there isn't one for `D`. Even the `D->getTypeForDecl` returns null.
> > > > 
> > > > And I have no idea where I'd even start debugging that. Or if it's even 
> > > > a bug
> > > > 
> > > I may have misinterpreted the patch. Are we trying to add highlightings 
> > > for the names of using aliases here? E.g. for the following range:
> > > ```
> > > template 
> > > struct Foo {
> > >   using [[D]] = T**;
> > > };
> > > ```
> > > 
> > > Why isn't this handled in `VisitNamedDecl`?
> > > We don't seem to call this function for `TypedefNameDecl` at all and it 
> > > actually weird. Is this because we attempt to highlight typedefs as their 
> > > underlying types?
> > So currently using aliases and typedefs are highlighted the same as the 
> > underlying type (in most cases). One case where they aren't is when the 
> > underlying type is a template parameter (which is what this patch is trying 
> > to solve).
> > 
> > 
> > > Why isn't this handled in VisitNamedDecl?
> > 
> > The Decl is actually visited in `VisitNamedDecl`, however as it is a 
> > `TypeAliasDecl` which we do not have a check for in the addToken function 
> > it will not get highlighted in that visit.
> > 
> > Actually, could add a check for `TypeAliasDecl` in `addToken` (should 
> > probably be a check for `TypedefNameDecl` to cover both `using ...` and 
> > `typedef ...`) and move the code from the `VisitTypedefNameDecl` to the 
> > `addToken` function inside that check instead.
> > 
> > 
> > 
> > > We don't seem to call this function for TypedefNameDecl at all and it 
> > > actually weird. Is this because we attempt to highlight typedefs as their 
> > > underlying types?
> > 
> > 
> > Don't understand what you mean. What function? 
> > So currently using aliases and typedefs are highlighted the same as the 
> > underlying type (in most cases). 
> Thanks for clarifying this. This is where my confusion is coming from.
> A few question to try understanding the approach taken (sorry if that's too 
> detailed, I am probably missing the context here)
> - What do we fallback to? From my reading of the code, we do not highlight 
> them at all if the underlying type is not one of the predefined cases.
> - Why are pointers and **l-value** references special? What about arrays, 
> r-value references, function types, pack expansions, etc.?
> 
> > Don't understand what you mean. What function?
> We don't call `VisitNamedDecl` from `VisitTypedefNameDecl`. I guess that's 
> intentional if we try to highlight them as underlying types.
Summarizing the offline discussion:
- we chose to highlight typedefs same as their underlying type (e.g. if it's a 
clas

Re: r370073 - [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 targets with atomics

[Hans Wennborg via cfe-commits]

Merged to release_90 in r370181.

On Tue, Aug 27, 2019 at 5:39 PM Sam Elliott via cfe-commits
 wrote:
>
> Author: lenary
> Date: Tue Aug 27 08:41:16 2019
> New Revision: 370073
>
> URL: http://llvm.org/viewvc/llvm-project?rev=370073&view=rev
> Log:
> [RISCV] Set MaxAtomicInlineWidth and MaxAtomicPromoteWidth for RV32/RV64 
> targets with atomics
>
> Summary: This ensures that libcalls aren't generated when the target supports 
> atomics. Atomics aren't in the base RV32I/RV64I instruction sets, so 
> MaxAtomicInlineWidth and MaxAtomicPromoteWidth are set only when the atomics 
> extension is being targeted. This must be done in setMaxAtomicWidth, as this 
> should be done after handleTargetFeatures has been called.
>
> Reviewers: jfb, jyknight, wmi, asb
>
> Reviewed By: asb
>
> Subscribers: pzheng, MaskRay, s.egerton, lenary, dexonsmith, psnobl, benna, 
> Jim, JohnLLVM, rbar, johnrusso, simoncook, apazos, sabuasal, niosHD, 
> kito-cheng, shiva0217, jrtc27, zzheng, edward-jones, rogfer01, MartinMosbeck, 
> brucehoult, the_o, rkruppe, PkmX, jocewei, lewis-revill, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D57450
>
> Added:
> cfe/trunk/test/CodeGen/riscv-atomics.c
> Modified:
> cfe/trunk/lib/Basic/Targets/RISCV.h
> cfe/trunk/test/Driver/riscv32-toolchain.c
> cfe/trunk/test/Driver/riscv64-toolchain.c
>
> Modified: cfe/trunk/lib/Basic/Targets/RISCV.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/RISCV.h?rev=370073&r1=370072&r2=370073&view=diff
> ==
> --- cfe/trunk/lib/Basic/Targets/RISCV.h (original)
> +++ cfe/trunk/lib/Basic/Targets/RISCV.h Tue Aug 27 08:41:16 2019
> @@ -93,6 +93,13 @@ public:
>  }
>  return false;
>}
> +
> +  void setMaxAtomicWidth() override {
> +MaxAtomicPromoteWidth = 128;
> +
> +if (HasA)
> +  MaxAtomicInlineWidth = 32;
> +  }
>  };
>  class LLVM_LIBRARY_VISIBILITY RISCV64TargetInfo : public RISCVTargetInfo {
>  public:
> @@ -110,6 +117,13 @@ public:
>  }
>  return false;
>}
> +
> +  void setMaxAtomicWidth() override {
> +MaxAtomicPromoteWidth = 128;
> +
> +if (HasA)
> +  MaxAtomicInlineWidth = 64;
> +  }
>  };
>  } // namespace targets
>  } // namespace clang
>
> Added: cfe/trunk/test/CodeGen/riscv-atomics.c
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/riscv-atomics.c?rev=370073&view=auto
> ==
> --- cfe/trunk/test/CodeGen/riscv-atomics.c (added)
> +++ cfe/trunk/test/CodeGen/riscv-atomics.c Tue Aug 27 08:41:16 2019
> @@ -0,0 +1,68 @@
> +// RUN: %clang_cc1 -triple riscv32 -O1 -emit-llvm %s -o - \
> +// RUN:   | FileCheck %s -check-prefix=RV32I
> +// RUN: %clang_cc1 -triple riscv32 -target-feature +a -O1 -emit-llvm %s -o - 
> \
> +// RUN:   | FileCheck %s -check-prefix=RV32IA
> +// RUN: %clang_cc1 -triple riscv64 -O1 -emit-llvm %s -o - \
> +// RUN:   | FileCheck %s -check-prefix=RV64I
> +// RUN: %clang_cc1 -triple riscv64 -target-feature +a -O1 -emit-llvm %s -o - 
> \
> +// RUN:   | FileCheck %s -check-prefix=RV64IA
> +
> +// This test demonstrates that MaxAtomicInlineWidth is set appropriately when
> +// the atomics instruction set extension is enabled.
> +
> +#include 
> +#include 
> +
> +void test_i8_atomics(_Atomic(int8_t) * a, int8_t b) {
> +  // RV32I:  call zeroext i8 @__atomic_load_1
> +  // RV32I:  call void @__atomic_store_1
> +  // RV32I:  call zeroext i8 @__atomic_fetch_add_1
> +  // RV32IA: load atomic i8, i8* %a seq_cst, align 1
> +  // RV32IA: store atomic i8 %b, i8* %a seq_cst, align 1
> +  // RV32IA: atomicrmw add i8* %a, i8 %b seq_cst
> +  // RV64I:  call zeroext i8 @__atomic_load_1
> +  // RV64I:  call void @__atomic_store_1
> +  // RV64I:  call zeroext i8 @__atomic_fetch_add_1
> +  // RV64IA: load atomic i8, i8* %a seq_cst, align 1
> +  // RV64IA: store atomic i8 %b, i8* %a seq_cst, align 1
> +  // RV64IA: atomicrmw add i8* %a, i8 %b seq_cst
> +  __c11_atomic_load(a, memory_order_seq_cst);
> +  __c11_atomic_store(a, b, memory_order_seq_cst);
> +  __c11_atomic_fetch_add(a, b, memory_order_seq_cst);
> +}
> +
> +void test_i32_atomics(_Atomic(int32_t) * a, int32_t b) {
> +  // RV32I:  call i32 @__atomic_load_4
> +  // RV32I:  call void @__atomic_store_4
> +  // RV32I:  call i32 @__atomic_fetch_add_4
> +  // RV32IA: load atomic i32, i32* %a seq_cst, align 4
> +  // RV32IA: store atomic i32 %b, i32* %a seq_cst, align 4
> +  // RV32IA: atomicrmw add i32* %a, i32 %b seq_cst
> +  // RV64I:  call signext i32 @__atomic_load_4
> +  // RV64I:  call void @__atomic_store_4
> +  // RV64I:  call signext i32 @__atomic_fetch_add_4
> +  // RV64IA: load atomic i32, i32* %a seq_cst, align 4
> +  // RV64IA: store atomic i32 %b, i32* %a seq_cst, align 4
> +  // RV64IA: atomicrmw add i32* %a, i32 %b seq_cst
> +  __c11_atomic_load(a, memory_order_seq_cst);
> +  __c11_atomic_store(a, b, memory_orde

[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp.
Herald added a reviewer: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: clang.

Correct order of fields and indirect fields in imported RecordDecl
is needed for correct work of record layout calculations.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66866

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,23 @@
   return Index == Order.size();
 }
 
+AST_MATCHER_P(RecordDecl, hasFieldIndirectFieldOrder, std::vector,
+  Order) {
+  size_t Index = 0;
+  for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+  if (auto *ND = cast(D)) {
+if (Index == Order.size())
+  return false;
+if (ND->getName() != Order[Index])
+  return false;
+++Index;
+  }
+}
+  }
+  return Index == Order.size();
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
TUshouldContainClassTemplateSpecializationOfExplicitInstantiation) {
   Decl *From, *To;
@@ -1493,6 +1510,31 @@
   Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+   CXXRecordDeclFieldAndIndirectFieldOrder) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  // First field is "a", then the field for unnamed union, then "b" and "c"
+  // from it, then "d".
+  R"s(
+  struct declToImport {
+int a = d;
+union { 
+  int b;
+  int c;
+};
+int d;
+  };
+  )s",
+  Lang_CXX11, "", Lang_CXX11);
+
+  MatchVerifier Verifier;
+  ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(hasFieldIndirectFieldOrder(
+   {"a", "", "b", "c", "d"};
+  EXPECT_TRUE(Verifier.match(
+  To, cxxRecordDecl(hasFieldIndirectFieldOrder({"a", "", "b", "c", 
"d"};
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1701,7 +1701,7 @@
   // Remove all declarations, which may be in wrong order in the
   // lexical DeclContext and then add them in the proper order.
   for (auto *D : FromRD->decls()) {
-if (isa(D) || isa(D)) {
+if (isa(D) || isa(D) || isa(D)) {
   assert(D && "DC contains a null decl");
   Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
   // Remove only the decls which we successfully imported.


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,23 @@
   return Index == Order.size();
 }
 
+AST_MATCHER_P(RecordDecl, hasFieldIndirectFieldOrder, std::vector,
+  Order) {
+  size_t Index = 0;
+  for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+  if (auto *ND = cast(D)) {
+if (Index == Order.size())
+  return false;
+if (ND->getName() != Order[Index])
+  return false;
+++Index;
+  }
+}
+  }
+  return Index == Order.size();
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
TUshouldContainClassTemplateSpecializationOfExplicitInstantiation) {
   Decl *From, *To;
@@ -1493,6 +1510,31 @@
   Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase,
+   CXXRecordDeclFieldAndIndirectFieldOrder) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  // First field is "a", then the field for unnamed union, then "b" and "c"
+  // from it, then "d".
+  R"s(
+  struct declToImport {
+int a = d;
+union { 
+  int b;
+  int c;
+};
+int d;
+  };
+  )s",
+  Lang_CXX11, "", Lang_CXX11);
+
+  MatchVerifier Verifier;
+  ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(hasFieldIndirectFieldOrder(
+   {"a", "", "b", "c", "d"};
+  EXPECT_TRUE(Verifier.match(
+  To, cxxRecordDecl(hasFieldIndirectFieldOrder({"a", "", "b", "c", "d"};
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1701,7 +1701,7 @@
   // Remove all declarations, whi

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev added a comment.

In D63325#1645892 , @mgehre wrote:

> I'm seeing the same issue `Not all CodeGen sections are inside any Frontend 
> section!` with python 3.7.1. Json: F9863382: check-time-trace-sections.json 
> 


Yes, confirmed that is python version issue, I'm working on it.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63325/new/

https://reviews.llvm.org/D63325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66588: [ARM NEON] Avoid duplicated decarations

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio updated this revision to Diff 217594.
dnsampaio added a comment.

Fix / Update / Rebase

- Avoid appending __noswap_ to intrinsics that are BigEndianSafe
- Moved to monorepo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66588/new/

https://reviews.llvm.org/D66588

Files:
  clang/utils/TableGen/NeonEmitter.cpp


Index: clang/utils/TableGen/NeonEmitter.cpp
===
--- clang/utils/TableGen/NeonEmitter.cpp
+++ clang/utils/TableGen/NeonEmitter.cpp
@@ -332,6 +332,17 @@
   NeonEmitter &Emitter;
   std::stringstream OS;
 
+  bool isBigEndianSafe() const {
+if (BigEndianSafe)
+  return true;
+
+for (const auto &T : Types){
+  if (T.isVector() && T.getNumElements() > 1)
+return false;
+}
+return true;
+  }
+
 public:
   Intrinsic(Record *R, StringRef Name, StringRef Proto, TypeSpec OutTS,
 TypeSpec InTS, ClassKind CK, ListInit *Body, NeonEmitter &Emitter,
@@ -1293,7 +1304,7 @@
 }
 
 void Intrinsic::emitArgumentReversal() {
-  if (BigEndianSafe)
+  if (isBigEndianSafe())
 return;
 
   // Reverse all vector arguments.
@@ -1314,7 +1325,7 @@
 }
 
 void Intrinsic::emitReturnReversal() {
-  if (BigEndianSafe)
+  if (isBigEndianSafe())
 return;
   if (!getReturnType().isVector() || getReturnType().isVoid() ||
   getReturnType().getNumElements() == 1)
@@ -1578,7 +1589,10 @@
   Intr.Dependencies.insert(&Callee);
 
   // Now create the call itself.
-  std::string S = CallPrefix.str() + Callee.getMangledName(true) + "(";
+  std::string S = "";
+  if (!Callee.isBigEndianSafe())
+S += CallPrefix.str();
+  S += Callee.getMangledName(true) + "(";
   for (unsigned I = 0; I < DI->getNumArgs() - 1; ++I) {
 if (I != 0)
   S += ", ";
@@ -1889,6 +1903,11 @@
 }
 
 std::string Intrinsic::generate() {
+  // Avoid duplicated code for big and small endians
+  if (isBigEndianSafe()) {
+generateImpl(false, "", "");
+return OS.str();
+  }
   // Little endian intrinsics are simple and don't require any argument
   // swapping.
   OS << "#ifdef __LITTLE_ENDIAN__\n";


Index: clang/utils/TableGen/NeonEmitter.cpp
===
--- clang/utils/TableGen/NeonEmitter.cpp
+++ clang/utils/TableGen/NeonEmitter.cpp
@@ -332,6 +332,17 @@
   NeonEmitter &Emitter;
   std::stringstream OS;
 
+  bool isBigEndianSafe() const {
+if (BigEndianSafe)
+  return true;
+
+for (const auto &T : Types){
+  if (T.isVector() && T.getNumElements() > 1)
+return false;
+}
+return true;
+  }
+
 public:
   Intrinsic(Record *R, StringRef Name, StringRef Proto, TypeSpec OutTS,
 TypeSpec InTS, ClassKind CK, ListInit *Body, NeonEmitter &Emitter,
@@ -1293,7 +1304,7 @@
 }
 
 void Intrinsic::emitArgumentReversal() {
-  if (BigEndianSafe)
+  if (isBigEndianSafe())
 return;
 
   // Reverse all vector arguments.
@@ -1314,7 +1325,7 @@
 }
 
 void Intrinsic::emitReturnReversal() {
-  if (BigEndianSafe)
+  if (isBigEndianSafe())
 return;
   if (!getReturnType().isVector() || getReturnType().isVoid() ||
   getReturnType().getNumElements() == 1)
@@ -1578,7 +1589,10 @@
   Intr.Dependencies.insert(&Callee);
 
   // Now create the call itself.
-  std::string S = CallPrefix.str() + Callee.getMangledName(true) + "(";
+  std::string S = "";
+  if (!Callee.isBigEndianSafe())
+S += CallPrefix.str();
+  S += Callee.getMangledName(true) + "(";
   for (unsigned I = 0; I < DI->getNumArgs() - 1; ++I) {
 if (I != 0)
   S += ", ";
@@ -1889,6 +1903,11 @@
 }
 
 std::string Intrinsic::generate() {
+  // Avoid duplicated code for big and small endians
+  if (isBigEndianSafe()) {
+generateImpl(false, "", "");
+return OS.str();
+  }
   // Little endian intrinsics are simple and don't require any argument
   // swapping.
   OS << "#ifdef __LITTLE_ENDIAN__\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66869: [clangd] Fix SelectionTree to allow selection range expression in foreach loops.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66869

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,22 @@
  struct Foo*> {};
   )cpp",
   "TemplateTemplateParmDecl"},
+
+  // Foreach has a weird AST, ensure we can select parts of the range init.
+  // This used to fail, because the DeclStmt for C claimed the whole range.
+  {
+  R"cpp(
+struct Str {
+  const char *begin();
+  const char *end();
+};
+Str makeStr(const char*);
+void loop() {
+  for (const char* C : [[mak^eStr("f^oo")]])
+;
+}
+  )cpp",
+  "CallExpr"},
   };
   for (const Case &C : Cases) {
 Annotations Test(C.Code);
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -228,6 +228,16 @@
   bool TraverseNestedNameSpecifier(NestedNameSpecifier *) { return true; }
   bool TraverseType(QualType) { return true; }
 
+  // The DeclStmt for the loop variable claims to cover the whole range
+  // inside the parens, this causes the range-init expression to not be hit.
+  // Traverse the loop VarDecl instead, which has the right source range.
+  bool TraverseCXXForRangeStmt(CXXForRangeStmt *S) {
+return traverseNode(S, [&] {
+  return TraverseStmt(S->getInit()) && TraverseDecl(S->getLoopVariable()) 
&&
+ TraverseStmt(S->getRangeInit()) && TraverseStmt(S->getBody());
+});
+  }
+
 private:
   using Base = RecursiveASTVisitor;
 


Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,22 @@
  struct Foo*> {};
   )cpp",
   "TemplateTemplateParmDecl"},
+
+  // Foreach has a weird AST, ensure we can select parts of the range init.
+  // This used to fail, because the DeclStmt for C claimed the whole range.
+  {
+  R"cpp(
+struct Str {
+  const char *begin();
+  const char *end();
+};
+Str makeStr(const char*);
+void loop() {
+  for (const char* C : [[mak^eStr("f^oo")]])
+;
+}
+  )cpp",
+  "CallExpr"},
   };
   for (const Case &C : Cases) {
 Annotations Test(C.Code);
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -228,6 +228,16 @@
   bool TraverseNestedNameSpecifier(NestedNameSpecifier *) { return true; }
   bool TraverseType(QualType) { return true; }
 
+  // The DeclStmt for the loop variable claims to cover the whole range
+  // inside the parens, this causes the range-init expression to not be hit.
+  // Traverse the loop VarDecl instead, which has the right source range.
+  bool TraverseCXXForRangeStmt(CXXForRangeStmt *S) {
+return traverseNode(S, [&] {
+  return TraverseStmt(S->getInit()) && TraverseDecl(S->getLoopVariable()) &&
+ TraverseStmt(S->getRangeInit()) && TraverseStmt(S->getBody());
+});
+  }
+
 private:
   using Base = RecursiveASTVisitor;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 217600.
Tyker added a comment.

Sorry for the long wait.

Changes:

- Rebased on current master
- Duplicated test file so that it runs for both importing


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/ASTMerge/APValue/APValue.cpp
  clang/test/PCH/APValue.cpp

Index: clang/test/PCH/APValue.cpp
===
--- /dev/null
+++ clang/test/PCH/APValue.cpp
@@ -0,0 +1,210 @@
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -include-pch %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+constexpr int Unique_Int = int(6789);
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int' 6789
+
+constexpr __uint128_t Unique_Int128 = ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+//CHECK:  VarDecl {{.*}} Unique_Int128 
+//CHECK-NEXT: ConstantExpr {{.*}} 'unsigned __int128' 156773562844924187900898496343692168785
+
+}
+
+namespace FloatingPoint {
+
+constexpr double Unique_Double = double(567890.67890);
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}} 'double' 5.678907e+05
+
+}
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B Basic_Struct = B{1, 0.7};
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::B' {1, 7.00e-01}
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  int i;
+  double d;
+  C c;
+};
+
+constexpr A Advanced_Struct = A{Basic_Struct, 1, 79.789, {}};
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::A' {{[{][{]}}1, 7.00e-01}, 1, 7.978900e+01, {9}}
+
+}
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Vector_Int = (v4si){8, 2, 3};
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'Vector::v4si':'__attribute__((__vector_size__(4 * sizeof(int int' {8, 2, 3, 0}
+
+}
+
+namespace Array {
+
+constexpr int Array_Int[] = {1, 2, 3, 4, 5, 6};
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int [6]' {1, 2, 3, 4, 5, 6}
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+constexpr A Array2_Struct[][3] = {{{}, {-45678, 9.8}, {9}}, {{}}};
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'Array::A const [2][3]' {{[{][{]}}{789, 6.789010e+04}, {-45678, 9.80e+00}, {9, 6.789010e+04}}, {{[{][{]}}789, 6.789010e+04}, {789, 6.789010e+04}, {789, 6.789010e+04}}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Array_Vector[] = {{1, 2, 3, 4}, {4, 5, 6, 7}};
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Array::v4si [2]' {{[{][{]}}1, 2, 3, 4}, {4, 5, 6, 7}}
+
+}
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+constexpr U Unique_Union1 = U{0};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.i = 0}
+
+constexpr U Unique_Union2 = U{};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.a = {567890, 9.876567e+03}}
+
+}
+
+namespace MemberPointer{
+
+struct A {
+  struct B {
+struct C {
+  struct D {
+struct E {
+  struct F {
+struct G {
+  int i;
+};
+  };
+};
+  };
+};
+  };
+};
+
+constexpr auto MemberPointer1 = &A::B::C::D::E::F::G::i;
+//CHECK:  VarDecl {{.*}} MemberPointer1
+//CHECK-NEXT: ConstantExpr {{.*}} 'int MemberPointer::A::B::C::D::E::F::G::*' &G::i
+
+struct A1 {
+  struct B1 {
+int f() const {
+  return 0;
+}
+  };
+
+};
+
+constexpr auto MemberPointer2 = &A1::B1::f;
+//CHECK:  VarDecl {{.*}} MemberPointer2
+//CHECK-NEXT: ConstantExpr {{.*}} 'int (MemberPointer::A1::B1::*)() const' &B1::f
+
+}
+
+namespace std {
+  struct type_info;
+};
+
+namespace LValue {
+
+constexpr int LValueInt = 0;
+constexpr const int& ConstIntRef = LValueInt;
+//CHECK:  VarDecl {{.*}} ConstIntRef
+//CHECK-NEXT: Con

[PATCH] D66864: [clang-tidy] readability-identifier-naming shouldn't complain about CRTP pseudo-overrides

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

looks good, thanks for fixing it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66864/new/

https://reviews.llvm.org/D66864



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50147: clang-format: support external styles

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50147/new/

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 217604.
serge-sans-paille added a comment.

@Meinersbur patch rebased. I removed the linker trick (which only work for 
global variables, not function, anyway), as it's no longer needed:

  target_compile_definitions(${name} PRIVATE LLVM_${name_upper}_LINK_INTO_TOOLS)

is used to only provide the symbol if we're in shared library mode.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61446/new/

https://reviews.llvm.org/D61446

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CMakeLists.txt
  clang/tools/driver/CMakeLists.txt
  clang/tools/driver/cc1_main.cpp
  llvm/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/docs/WritingAnLLVMPass.rst
  llvm/examples/Bye/Bye.cpp
  llvm/examples/Bye/CMakeLists.txt
  llvm/examples/CMakeLists.txt
  llvm/include/llvm/Config/llvm-config.h.cmake
  llvm/test/Feature/load_extension.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/opt-O0-pipeline.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/lit.cfg.py
  llvm/test/lit.site.cfg.py.in
  llvm/tools/CMakeLists.txt
  llvm/tools/bugpoint/CMakeLists.txt
  llvm/tools/bugpoint/bugpoint.cpp
  llvm/tools/opt/CMakeLists.txt
  llvm/tools/opt/NewPMDriver.cpp
  llvm/tools/opt/opt.cpp
  llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn
  polly/include/polly/RegisterPasses.h
  polly/lib/CMakeLists.txt
  polly/lib/Polly.cpp
  polly/lib/Support/RegisterPasses.cpp
  polly/test/Unit/lit.site.cfg.in
  polly/test/lit.site.cfg.in
  polly/test/update_check.py

Index: polly/test/update_check.py
===
--- polly/test/update_check.py
+++ polly/test/update_check.py
@@ -15,7 +15,7 @@
 polly_lib_dir = '''@POLLY_LIB_DIR@'''
 shlibext = '''@LLVM_SHLIBEXT@'''
 llvm_tools_dir = '''@LLVM_TOOLS_DIR@'''
-link_polly_into_tools = not '''@LINK_POLLY_INTO_TOOLS@'''.lower() in {'','0','n','no','off','false','notfound','link_polly_into_tools-notfound'}
+llvm_polly_link_into_tools = not '''@LLVM_POLLY_LINK_INTO_TOOLS@'''.lower() in {'','0','n','no','off','false','notfound','llvm_polly_link_into_tools-notfound'}
 
 runre = re.compile(r'\s*\;\s*RUN\s*\:(?P.*)')
 filecheckre = re.compile(r'\s*(?P.*)\|\s*(?PFileCheck\s[^|]*)')
@@ -298,7 +298,7 @@
 toolarg = toolarg.replace('%s', filename)
 toolarg = toolarg.replace('%S', os.path.dirname(filename))
 if toolarg == '%loadPolly':
-if not link_polly_into_tools:
+if not llvm_polly_link_into_tools:
 newtool += ['-load',os.path.join(polly_lib_dir,'LLVMPolly' + shlibext)]
 newtool.append('-polly-process-unprofitable')
 newtool.append('-polly-remarks-minimal')
Index: polly/test/lit.site.cfg.in
===
--- polly/test/lit.site.cfg.in
+++ polly/test/lit.site.cfg.in
@@ -8,7 +8,7 @@
 config.polly_lib_dir = "@POLLY_LIB_DIR@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.enable_gpgpu_codegen = "@GPU_CODEGEN@"
-config.link_polly_into_tools = "@LINK_POLLY_INTO_TOOLS@"
+config.llvm_polly_link_into_tools = "@LLVM_POLLY_LINK_INTO_TOOLS@"
 config.targets_to_build = "@TARGETS_TO_BUILD@"
 config.extra_paths = "@POLLY_TEST_EXTRA_PATHS@".split(";")
 
@@ -36,14 +36,14 @@
 # directories.
 config.excludes = ['Inputs']
 
-if config.link_polly_into_tools == '' or \
-   config.link_polly_into_tools.lower() == '0' or \
-   config.link_polly_into_tools.lower() == 'n' or \
-   config.link_polly_into_tools.lower() == 'no' or \
-   config.link_polly_into_tools.lower() == 'off' or \
-   config.link_polly_into_tools.lower() == 'false' or \
-   config.link_polly_into_tools.lower() == 'notfound' or \
-   config.link_polly_into_tools.lower() == 'link_polly_into_tools-notfound':
+if config.llvm_polly_link_into_tools == '' or \
+   config.llvm_polly_link_into_tools.lower() == '0' or \
+   config.llvm_polly_link_into_tools.lower() == 'n' or \
+   config.llvm_polly_link_into_tools.lower() == 'no' or \
+   config.llvm_polly_link_into_tools.lower() == 'off' or \
+   config.llvm_polly_link_into_tools.lower() == 'false' or \
+   config.llvm_polly_link_into_tools.lower() == 'notfound' or \
+   config.llvm_polly_link_into_tools.lower() == 'llvm_polly_link_into_tools-notfound':
 config.substitutions.append(('%loadPolly', '-load '
  + config.polly_lib_dir + '/LLVMPolly@LLVM_SHLIBEXT@'
  + ' -load-pass-plugin '
Index: polly/test/Unit/lit.site.cfg.in
===
--- polly/test/Unit/lit.site.cfg.in
+++ polly/test/Unit/lit.site.cfg.in
@@ -13,7 +13,7 @@
 config.shlibdir = "@SHLIBDIR@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.enable_gpgpu_codegen = "@GPU_CODEG

[PATCH] D66869: [clangd] Fix SelectionTree to allow selection range expression in foreach loops.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:275
+void loop() {
+  for (const char* C : [[mak^eStr("f^oo")]])
+;

nit: the ^ inside the string "foo" is a bit tricky, I didn't spot it at the 
first glance, I'd move it out of the string, e.g.  `mak^eStr("foo"^)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66869/new/

https://reviews.llvm.org/D66869



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66872: [clangd] SelectionTree should mark a node as fully-selected if the only claimed tokens were early-claimed.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: SureYeaah.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

Previously they would be marked as partially-selected based on the early claim,
and never updated as no more tokens were claimed.
This affects simple VarDecls like "int x".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66872

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -329,6 +329,8 @@
 #define ECHO(X) X
 ECHO(EC^HO([[$C[[int]]) EC^HO(a]]));
   ]])cpp",
+  R"cpp( $C[[^$C[[int]] a^]]; )cpp",
+  R"cpp( $C[[^$C[[int]] a = $C[[5]]^]]; )cpp",
   };
   for (const char *C : Cases) {
 Annotations Test(C);
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -60,13 +60,13 @@
 
   // Associates any tokens overlapping [Begin, End) with an AST node.
   // Tokens that were already claimed by another AST node are not claimed again.
-  // Returns whether the node is selected in the sense of SelectionTree.
-  SelectionTree::Selection claim(unsigned Begin, unsigned End) {
+  // Updates Result if the node is selected in the sense of SelectionTree.
+  void claim(unsigned Begin, unsigned End, SelectionTree::Selection &Result) {
 assert(Begin <= End);
 
 // Fast-path for missing the selection entirely.
 if (Begin >= SelEnd || End <= SelBegin)
-  return SelectionTree::Unselected;
+  return;
 
 // We will consider the range (at least partially) selected if it hit any
 // selected and previously unclaimed token.
@@ -96,10 +96,15 @@
 PartialSelection = true;
   }
 }
-
-if (!ClaimedAnyToken)
-  return SelectionTree::Unselected;
-return PartialSelection ? SelectionTree::Partial : SelectionTree::Complete;
+llvm::errs() << PartialSelection << "partial?\n";
+
+// If some tokens were previously claimed (Result != Unselected), we may
+// upgrade from Partial->Complete, even if no new tokens were claimed.
+// Important for [[int a]].
+if (ClaimedAnyToken || Result) {
+  Result = std::max(Result, PartialSelection ? SelectionTree::Partial
+ : SelectionTree::Complete);
+}
   }
 
 private:
@@ -161,10 +166,13 @@
 assert(V.Stack.size() == 1 && "Unpaired push/pop?");
 assert(V.Stack.top() == &V.Nodes.front());
 // We selected TUDecl if tokens were unclaimed (or the file is empty).
-if (V.Nodes.size() == 1 || V.Claimed.claim(Begin, End)) {
+SelectionTree::Selection UnclaimedTokens = SelectionTree::Unselected;
+V.Claimed.claim(Begin, End, UnclaimedTokens);
+if (UnclaimedTokens || V.Nodes.size() == 1) {
   StringRef FileContent = AST.getSourceManager().getBufferData(File);
   // Don't require the trailing newlines to be selected.
   bool SelectedAll = Begin == 0 && End >= FileContent.rtrim().size();
+  llvm::errs() << "overriding!\n";
   V.Stack.top()->Selected =
   SelectedAll ? SelectionTree::Complete : SelectionTree::Partial;
 }
@@ -317,10 +325,11 @@
 Nodes.emplace_back();
 Nodes.back().ASTNode = std::move(Node);
 Nodes.back().Parent = Stack.top();
-// Early hit detection never selects the whole node.
 Stack.push(&Nodes.back());
-Nodes.back().Selected =
-claimRange(Early) ? SelectionTree::Partial : SelectionTree::Unselected;
+claimRange(Early, Nodes.back().Selected);
+// Early hit detection never selects the whole node.
+if (Nodes.back().Selected)
+  Nodes.back().Selected = SelectionTree::Partial;
   }
 
   // Pops a node off the ancestor stack, and finalizes it. Pairs with push().
@@ -328,8 +337,7 @@
   void pop() {
 Node &N = *Stack.top();
 dlog("{1}pop: {0}", printNodeToString(N.ASTNode, PrintPolicy), indent(-1));
-if (auto Sel = claimRange(N.ASTNode.getSourceRange()))
-  N.Selected = Sel;
+claimRange(N.ASTNode.getSourceRange(), N.Selected);
 if (N.Selected || !N.Children.empty()) {
   // Attach to the tree.
   N.Parent->Children.push_back(&N);
@@ -359,9 +367,10 @@
   // Perform hit-testing of a complete Node against the selection.
   // This runs for every node in the AST, and must be fast in common cases.
   // This is usually called from pop(), so we can take children into account.
-  SelectionTree::Selection claimRange(SourceRange S) {
+  // The existing state of Result is relevant (early/late claims can interact).
+  void claimRange(SourceRange S, SelectionTree::Selection &Re

[clang-tools-extra] r370191 - [clangd] Fix SelectionTree to allow selection range expression in foreach loops.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Aug 28 05:05:12 2019
New Revision: 370191

URL: http://llvm.org/viewvc/llvm-project?rev=370191&view=rev
Log:
[clangd] Fix SelectionTree to allow selection range expression in foreach loops.

Reviewers: hokein

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66869

Modified:
clang-tools-extra/trunk/clangd/Selection.cpp
clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp

Modified: clang-tools-extra/trunk/clangd/Selection.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Selection.cpp?rev=370191&r1=370190&r2=370191&view=diff
==
--- clang-tools-extra/trunk/clangd/Selection.cpp (original)
+++ clang-tools-extra/trunk/clangd/Selection.cpp Wed Aug 28 05:05:12 2019
@@ -228,6 +228,16 @@ public:
   bool TraverseNestedNameSpecifier(NestedNameSpecifier *) { return true; }
   bool TraverseType(QualType) { return true; }
 
+  // The DeclStmt for the loop variable claims to cover the whole range
+  // inside the parens, this causes the range-init expression to not be hit.
+  // Traverse the loop VarDecl instead, which has the right source range.
+  bool TraverseCXXForRangeStmt(CXXForRangeStmt *S) {
+return traverseNode(S, [&] {
+  return TraverseStmt(S->getInit()) && TraverseDecl(S->getLoopVariable()) 
&&
+ TraverseStmt(S->getRangeInit()) && TraverseStmt(S->getBody());
+});
+  }
+
 private:
   using Base = RecursiveASTVisitor;
 

Modified: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp?rev=370191&r1=370190&r2=370191&view=diff
==
--- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp Wed Aug 28 
05:05:12 2019
@@ -261,6 +261,22 @@ TEST(SelectionTest, CommonAncestor) {
  struct Foo*> {};
   )cpp",
   "TemplateTemplateParmDecl"},
+
+  // Foreach has a weird AST, ensure we can select parts of the range init.
+  // This used to fail, because the DeclStmt for C claimed the whole range.
+  {
+  R"cpp(
+struct Str {
+  const char *begin();
+  const char *end();
+};
+Str makeStr(const char*);
+void loop() {
+  for (const char* C : [[mak^eStr("foo"^)]])
+;
+}
+  )cpp",
+  "CallExpr"},
   };
   for (const Case &C : Cases) {
 Annotations Test(C.Code);


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66869: [clangd] Fix SelectionTree to allow selection range expression in foreach loops.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rL370191: [clangd] Fix SelectionTree to allow selection range 
expression in foreach loops. (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66869?vs=217598&id=217609#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66869/new/

https://reviews.llvm.org/D66869

Files:
  clang-tools-extra/trunk/clangd/Selection.cpp
  clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp


Index: clang-tools-extra/trunk/clangd/Selection.cpp
===
--- clang-tools-extra/trunk/clangd/Selection.cpp
+++ clang-tools-extra/trunk/clangd/Selection.cpp
@@ -228,6 +228,16 @@
   bool TraverseNestedNameSpecifier(NestedNameSpecifier *) { return true; }
   bool TraverseType(QualType) { return true; }
 
+  // The DeclStmt for the loop variable claims to cover the whole range
+  // inside the parens, this causes the range-init expression to not be hit.
+  // Traverse the loop VarDecl instead, which has the right source range.
+  bool TraverseCXXForRangeStmt(CXXForRangeStmt *S) {
+return traverseNode(S, [&] {
+  return TraverseStmt(S->getInit()) && TraverseDecl(S->getLoopVariable()) 
&&
+ TraverseStmt(S->getRangeInit()) && TraverseStmt(S->getBody());
+});
+  }
+
 private:
   using Base = RecursiveASTVisitor;
 
Index: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,22 @@
  struct Foo*> {};
   )cpp",
   "TemplateTemplateParmDecl"},
+
+  // Foreach has a weird AST, ensure we can select parts of the range init.
+  // This used to fail, because the DeclStmt for C claimed the whole range.
+  {
+  R"cpp(
+struct Str {
+  const char *begin();
+  const char *end();
+};
+Str makeStr(const char*);
+void loop() {
+  for (const char* C : [[mak^eStr("foo"^)]])
+;
+}
+  )cpp",
+  "CallExpr"},
   };
   for (const Case &C : Cases) {
 Annotations Test(C.Code);


Index: clang-tools-extra/trunk/clangd/Selection.cpp
===
--- clang-tools-extra/trunk/clangd/Selection.cpp
+++ clang-tools-extra/trunk/clangd/Selection.cpp
@@ -228,6 +228,16 @@
   bool TraverseNestedNameSpecifier(NestedNameSpecifier *) { return true; }
   bool TraverseType(QualType) { return true; }
 
+  // The DeclStmt for the loop variable claims to cover the whole range
+  // inside the parens, this causes the range-init expression to not be hit.
+  // Traverse the loop VarDecl instead, which has the right source range.
+  bool TraverseCXXForRangeStmt(CXXForRangeStmt *S) {
+return traverseNode(S, [&] {
+  return TraverseStmt(S->getInit()) && TraverseDecl(S->getLoopVariable()) &&
+ TraverseStmt(S->getRangeInit()) && TraverseStmt(S->getBody());
+});
+  }
+
 private:
   using Base = RecursiveASTVisitor;
 
Index: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
@@ -261,6 +261,22 @@
  struct Foo*> {};
   )cpp",
   "TemplateTemplateParmDecl"},
+
+  // Foreach has a weird AST, ensure we can select parts of the range init.
+  // This used to fail, because the DeclStmt for C claimed the whole range.
+  {
+  R"cpp(
+struct Str {
+  const char *begin();
+  const char *end();
+};
+Str makeStr(const char*);
+void loop() {
+  for (const char* C : [[mak^eStr("foo"^)]])
+;
+}
+  )cpp",
+  "CallExpr"},
   };
   for (const Case &C : Cases) {
 Annotations Test(C.Code);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r370193 - [clang-tidy] readability-identifier-naming shouldn't complain about CRTP pseudo-overrides

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Aug 28 05:08:57 2019
New Revision: 370193

URL: http://llvm.org/viewvc/llvm-project?rev=370193&view=rev
Log:
[clang-tidy] readability-identifier-naming shouldn't complain about CRTP 
pseudo-overrides

Reviewers: ilya-biryukov

Subscribers: xazax.hun, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66864

Modified:
clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp?rev=370193&r1=370192&r2=370193&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp 
Wed Aug 28 05:08:57 2019
@@ -10,6 +10,7 @@
 
 #include "../utils/ASTUtils.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
@@ -579,6 +580,15 @@ static StyleKind findStyleKind(
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 
+// If this method has the same name as any base method, this is likely
+// necessary even if it's not an override. e.g. CRTP.
+auto FindHidden = [&](const CXXBaseSpecifier *S, clang::CXXBasePath &P) {
+  return CXXRecordDecl::FindOrdinaryMember(S, P, Decl->getDeclName());
+};
+CXXBasePaths UnusedPaths;
+if (Decl->getParent()->lookupInBases(FindHidden, UnusedPaths))
+  return SK_Invalid;
+
 if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod])
   return SK_ConstexprMethod;
 

Modified: 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst?rev=370193&r1=370192&r2=370193&view=diff
==
--- 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
 (original)
+++ 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
 Wed Aug 28 05:08:57 2019
@@ -26,6 +26,10 @@ Many configuration options are available
 different rules for different kinds of identifiers. In general, the rules are
 falling back to a more generic rule if the specific case is not configured.
 
+The naming of virtual methods is reported where they occur in the base class,
+but not where they are overridden, as it can't be fixed locally there.
+This also applies for pseudo-override patterns like CRTP.
+
 Options
 ---
 

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp?rev=370193&r1=370192&r2=370193&view=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp 
(original)
+++ clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp 
Wed Aug 28 05:08:57 2019
@@ -262,6 +262,32 @@ void InstantiateClassMethods() {
   CMyWellNamedClass2 x5(42, nullptr);
 }
 
+class AOverridden {
+public:
+  virtual ~AOverridden() = default;
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual 
method 'BadBaseMethod'
+};
+
+class COverriding : public AOverridden {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+};
+
+template 
+class CRTPBase {
+public:
+  void BadBaseMethod(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 
'BadBaseMethod'
+};
+
+class CRTPDerived : CRTPBase {
+public:
+  // Hiding a badly-named base isn't a new violation.
+  double BadBaseMethod(double) { return 0; }
+};
+
 template
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type 
template parameter 'T'
 // CHECK-FIXES: {{^}}template{{$}}


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66864: [clang-tidy] readability-identifier-naming shouldn't complain about CRTP pseudo-overrides

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370193: [clang-tidy] readability-identifier-naming 
shouldn't complain about CRTP pseudo… (authored by sammccall, committed by 
).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66864?vs=217584&id=217610#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66864/new/

https://reviews.llvm.org/D66864

Files:
  clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp
  
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
  clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp


Index: clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
@@ -262,6 +262,32 @@
   CMyWellNamedClass2 x5(42, nullptr);
 }
 
+class AOverridden {
+public:
+  virtual ~AOverridden() = default;
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual 
method 'BadBaseMethod'
+};
+
+class COverriding : public AOverridden {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+};
+
+template 
+class CRTPBase {
+public:
+  void BadBaseMethod(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 
'BadBaseMethod'
+};
+
+class CRTPDerived : CRTPBase {
+public:
+  // Hiding a badly-named base isn't a new violation.
+  double BadBaseMethod(double) { return 0; }
+};
+
 template
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type 
template parameter 'T'
 // CHECK-FIXES: {{^}}template{{$}}
Index: clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -10,6 +10,7 @@
 
 #include "../utils/ASTUtils.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
@@ -579,6 +580,15 @@
 Decl->size_overridden_methods() > 0)
   return SK_Invalid;
 
+// If this method has the same name as any base method, this is likely
+// necessary even if it's not an override. e.g. CRTP.
+auto FindHidden = [&](const CXXBaseSpecifier *S, clang::CXXBasePath &P) {
+  return CXXRecordDecl::FindOrdinaryMember(S, P, Decl->getDeclName());
+};
+CXXBasePaths UnusedPaths;
+if (Decl->getParent()->lookupInBases(FindHidden, UnusedPaths))
+  return SK_Invalid;
+
 if (Decl->isConstexpr() && NamingStyles[SK_ConstexprMethod])
   return SK_ConstexprMethod;
 
Index: 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
+++ 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -26,6 +26,10 @@
 different rules for different kinds of identifiers. In general, the rules are
 falling back to a more generic rule if the specific case is not configured.
 
+The naming of virtual methods is reported where they occur in the base class,
+but not where they are overridden, as it can't be fixed locally there.
+This also applies for pseudo-override patterns like CRTP.
+
 Options
 ---
 


Index: clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-identifier-naming.cpp
@@ -262,6 +262,32 @@
   CMyWellNamedClass2 x5(42, nullptr);
 }
 
+class AOverridden {
+public:
+  virtual ~AOverridden() = default;
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
+};
+
+class COverriding : public AOverridden {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+};
+
+template 
+class CRTPBase {
+public:
+  void BadBaseMethod(int) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for method 'BadBaseMethod'
+};
+
+class CRTPDerived : CRTPBase {
+public:
+  // Hiding a badly-named base isn't a new violation.
+  double BadBaseMethod(double) { return 0; }
+};
+
 template
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for type temp

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

LGTM, but missing a test case.

In D66850#1648557 , @sidorovd wrote:

> LGTM. I'm not an expert in JSON, but may be it makes sense to move the change 
> a line earlier before creation of pointer representation?


I would prefer it remains where it is -- having the `0x0` in the output for a 
null pointer is a good thing because it conveys more information than a totally 
empty `Type` object. We're accidentally inconsistent about this currently (Decl 
prints 0x0 but Stmt gives an empty object).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66873: [Test][Time profiler] Fix test for python3

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev created this revision.
anton-afanasyev added reviewers: mgehre, nathanchance.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix test checking time profiler generates correct tracing json-file.
`filter` works differently for python2 and python3, so unifying this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66873

Files:
  clang/test/Driver/check-time-trace-sections.py


Index: clang/test/Driver/check-time-trace-sections.py
===
--- clang/test/Driver/check-time-trace-sections.py
+++ clang/test/Driver/check-time-trace-sections.py
@@ -12,9 +12,9 @@
 return b <= c
 
 events = json.loads(sys.stdin.read())["traceEvents"]
-codegens = filter(lambda x: x["name"] == "CodeGen Function", events)
-frontends = filter(lambda x: x["name"] == "Frontend", events)
-backends = filter(lambda x: x["name"] == "Backend", events)
+codegens = [event for event in events if event["name"] == "CodeGen Function"]
+frontends = [event for event in events if event["name"] == "Frontend"]
+backends = [event for event in events if event["name"] == "Backend"]
 
 if not all([any([is_inside(codegen, frontend) for frontend in frontends])
 for codegen in codegens]):


Index: clang/test/Driver/check-time-trace-sections.py
===
--- clang/test/Driver/check-time-trace-sections.py
+++ clang/test/Driver/check-time-trace-sections.py
@@ -12,9 +12,9 @@
 return b <= c
 
 events = json.loads(sys.stdin.read())["traceEvents"]
-codegens = filter(lambda x: x["name"] == "CodeGen Function", events)
-frontends = filter(lambda x: x["name"] == "Frontend", events)
-backends = filter(lambda x: x["name"] == "Backend", events)
+codegens = [event for event in events if event["name"] == "CodeGen Function"]
+frontends = [event for event in events if event["name"] == "Frontend"]
+backends = [event for event in events if event["name"] == "Backend"]
 
 if not all([any([is_inside(codegen, frontend) for frontend in frontends])
 for codegen in codegens]):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev added a comment.

Hi @nathanchance, @mgehre, fixed here: https://reviews.llvm.org/D66873, please, 
look at it to commit.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63325/new/

https://reviews.llvm.org/D63325



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50147: clang-format: support external styles

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: MyDeveloperDay.
sammccall added a comment.

Sorry about the long delays in responses.
I don't think the feature, as you want to scope it, belongs in clang-format.
@klimek @MyDeveloperDay may have different opinions.

In D50147#1533892 , @Typz wrote:

> In D50147#157 , @sammccall wrote:
>
> > One thing that's unclear to me is whether your aim is to
> >
> > 1. solve a concrete problem for your organization
> > 2. solve a family of problems for similar organizations
> > 3. add a new way of configuring styles for many types of users/projects
>
>
> First and forehand, I have a problem to solve in my organization : we have 
> many projects, and maintaining the config file in some many repositories is 
> not practical.
>  In some case (like, LLVM, google or mozilla), this is not an issue because 
> the formatting rules are hard-coded in clang-format.


I think this option is available to anyone with a public style guide that 
covers a reasonably large body of open-source code, and which can be reasonably 
well supported by clang-format.
I'd expect QT, KDE etc should be able to use this mechanism.

> In our case, we actually have more than one "standard" style, a combination 
> of various OS (linux, windows, macos), and not a very strong control on user 
> computers. So we cannot rely on a specific file or variable being setup by an 
> administrator.

In this case my best advice would be in the short term to use .clang-format 
files. Longer term, some combination of using well-known styles, publicising 
and teaching clang-format about the styles you use, and gaining the ability to 
set environment variables would reduce duplication.

> I think many orgs have the same issue, but some of them have found a solution 
> by hard-coding their style in clang-format...

I'd like to see evidence that this is a widespread problem.

>> With that in mind, I'd be very happy to approve the build time config and/or 
>> an env variable, as long as they're off by default. It's easy to turn them 
>> on later, but not easy to turn them off.
>>  If they're going to be on by default, I think we need a strong reason.
> 
> I they are going to be off by default, it means we would still need to patch 
> clang-format to use it, correct ?

Sorry, by "off by default" I mean that if the environment variable is not set, 
there would be no default search directory. Relative paths would be an error.
So you could install styles centrally on each machine, and they would work if 
CLANG_FORMAT_STYLE_PATH was set, otherwise you'd get the fallback style. Would 
that be workable?

> In D50147#157 , @sammccall wrote:
> 
>> >> - understanding how distro packaging is going to work
>>
>> There's a mechanism, but how is it to be used? Will/should projects with a 
>> style guide provide style packages for distros? Or should these be part of 
>> the "official" clang-format package? 
>>  If separate packages exist, how much is it going to confuse users that 
>> clang-format will silently format the same project with a `.clang-format` 
>> file different ways depending on what's installed?
> 
> 
> The goal is to actually separate the styles from clang-format : so I don't 
> see the point to make them part of the official clang-format package.
>  Usage may be different: the styles may be setup through different packages 
> (e.g. Qt style in qt-core package), installed manually by user, 
>  This is surely not perfect, since different packages may indeed provide the 
> same style : technically this is not an issue (packages must just be marked 
> as conflicting), but it is indeed the organisation's responsibility to use 
> different names for the styles...
>  (to some extent, this may be improved by passing URLs for external styles, 
> e.g. from git ; but this may even be an issue if different mirrors must be 
> used...)

The large majority of our users install clang-format through LLVM releases or 
OS distributors that repackage the same releases. There's no "the organization" 
to defer responsibility to; that's us.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50147/new/

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63960: [C++20] Add consteval-specifique semantic

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 217615.
Tyker added a comment.

Rebased

@rsmith Ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63960/new/

https://reviews.llvm.org/D63960

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -56,3 +56,276 @@
 consteval int main() { // expected-error {{'main' is not allowed to be declared consteval}}
   return 0;
 }
+
+int i_runtime; // expected-note+ {{declared here}}
+constexpr int i_constexpr = 0;
+
+consteval int f_eval(int i) {
+// expected-note@-1+ {{declared here}}
+  return i;
+}
+
+constexpr auto l_eval = [](int i) consteval {
+// expected-note@-1+ {{declared here}}
+  return i;
+};
+
+struct A {
+  int I = 0;
+  consteval int f_eval(int i) const {
+// expected-note@-1+ {{declared here}}
+return I + i;
+// expected-note@-1 {{is not allowed in a constant expression}}
+  }
+};
+
+constexpr A a;
+
+namespace invalid_call {
+
+int d2 = f_eval(i_runtime);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+int l2 = l_eval(i_runtime);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+int m2 = a.f_eval(i_runtime);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+int d4 = f_eval(i_constexpr);
+int l4 = l_eval(i_constexpr);
+int m4 = a.f_eval(i_constexpr);
+
+constexpr int f1(int i) { // expected-note+ {{declared here}}
+  int d0 = f_eval(0);
+  int l0 = l_eval(0);
+  int m0 = a.f_eval(0);
+  int d2 = f_eval(i);
+  // expected-error@-1 {{could not be evaluated}}
+  // expected-error@-2 {{must be initialized}}
+  // FIXME: the error above should not appear when the initializer present but is invalid.
+  // expected-note@-4 {{is not allowed in a constant expression}}
+
+  int l2 = l_eval(i);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+  int m2 = a.f_eval(i);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+  int d6 = f_eval(i_constexpr);
+  int l6 = l_eval(i_constexpr);
+  int m6 = a.f_eval(i_constexpr);
+  int d8 = f_eval(i_runtime);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+  int l8 = l_eval(i_runtime);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+  int m8 = a.f_eval(i_runtime);
+// expected-error@-1 {{could not be evaluated}}
+// expected-note@-2 {{is not allowed in a constant expression}}
+  return 0;
+}
+
+consteval int f2(int i) {
+// expected-error@-1 {{never produces a constant expression}}
+  int d0 = f_eval(i);
+  int l0 = l_eval(i);
+  int m0 = a.f_eval(i);
+  int d1 = f_eval(i_runtime);
+// expected-note@-1 {{is not allowed in a constant expression}}
+  int l1 = l_eval(i_runtime);
+  int m1 = a.f_eval(i_runtime);
+  return 0;
+}
+
+void test() {
+  A a_dependent;
+  // expected-note@-1+ {{declared here}}
+
+  int Int = 0;
+  auto l_dependent = [Int](int i) consteval {
+// expected-note@-1+ {{declared here}}
+return Int + i;
+// expected-note@-1 {{is not allowed in a constant expression}}
+  };
+
+  int i_l = l_dependent(0);
+  // expected-error@-1 {{could not be evaluated}}
+  // expected-note@-2 {{in call}}
+
+  int i_m = a_dependent.f_eval(0);
+  // expected-error@-1 {{could not be evaluated}}
+  // expected-note@-2 {{in call}}
+}
+
+}
+
+namespace taking_address {
+
+using func_type = int(int);
+using mem_ptr_type = int(A::*)(int);
+
+func_type* p1 = (&f_eval);
+// expected-error@-1 {{take address}}
+func_type* p2= &(((f_eval)));
+// expected-error@-1 {{take address}}
+func_type* p3 = (func_type*)f_eval;
+// expected-error@-1 {{take address}}
+func_type* p4 = static_cast(f_eval);
+// expected-error@-1 {{take address}}
+func_type* p5 = reinterpret_cast(f_eval);
+// expected-error@-1 {{take address}}
+func_type* p6 = reinterpret_cast(&reinterpret_cast(f_eval));
+// expected-error@-1 {{take address}}
+func_type* p7 = __builtin_addressof(f_eval);
+// expected-error@-1 {{take address}}
+
+auto p = f_eval;
+// expected-error@-1 {{take address}}
+
+mem_ptr_type m1 = &A::f_eval;
+// expected-error@-1 {{take address}}
+auto* l1 = &decltype(l_eval)::operator();
+// expected-error@-1 {{take address}}
+
+consteval int f(int i) {
+// expected-note@-1+ {{declared here}}
+  return i;
+}
+
+auto ptr = &f;
+// expected-error@-1 {{take address}}

[PATCH] D66556: [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D66556#1648591 , @hans wrote:

> In D66556#1648118 , @dexonsmith 
> wrote:
>
> > In D66556#1648109 , @rnk wrote:
> >
> > > I'm not sure what happens, but I see you added .gitattributes. I'd commit 
> > > it as is. Buildbots using svn will keep working. You can check that the 
> > > monorepo has the right line endings afterwards, and try again if not.
> >
> >
> > SGTM.
>
>
> This broke users of the monorepo, where the file would show up as having 
> changed locally, and with no way to reset it. I'm guessing that's because it 
> was checked in with LF endings and then because of the .gitattributes file, 
> it changes at checkout. I think the correct solution would be to check in the 
> file with CRLF endings and not set any attributes or stuff. (Though this is 
> all super confusing and I might have got it wrong.)
>
> I've deleted the test file in r370175 to unblock development in the meantime.


It already had CRLF endings locally before commit.
It is strange, the file shows up as having CRLF endings in the old revision in 
git, before the revert: 
https://raw.githubusercontent.com/llvm/llvm-project/9774a2ba279aea35f166b8ca489d0e8292026c38/clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
The same kind of problem occured in rL311683 
, later fixed by rL311732 
.
If you're fine with that, I'll do the same thing: remove `.gitattributes` and 
just re-commit the file 
`minimize_source_to_dependency_directives_invalid_error.c` with `svn:eol-style 
CRLF`.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66556/new/

https://reviews.llvm.org/D66556



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66874: [clang-tidy] Fix the potential infinite loop in recordIsTriviallyDefaultConstructible.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 217617.
hokein removed a subscriber: wuzish.
hokein added a comment.

Add newline at end of tile.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66874/new/

https://reviews.llvm.org/D66874

Files:
  clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
  
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp


Index: 
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s 
cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' 
[clang-diagnostic-error]
+  int a = 10;
+};
Index: clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,11 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // Don't perform the check on an ill-formed Decl. As we will visit every 
class
+  // member recursively, an ill-formed Decl may cause an infinite loop during
+  // the runtime.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())


Index: clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' [clang-diagnostic-error]
+  int a = 10;
+};
Index: clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,11 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // Don't perform the check on an ill-formed Decl. As we will visit every class
+  // member recursively, an ill-formed Decl may cause an infinite loop during
+  // the runtime.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66874: [clang-tidy] Fix the potential infinite loop in recordIsTriviallyDefaultConstructible.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: gribozavr.
Herald added subscribers: kbarton, xazax.hun, nemanjai.
Herald added a project: clang.

The recordIsTriviallyDefaultConstructible may cause an infinite loop when
running on an ill-formed decl.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66874

Files:
  clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
  
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp


Index: 
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s 
cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' 
[clang-diagnostic-error]
+  int a = 10;
+};
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,11 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // Don't perform the check on an ill-formed Decl. As we will visit every 
class
+  // member recursively, an ill-formed Decl may cause an infinite loop during
+  // the runtime.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())


Index: clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' [clang-diagnostic-error]
+  int a = 10;
+};
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,11 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // Don't perform the check on an ill-formed Decl. As we will visit every class
+  // member recursively, an ill-formed Decl may cause an infinite loop during
+  // the runtime.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66556: [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D66556#1648591 , @hans wrote:

> In D66556#1648118 , @dexonsmith 
> wrote:
>
> > In D66556#1648109 , @rnk wrote:
> >
> > > I'm not sure what happens, but I see you added .gitattributes. I'd commit 
> > > it as is. Buildbots using svn will keep working. You can check that the 
> > > monorepo has the right line endings afterwards, and try again if not.
> >
> >
> > SGTM.
>
>
> This broke users of the monorepo, where the file would show up as having 
> changed locally, and with no way to reset it. I'm guessing that's because it 
> was checked in with LF endings and then because of the .gitattributes file, 
> it changes at checkout. I think the correct solution would be to check in the 
> file with CRLF endings and not set any attributes or stuff. (Though this is 
> all super confusing and I might have got it wrong.)
>
> I've deleted the test file in r370175 to unblock development in the meantime.


Fwiw, this seems to me somewhat like a git bug; if I edit the .gitattributes 
file and comment out the problematic line, do a git diff, then uncomment the 
line again, then git diff suddenly shows no changes.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66556/new/

https://reviews.llvm.org/D66556



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66856: [Sema] Suppress -Wformat diagnostics for bool types when printed using %hhd

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm wondering whether this should even warn pedantically. There are no format 
specifiers that apply directly to a `_Bool` datatype, so the user is left 
making a choice as to what specifier fits best. I think `hhd` and `hhu` are 
both equally reasonable types, as are the `d` and `u` groups directly -- I 
don't think we should warn on either of those specifiers with a `_Bool` 
argument. I could maybe see a case for pedantically diagnosing `h`, `l`, and 
`ll` prefixes with `_Bool` because that seems a bit type-confused to me. `c` as 
a specifier seems weird (given that false values will potentially terminate the 
string suddenly depending on terminal behavior, IIRC), but `lc` seems like type 
confusion again.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66856/new/

https://reviews.llvm.org/D66856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66874: [clang-tidy] Fix the potential infinite loop in recordIsTriviallyDefaultConstructible.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/utils/TypeTraits.cpp:57
 return true;
+  // Don't perform the check on an ill-formed Decl. As we will visit every 
class
+  // member recursively, an ill-formed Decl may cause an infinite loop during

I'd suggest to not mention the infinite loop. Just say that it is impossible to 
determine whether the decl is trivially constructible if there was an error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66874/new/

https://reviews.llvm.org/D66874



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66591: [RISCV] Correct Logic around ilp32e macros

[Luís Marques via Phabricator via cfe-commits]

luismarques accepted this revision.
luismarques added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66591/new/

https://reviews.llvm.org/D66591



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 217618.
jvikstrom added a comment.

Added a RecursiveASTVisitor for finding 'underlying' types.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66516/new/

https://reviews.llvm.org/D66516

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -16,6 +16,10 @@
 
 namespace clang {
 namespace clangd {
+void PrintTo(const HighlightingToken &T, ::std::ostream *OS) {
+  *OS << "(" << T.R.start.line << ", " << T.R.start.character << ") -> (" << T.R.end.line << ", " << T.R.end.character << "): " << (int)T.Kind;
+}
+
 namespace {
 
 MATCHER_P(LineNumber, L, "") { return arg.Line == L; }
@@ -225,7 +229,9 @@
 )cpp",
 R"cpp(
   namespace $Namespace[[a]] {
-struct $Class[[A]] {};
+struct $Class[[A]] {
+  $Primitive[[void]] $Method[[foo]]($Class[[A]]*);
+};
 typedef $Primitive[[char]] $Primitive[[C]];
   }
   typedef $Namespace[[a]]::$Class[[A]] $Class[[B]];
@@ -238,6 +244,12 @@
   $Enum[[CD]] $Function[[f]]($Class[[BB]]);
   typedef $Namespace[[a]]::$Primitive[[C]] $Primitive[[PC]];
   typedef $Primitive[[float]] $Primitive[[F]];
+  using $Primitive[[Member]] =
+$Primitive[[void]] (B::*)($Namespace[[a]]::$Class[[A]]*);
+  $Primitive[[void]] $Function[[foo]]($Primitive[[int]], $Class[[B]]);
+  typedef decltype($Function[[foo]]) $Primitive[[fooo]];
+  typedef $Class[[B]] (*$Class[[func]])();
+  typedef $Primitive[[int]] (*$Primitive[[func]])();
 )cpp",
 R"cpp(
   template
@@ -431,6 +443,20 @@
 assert($Variable[[x]] != $Variable[[y]]);
 assert($Variable[[x]] != $Function[[f]]());
   }
+)cpp",
+R"cpp(
+  template
+  class $Class[[A]] {
+using $TemplateParameter[[D]] = $TemplateParameter[[T]];
+using $TemplateParameter[[DD]] = $TemplateParameter[[T]] *;
+using $TemplateParameter[[DDD]] = $TemplateParameter[[T]] &;
+using $TemplateParameter[[B]] = $TemplateParameter[[T]]*[3];
+using $TemplateParameter[[BB]] = $TemplateParameter[[T]]&&;
+using $TemplateParameter[[Member]] =
+  BB (T::*)($Primitive[[int]]);
+using $TemplateParameter[[MemberT]] =
+  $TemplateParameter[[T]]*& (T::*)($Class[[A]]);
+  };
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -14,12 +14,83 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
 #include 
 
 namespace clang {
 namespace clangd {
 namespace {
 
+/// Gets an underlying type in a type hierarchy. Should be used to find the type
+/// in a hieracrchy when the HighlightingTokenCollector's RecursiveASTVisitor
+/// can not recurse to it by default and the addType function is used outside a
+/// Visit*TypeLoc.
+class UnderlyingTypeVisitor
+: public RecursiveASTVisitor {
+  const Type *Underlying;
+
+public:
+  /// Gets an underlying type in the \p T type hierarchy.
+  /// Gets the first type ptr with a Tag decl if one exists.
+  /// Else gets the first Leaf type ptr.
+  /// If neither are found returns the type ptr.
+  const Type *getUnderlyingType(QualType T) {
+Underlying = &(*T);
+TraverseType(T);
+return Underlying;
+  }
+
+  // Check if T has a tag decl and if so stop traversing the type hierachy as we
+  // want to return this.
+  bool VisitType(Type *T) {
+if (T->getAsTagDecl()) {
+  Underlying = T;
+  return false;
+}
+return true;
+  }
+
+  // The default behaviour for RecursiveASTVisitor is to traverse the class type
+  // as well. This makes member pointers become highlighted as classes which is
+  // incorrect.
+  bool TraverseMemberPointerType(MemberPointerType *T) {
+return RecursiveASTVisitor::TraverseType(
+T->getPointeeType());
+  }
+
+  // RecursiveASTVisitor does not traverse the underlying type of a decltype.
+  bool TraverseDecltypeType(DecltypeType *T) {
+return RecursiveASTVisitor::TraverseType(
+T->getUnderlyingType());
+  }
+
+  // RecursiveASTVisitor also traverses parameter and exception types. Only want
+  // return type for this.
+  bool TraverseFunctionProtoType(FunctionProtoType *T) {
+return RecursiveASTVisitor::TraverseType(
+T->getReturnType());
+  }
+
+  /

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 217619.
SureYeaah marked 45 inline comments as done.
SureYeaah added a comment.

Addressed review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -599,6 +599,112 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+  Context = Function;
+
+  // Root statements should have common parent.
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
+  // We don't support extraction from lambdas.
+  EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+
+  // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
+  // lead to break being included in the extraction zone.
+  EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted"));
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+  // Don't extract because needs hoisting.
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
+  // Don't extract return
+  EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this since it's non broken.
+  EXPECT_THAT(apply(" [[for(;;) break;]] "), StartsWith("fail"));
+  EXPECT_THAT(apply(" for(;;) [[continue;]] "), StartsWith("fail"));
+}
+
+TEST_F(ExtractFunctionTest, FileTest) {
+  // Check all parameters are in order
+  std::string ParameterCheckInput = R"cpp(
+struct Foo {
+  int x;
+};
+void f(int a) {
+  int b;
+  int *ptr = &a;
+  Foo foo;
+  [[a += foo.x + b;
+  *ptr++;]]
+})cpp";
+  std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+  int x;
+};
+void extracted(int &a, int &b, int * &ptr, Foo &foo) {
+a += foo.x + b;
+  *ptr++;
+}
+void f(int a) {
+  int b;
+  int *ptr = &a;
+  Foo foo;
+  extracted(a, b, ptr, foo);
+})cpp";
+  EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
+
+  // Check const qualifier
+  std::string ConstCheckInput = R"cpp(
+void f(const int c) {
+  [[while(c) {}]]
+})cpp";
+  std::string ConstCheckOutput = R"cpp(
+void extracted(const int &c) {
+while(c) {}
+}
+void f(const int c) {
+  extracted(c);
+})cpp";
+  EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
+
+  // Don't extract when we need to make a function as a parameter.
+  EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
+
+  // We don't extract from methods for now since they may involve multi-file
+  // edits
+  std::string MethodFailInput = R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+
+  // We don't extract from templated functions for now as templates are hard
+  // to deal with.
+  std::string TemplateFailInput = R"cpp(
+template
+void f() {
+  [[int x;]]
+}
+  )cpp";
+  EXPECT_EQ(apply(TemplateFailInput), "unavailable");
+
+  // FIXME: This should be extractable after selectionTree works correctly for
+  // macros (currently it doesn't select anything for the following case)
+  std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+  )cpp";
+  EXPECT_EQ(apply(MacroFailInput), "unavailable");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,608 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Extracts statements to a new function and replaces the statements with a
+// call to the new function.
+// Before:
+//   void f(int a) {
+// [[if(a < 5)
+//   a = 5;]]
+//   

[PATCH] D66564: [clang-tidy] new FPGA struct pack align check

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

I do not understand why this check is specific to FPGAs.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+else
+  SR.setEnd(ChildFileRange->getEnd());
+  }

kadircet wrote:
> I suppose this relies on the fact that "AST contains the nodes ordered by 
> their begin location"? Could you add an assertion for that?
I've removed the loop, should I still add this?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:181
+// Check if all child nodes of (unselected) Parent are RootStmts.
+bool hasOnlyRootStmtChildren(const Node *Parent) {
+  for (const Node *Child : Parent->Children) {

sammccall wrote:
> kadircet wrote:
> > `hasOnlyRootStmtsAsChildren` ?
> nit: I think this would be clearer as
> `canBeRootStmt(const Node*)` and write the callsite as 
> `llvm::any_of(CommonAnc->Children, canBeRootStmt)`. Up to you though
Replaced with isRootStmt



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+  const SourceManager &SM,

sammccall wrote:
> kadircet wrote:
> > why is this function returning a shared_ptr ?
> avoid shared_ptr unless there's a strong reason. `Optional` 
> seems fine here?
And store a unique_ptr to the optional in ExtractFunction?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+  const SourceManager &SM,

SureYeaah wrote:
> sammccall wrote:
> > kadircet wrote:
> > > why is this function returning a shared_ptr ?
> > avoid shared_ptr unless there's a strong reason. `Optional` 
> > seems fine here?
> And store a unique_ptr to the optional in ExtractFunction?
Because ExtractFunction needs to store a pointer/reference to ExtractionZone 
somehow in prepare and access it in apply.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:270
+  tooling::ExtractionSemicolonPolicy SemicolonPolicy;
+  NewFunction(SourceRange BodyRange, SourceLocation InsertionPoint,
+  tooling::ExtractionSemicolonPolicy SemicolonPolicy,

sammccall wrote:
> this is just initializing public fields, drop the constructor?
> (The callsite is clearer here if you initialize them by name)
`ExtractionSemicolonPolicy` doesn't have a default constructor.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+  if (WithTypeAndQualifiers) {
+if (IsConst)
+  Result += "const ";

kadircet wrote:
> why don't we infer this const from QualType ?
In a future patch we will want to use heuristics like has the variable been 
assigned, was it passed as a non-const reference, was its address taken, etc. 



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+  std::shared_ptr ExtZone;
+};

kadircet wrote:
> why do you need a shared_ptr here?
getExtractionZone creates an Optional which needs to persist 
from prepare to apply. Is there a better way to store a reference to the 
ExtractionZone instance inside ExtractFunction?



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:611
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");

sammccall wrote:
> wait, what?
> expressions that *aren't statements* shouldn't be extracted.
> But what's wrong with this one?
> (Many useful statements are expressions, such as function calls)
For now we don't extract if the selection is an expression. I've added a fixme 
to change that to sub-expressions only.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 217620.
SureYeaah added a comment.

NFC: Whitespace formatting


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -599,6 +599,112 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+  Context = Function;
+
+  // Root statements should have common parent.
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
+  // We don't support extraction from lambdas.
+  EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+
+  // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
+  // lead to break being included in the extraction zone.
+  EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted"));
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+  // Don't extract because needs hoisting.
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
+  // Don't extract return
+  EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this since it's non broken.
+  EXPECT_THAT(apply(" [[for(;;) break;]] "), StartsWith("fail"));
+  EXPECT_THAT(apply(" for(;;) [[continue;]] "), StartsWith("fail"));
+}
+
+TEST_F(ExtractFunctionTest, FileTest) {
+  // Check all parameters are in order
+  std::string ParameterCheckInput = R"cpp(
+struct Foo {
+  int x;
+};
+void f(int a) {
+  int b;
+  int *ptr = &a;
+  Foo foo;
+  [[a += foo.x + b;
+  *ptr++;]]
+})cpp";
+  std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+  int x;
+};
+void extracted(int &a, int &b, int * &ptr, Foo &foo) {
+a += foo.x + b;
+  *ptr++;
+}
+void f(int a) {
+  int b;
+  int *ptr = &a;
+  Foo foo;
+  extracted(a, b, ptr, foo);
+})cpp";
+  EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
+
+  // Check const qualifier
+  std::string ConstCheckInput = R"cpp(
+void f(const int c) {
+  [[while(c) {}]]
+})cpp";
+  std::string ConstCheckOutput = R"cpp(
+void extracted(const int &c) {
+while(c) {}
+}
+void f(const int c) {
+  extracted(c);
+})cpp";
+  EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
+
+  // Don't extract when we need to make a function as a parameter.
+  EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
+
+  // We don't extract from methods for now since they may involve multi-file
+  // edits
+  std::string MethodFailInput = R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+
+  // We don't extract from templated functions for now as templates are hard
+  // to deal with.
+  std::string TemplateFailInput = R"cpp(
+template
+void f() {
+  [[int x;]]
+}
+  )cpp";
+  EXPECT_EQ(apply(TemplateFailInput), "unavailable");
+
+  // FIXME: This should be extractable after selectionTree works correctly for
+  // macros (currently it doesn't select anything for the following case)
+  std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+  )cpp";
+  EXPECT_EQ(apply(MacroFailInput), "unavailable");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,608 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Extracts statements to a new function and replaces the statements with a
+// call to the new function.
+// Before:
+//   void f(int a) {
+// [[if(a < 5)
+//   a = 5;]]
+//   }
+// After:
+//   void extracted(int &a) {

[PATCH] D66876: Indexing: create PP callbacks in the ASTConsumer

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.
gribozavr added a parent revision: D66875: [Index] Marked a bunch of classes 
'final'.

Doing so removes one reason to create a custom FrontendAction.
FrontendActions are not desirable because they are difficult to compose.
ASTConsumers are much easier to compose.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66876

Files:
  clang/lib/Index/IndexingAction.cpp

Index: clang/lib/Index/IndexingAction.cpp
===
--- clang/lib/Index/IndexingAction.cpp
+++ clang/lib/Index/IndexingAction.cpp
@@ -23,7 +23,37 @@
 
 namespace {
 
-class IndexASTConsumer : public ASTConsumer {
+class IndexPPCallbacks final : public PPCallbacks {
+  std::shared_ptr IndexCtx;
+
+public:
+  IndexPPCallbacks(std::shared_ptr IndexCtx)
+  : IndexCtx(std::move(IndexCtx)) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+SourceRange Range, const MacroArgs *Args) override {
+IndexCtx->handleMacroReference(*MacroNameTok.getIdentifierInfo(),
+   Range.getBegin(), *MD.getMacroInfo());
+  }
+
+  void MacroDefined(const Token &MacroNameTok,
+const MacroDirective *MD) override {
+IndexCtx->handleMacroDefined(*MacroNameTok.getIdentifierInfo(),
+ MacroNameTok.getLocation(),
+ *MD->getMacroInfo());
+  }
+
+  void MacroUndefined(const Token &MacroNameTok, const MacroDefinition &MD,
+  const MacroDirective *Undef) override {
+if (!MD.getMacroInfo())  // Ignore noop #undef.
+  return;
+IndexCtx->handleMacroUndefined(*MacroNameTok.getIdentifierInfo(),
+   MacroNameTok.getLocation(),
+   *MD.getMacroInfo());
+  }
+};
+
+class IndexASTConsumer final : public ASTConsumer {
   std::shared_ptr PP;
   std::shared_ptr IndexCtx;
 
@@ -37,6 +67,7 @@
 IndexCtx->setASTContext(Context);
 IndexCtx->getDataConsumer().initialize(Context);
 IndexCtx->getDataConsumer().setPreprocessor(PP);
+PP->addPPCallbacks(std::make_unique(IndexCtx));
   }
 
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
@@ -55,36 +86,6 @@
   }
 };
 
-class IndexPPCallbacks : public PPCallbacks {
-  std::shared_ptr IndexCtx;
-
-public:
-  IndexPPCallbacks(std::shared_ptr IndexCtx)
-  : IndexCtx(std::move(IndexCtx)) {}
-
-  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
-SourceRange Range, const MacroArgs *Args) override {
-IndexCtx->handleMacroReference(*MacroNameTok.getIdentifierInfo(),
-   Range.getBegin(), *MD.getMacroInfo());
-  }
-
-  void MacroDefined(const Token &MacroNameTok,
-const MacroDirective *MD) override {
-IndexCtx->handleMacroDefined(*MacroNameTok.getIdentifierInfo(),
- MacroNameTok.getLocation(),
- *MD->getMacroInfo());
-  }
-
-  void MacroUndefined(const Token &MacroNameTok, const MacroDefinition &MD,
-  const MacroDirective *Undef) override {
-if (!MD.getMacroInfo())  // Ignore noop #undef.
-  return;
-IndexCtx->handleMacroUndefined(*MacroNameTok.getIdentifierInfo(),
-   MacroNameTok.getLocation(),
-   *MD.getMacroInfo());
-  }
-};
-
 class IndexActionBase {
 protected:
   std::shared_ptr DataConsumer;
@@ -101,16 +102,12 @@
IndexCtx);
   }
 
-  std::unique_ptr createIndexPPCallbacks() {
-return std::make_unique(IndexCtx);
-  }
-
   void finish() {
 DataConsumer->finish();
   }
 };
 
-class IndexAction : public ASTFrontendAction, IndexActionBase {
+class IndexAction final : public ASTFrontendAction, IndexActionBase {
 public:
   IndexAction(std::shared_ptr DataConsumer,
   IndexingOptions Opts)
@@ -122,18 +119,13 @@
 return createIndexASTConsumer(CI);
   }
 
-  bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-CI.getPreprocessor().addPPCallbacks(createIndexPPCallbacks());
-return true;
-  }
-
   void EndSourceFileAction() override {
 FrontendAction::EndSourceFileAction();
 finish();
   }
 };
 
-class WrappingIndexAction : public WrapperFrontendAction, IndexActionBase {
+class WrappingIndexAction final : public WrapperFrontendAction, IndexActionBase {
   bool IndexActionFailed = false;
 
 public:
@@ -158,12 +150,6 @@
 return std::make_unique(std::move(Consumers));
   }
 
-  bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-WrapperFrontendAction::BeginSourceFileAction(CI);
-CI.getPreprocessor().addPPCallbacks(createIndexPPCallbacks());
-return true;
-  }
-
   void EndSourceFileAction() override {
   

[PATCH] D66875: [Index] Marked a bunch of classes 'final'

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.
gribozavr added a reviewer: ilya-biryukov.
gribozavr added a child revision: D66876: Indexing: create PP callbacks in the 
ASTConsumer.

This file defines multiple inheritance hierarchies and 'final' helps
with readability.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66875

Files:
  clang/lib/Index/IndexingAction.cpp


Index: clang/lib/Index/IndexingAction.cpp
===
--- clang/lib/Index/IndexingAction.cpp
+++ clang/lib/Index/IndexingAction.cpp
@@ -23,7 +23,7 @@
 
 namespace {
 
-class IndexASTConsumer : public ASTConsumer {
+class IndexASTConsumer final : public ASTConsumer {
   std::shared_ptr PP;
   std::shared_ptr IndexCtx;
 
@@ -55,7 +55,7 @@
   }
 };
 
-class IndexPPCallbacks : public PPCallbacks {
+class IndexPPCallbacks final : public PPCallbacks {
   std::shared_ptr IndexCtx;
 
 public:
@@ -110,7 +110,7 @@
   }
 };
 
-class IndexAction : public ASTFrontendAction, IndexActionBase {
+class IndexAction final : public ASTFrontendAction, IndexActionBase {
 public:
   IndexAction(std::shared_ptr DataConsumer,
   IndexingOptions Opts)
@@ -133,7 +133,7 @@
   }
 };
 
-class WrappingIndexAction : public WrapperFrontendAction, IndexActionBase {
+class WrappingIndexAction final : public WrapperFrontendAction, 
IndexActionBase {
   bool IndexActionFailed = false;
 
 public:


Index: clang/lib/Index/IndexingAction.cpp
===
--- clang/lib/Index/IndexingAction.cpp
+++ clang/lib/Index/IndexingAction.cpp
@@ -23,7 +23,7 @@
 
 namespace {
 
-class IndexASTConsumer : public ASTConsumer {
+class IndexASTConsumer final : public ASTConsumer {
   std::shared_ptr PP;
   std::shared_ptr IndexCtx;
 
@@ -55,7 +55,7 @@
   }
 };
 
-class IndexPPCallbacks : public PPCallbacks {
+class IndexPPCallbacks final : public PPCallbacks {
   std::shared_ptr IndexCtx;
 
 public:
@@ -110,7 +110,7 @@
   }
 };
 
-class IndexAction : public ASTFrontendAction, IndexActionBase {
+class IndexAction final : public ASTFrontendAction, IndexActionBase {
 public:
   IndexAction(std::shared_ptr DataConsumer,
   IndexingOptions Opts)
@@ -133,7 +133,7 @@
   }
 };
 
-class WrappingIndexAction : public WrapperFrontendAction, IndexActionBase {
+class WrappingIndexAction final : public WrapperFrontendAction, IndexActionBase {
   bool IndexActionFailed = false;
 
 public:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.

Doing so removes the last reason to expose a FrontendAction from
libIndex.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66877

Files:
  clang/lib/Index/IndexingAction.cpp

Index: clang/lib/Index/IndexingAction.cpp
===
--- clang/lib/Index/IndexingAction.cpp
+++ clang/lib/Index/IndexingAction.cpp
@@ -23,39 +23,7 @@
 
 namespace {
 
-class IndexASTConsumer : public ASTConsumer {
-  std::shared_ptr PP;
-  std::shared_ptr IndexCtx;
-
-public:
-  IndexASTConsumer(std::shared_ptr PP,
-   std::shared_ptr IndexCtx)
-  : PP(std::move(PP)), IndexCtx(std::move(IndexCtx)) {}
-
-protected:
-  void Initialize(ASTContext &Context) override {
-IndexCtx->setASTContext(Context);
-IndexCtx->getDataConsumer().initialize(Context);
-IndexCtx->getDataConsumer().setPreprocessor(PP);
-  }
-
-  bool HandleTopLevelDecl(DeclGroupRef DG) override {
-return IndexCtx->indexDeclGroupRef(DG);
-  }
-
-  void HandleInterestingDecl(DeclGroupRef DG) override {
-// Ignore deserialized decls.
-  }
-
-  void HandleTopLevelDeclInObjCContainer(DeclGroupRef DG) override {
-IndexCtx->indexDeclGroupRef(DG);
-  }
-
-  void HandleTranslationUnit(ASTContext &Ctx) override {
-  }
-};
-
-class IndexPPCallbacks : public PPCallbacks {
+class IndexPPCallbacks final : public PPCallbacks {
   std::shared_ptr IndexCtx;
 
 public:
@@ -85,32 +53,66 @@
   }
 };
 
-class IndexActionBase {
-protected:
+class IndexASTConsumer final : public ASTConsumer {
   std::shared_ptr DataConsumer;
   std::shared_ptr IndexCtx;
+  std::shared_ptr PP;
 
-  IndexActionBase(std::shared_ptr dataConsumer,
-  IndexingOptions Opts)
-  : DataConsumer(std::move(dataConsumer)),
-IndexCtx(new IndexingContext(Opts, *DataConsumer)) {}
+public:
+  IndexASTConsumer(std::shared_ptr DataConsumer,
+   const IndexingOptions &Opts,
+   std::shared_ptr PP)
+  : DataConsumer(std::move(DataConsumer)),
+IndexCtx(new IndexingContext(Opts, *this->DataConsumer)),
+PP(std::move(PP)) {
+assert(this->DataConsumer != nullptr);
+assert(this->PP != nullptr);
+  }
 
-  std::unique_ptr
-  createIndexASTConsumer(CompilerInstance &CI) {
-return std::make_unique(CI.getPreprocessorPtr(),
-   IndexCtx);
+protected:
+  void Initialize(ASTContext &Context) override {
+IndexCtx->setASTContext(Context);
+IndexCtx->getDataConsumer().initialize(Context);
+IndexCtx->getDataConsumer().setPreprocessor(PP);
+PP->addPPCallbacks(std::make_unique(IndexCtx));
+  }
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+return IndexCtx->indexDeclGroupRef(DG);
   }
 
-  std::unique_ptr createIndexPPCallbacks() {
-return std::make_unique(IndexCtx);
+  void HandleInterestingDecl(DeclGroupRef DG) override {
+// Ignore deserialized decls.
   }
 
-  void finish() {
+  void HandleTopLevelDeclInObjCContainer(DeclGroupRef DG) override {
+IndexCtx->indexDeclGroupRef(DG);
+  }
+
+  void HandleTranslationUnit(ASTContext &Ctx) override {
 DataConsumer->finish();
   }
 };
 
-class IndexAction : public ASTFrontendAction, IndexActionBase {
+class IndexActionBase {
+protected:
+  std::shared_ptr DataConsumer;
+  IndexingOptions Opts;
+
+  IndexActionBase(std::shared_ptr DataConsumer,
+  IndexingOptions Opts)
+  : DataConsumer(std::move(DataConsumer)), Opts(Opts) {
+assert(this->DataConsumer != nullptr);
+  }
+
+  std::unique_ptr
+  createIndexASTConsumer(CompilerInstance &CI) {
+return std::make_unique(DataConsumer, Opts,
+  CI.getPreprocessorPtr());
+  }
+};
+
+class IndexAction final : public ASTFrontendAction, IndexActionBase {
 public:
   IndexAction(std::shared_ptr DataConsumer,
   IndexingOptions Opts)
@@ -121,21 +123,10 @@
  StringRef InFile) override {
 return createIndexASTConsumer(CI);
   }
-
-  bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-CI.getPreprocessor().addPPCallbacks(createIndexPPCallbacks());
-return true;
-  }
-
-  void EndSourceFileAction() override {
-FrontendAction::EndSourceFileAction();
-finish();
-  }
 };
 
-class WrappingIndexAction : public WrapperFrontendAction, IndexActionBase {
-  bool IndexActionFailed = false;
-
+class WrappingIndexAction final : public WrapperFrontendAction,
+  IndexActionBase {
 public:
   WrappingIndexAction(std::unique_ptr WrappedAction,
   std::shared_ptr DataConsumer,
@@ -148,7 +139,6 @@
  StringRef InFile) override {
 auto OtherConsumer = WrapperFrontendAction::CreateASTConsumer(CI, InFile);
 if (!OtherConsu

[PATCH] D66878: [Index] Stopped wrapping FrontendActions in libIndex and its users

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous.
Herald added a project: clang.
gribozavr added reviewers: ilya-biryukov, jkorous.
Herald added a subscriber: dexonsmith.
gribozavr added a parent revision: D66877: Moved the IndexDataConsumer::finish 
call into the IndexASTConsumer from IndexAction.
gribozavr added a child revision: D66879: [Index] Added a 
ShouldSkipFunctionBody callback to libIndex, and refactored clients to use it 
instead of inventing their own solution.

Exposed a new function, createIndexingASTConsumer, that creates an
ASTConsumer. ASTConsumers compose well.

Removed wrapping functionality from createIndexingAction.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66878

Files:
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Index/IndexingAction.h
  clang/lib/Index/IndexingAction.cpp
  clang/tools/c-index-test/core_main.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/MultiplexConsumer.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/HeaderSearch.h"
@@ -367,14 +368,16 @@
 
 class IndexingFrontendAction : public ASTFrontendAction {
   std::shared_ptr DataConsumer;
+  IndexingOptions Opts;
 
   SharedParsedRegionsStorage *SKData;
   std::unique_ptr ParsedLocsTracker;
 
 public:
   IndexingFrontendAction(std::shared_ptr dataConsumer,
+ const IndexingOptions &Opts,
  SharedParsedRegionsStorage *skData)
-  : DataConsumer(std::move(dataConsumer)), SKData(skData) {}
+  : DataConsumer(std::move(dataConsumer)), Opts(Opts), SKData(skData) {}
 
   std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
  StringRef InFile) override {
@@ -398,8 +401,12 @@
   std::make_unique(*SKData, *PPRec, PP);
 }
 
-return std::make_unique(*DataConsumer,
-  ParsedLocsTracker.get());
+std::vector> Consumers;
+Consumers.push_back(std::make_unique(
+*DataConsumer, ParsedLocsTracker.get()));
+Consumers.push_back(
+createIndexingASTConsumer(DataConsumer, Opts, CI.getPreprocessorPtr()));
+return std::make_unique(std::move(Consumers));
   }
 
   TranslationUnitKind getTranslationUnitKind() override {
@@ -569,12 +576,9 @@
   auto DataConsumer =
 std::make_shared(client_data, CB, index_options,
   CXTU->getTU());
-  auto InterAction = std::make_unique(DataConsumer,
- SkipBodies ? IdxSession->SkipBodyData.get() : nullptr);
-  std::unique_ptr IndexAction;
-  IndexAction = createIndexingAction(DataConsumer,
-getIndexingOptionsFromCXOptions(index_options),
- std::move(InterAction));
+  auto IndexAction = std::make_unique(
+  DataConsumer, getIndexingOptionsFromCXOptions(index_options),
+  SkipBodies ? IdxSession->SkipBodyData.get() : nullptr);
 
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar
@@ -995,4 +999,3 @@
   *static_cast(location.ptr_data[0]);
   return cxloc::translateSourceLocation(DataConsumer.getASTContext(), Loc);
 }
-
Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -221,9 +221,8 @@
   auto DataConsumer = std::make_shared(OS);
   IndexingOptions IndexOpts;
   IndexOpts.IndexFunctionLocals = indexLocals;
-  std::unique_ptr IndexAction;
-  IndexAction = createIndexingAction(DataConsumer, IndexOpts,
- /*WrappedAction=*/nullptr);
+  std::unique_ptr IndexAction =
+  createIndexingAction(DataConsumer, IndexOpts);
 
   auto PCHContainerOps = std::make_shared();
   std::unique_ptr Unit(ASTUnit::LoadFromCompilerInvocationAction(
Index: clang/lib/Index/IndexingAction.cpp
===
--- clang/lib/Index/IndexingAction.cpp
+++ clang/lib/Index/IndexingAction.cpp
@@ -23,39 +23,7 @@
 
 namespace {
 
-class IndexASTConsumer : public ASTConsumer {
-  std::shared_ptr PP;
-  std::shared_ptr IndexCtx;
-
-public:
-  IndexASTConsumer(std::shared_ptr PP,
-   std::shared_ptr IndexCtx)
-  : PP(std::move(PP)), IndexCtx(std::move(IndexCtx)) {}
-
-protected:
-  void Initialize(ASTContext &Context) override {
-IndexCtx-

[PATCH] D66879: [Index] Added a ShouldSkipFunctionBody callback to libIndex, and refactored clients to use it instead of inventing their own solution

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous.
Herald added a project: clang.
gribozavr added reviewers: ilya-biryukov, jkorous.
Herald added a subscriber: dexonsmith.
gribozavr added a parent revision: D66878: [Index] Stopped wrapping 
FrontendActions in libIndex and its users.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66879

Files:
  clang-tools-extra/clangd/index/IndexAction.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/include/clang/Index/IndexingAction.h
  clang/lib/Index/IndexingAction.cpp
  clang/tools/c-index-test/core_main.cpp
  clang/tools/libclang/Indexing.cpp

Index: clang/tools/libclang/Indexing.cpp
===
--- clang/tools/libclang/Indexing.cpp
+++ clang/tools/libclang/Indexing.cpp
@@ -19,6 +19,7 @@
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/MultiplexConsumer.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/HeaderSearch.h"
@@ -296,54 +297,20 @@
 
 class IndexingConsumer : public ASTConsumer {
   CXIndexDataConsumer &DataConsumer;
-  ParsedSrcLocationsTracker *ParsedLocsTracker;
 
 public:
   IndexingConsumer(CXIndexDataConsumer &dataConsumer,
ParsedSrcLocationsTracker *parsedLocsTracker)
-  : DataConsumer(dataConsumer), ParsedLocsTracker(parsedLocsTracker) {}
-
-  // ASTConsumer Implementation
+  : DataConsumer(dataConsumer) {}
 
   void Initialize(ASTContext &Context) override {
 DataConsumer.setASTContext(Context);
 DataConsumer.startedTranslationUnit();
   }
 
-  void HandleTranslationUnit(ASTContext &Ctx) override {
-if (ParsedLocsTracker)
-  ParsedLocsTracker->syncWithStorage();
-  }
-
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
 return !DataConsumer.shouldAbort();
   }
-
-  bool shouldSkipFunctionBody(Decl *D) override {
-if (!ParsedLocsTracker) {
-  // Always skip bodies.
-  return true;
-}
-
-const SourceManager &SM = DataConsumer.getASTContext().getSourceManager();
-SourceLocation Loc = D->getLocation();
-if (Loc.isMacroID())
-  return false;
-if (SM.isInSystemHeader(Loc))
-  return true; // always skip bodies from system headers.
-
-FileID FID;
-unsigned Offset;
-std::tie(FID, Offset) = SM.getDecomposedLoc(Loc);
-// Don't skip bodies from main files; this may be revisited.
-if (SM.getMainFileID() == FID)
-  return false;
-const FileEntry *FE = SM.getFileEntryForID(FID);
-if (!FE)
-  return false;
-
-return ParsedLocsTracker->hasAlredyBeenParsed(Loc, FID, FE);
-  }
 };
 
 //===--===//
@@ -367,14 +334,16 @@
 
 class IndexingFrontendAction : public ASTFrontendAction {
   std::shared_ptr DataConsumer;
+  IndexingOptions Opts;
 
   SharedParsedRegionsStorage *SKData;
   std::unique_ptr ParsedLocsTracker;
 
 public:
   IndexingFrontendAction(std::shared_ptr dataConsumer,
+ const IndexingOptions &Opts,
  SharedParsedRegionsStorage *skData)
-  : DataConsumer(std::move(dataConsumer)), SKData(skData) {}
+  : DataConsumer(std::move(dataConsumer)), Opts(Opts), SKData(skData) {}
 
   std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
  StringRef InFile) override {
@@ -398,8 +367,39 @@
   std::make_unique(*SKData, *PPRec, PP);
 }
 
-return std::make_unique(*DataConsumer,
-  ParsedLocsTracker.get());
+std::vector> Consumers;
+Consumers.push_back(std::make_unique(
+*DataConsumer, ParsedLocsTracker.get()));
+Consumers.push_back(createIndexingASTConsumer(
+DataConsumer, Opts, CI.getPreprocessorPtr(),
+[this](const Decl *D) { return this->shouldSkipFunctionBody(D); }));
+return std::make_unique(std::move(Consumers));
+  }
+
+  bool shouldSkipFunctionBody(const Decl *D) {
+if (!ParsedLocsTracker) {
+  // Always skip bodies.
+  return true;
+}
+
+const SourceManager &SM = D->getASTContext().getSourceManager();
+SourceLocation Loc = D->getLocation();
+if (Loc.isMacroID())
+  return false;
+if (SM.isInSystemHeader(Loc))
+  return true; // always skip bodies from system headers.
+
+FileID FID;
+unsigned Offset;
+std::tie(FID, Offset) = SM.getDecomposedLoc(Loc);
+// Don't skip bodies from main files; this may be revisited.
+if (SM.getMainFileID() == FID)
+  return false;
+const FileEntry *FE = SM.getFileEntryForID(FID);
+if (!FE)
+  return false;
+
+return ParsedLocsTracker->hasAlredyBeenParsed(Loc, FID, FE);
   }
 
   TranslationUnitKind getTranslationUnitKind() override {
@@ -409,6 +409,1

[PATCH] D66875: [Index] Marked a bunch of classes 'final'

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66875/new/

https://reviews.llvm.org/D66875



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66876: Indexing: create PP callbacks in the ASTConsumer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66876/new/

https://reviews.llvm.org/D66876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Looks good, I just have a comment about the matcher.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:1434
 
+AST_MATCHER_P(RecordDecl, hasFieldIndirectFieldOrder, std::vector,
+  Order) {

This name sounds strange for me, perhaps `hasAnyKindOfFieldOrder` ?
Also, this matcher I think supersedes the `hasFieldOrder`, I mean in the tests 
where we use `hasFieldOrder` we could use this new matcher too, can't we? In 
that case, however, we can delete the old implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66866/new/

https://reviews.llvm.org/D66866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66874: [clang-tidy] Fix the potential infinite loop in recordIsTriviallyDefaultConstructible.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 217628.
hokein marked an inline comment as done.
hokein added a comment.

Tweak the comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66874/new/

https://reviews.llvm.org/D66874

Files:
  clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
  
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp


Index: 
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s 
cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' 
[clang-diagnostic-error]
+  int a = 10;
+};
Index: clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,10 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // It is impossible to detemine whether an ill-formed decl is trivially
+  // constructible.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())


Index: clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' [clang-diagnostic-error]
+  int a = 10;
+};
Index: clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,10 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // It is impossible to detemine whether an ill-formed decl is trivially
+  // constructible.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66516: [clangd] Added highlighting to types dependant on templates.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom marked an inline comment as done.
jvikstrom added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:177
   return;
+if (TP->isPointerType() || TP->isLValueReferenceType())
+  // When highlighting dependant template types the type can be a pointer 
or

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > jvikstrom wrote:
> > > ilya-biryukov wrote:
> > > > jvikstrom wrote:
> > > > > ilya-biryukov wrote:
> > > > > > jvikstrom wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > `RecursiveASTVisitor` also traverses the pointer and reference 
> > > > > > > > types, why does it not reach the inner `TemplateTypeParmType` 
> > > > > > > > in the cases you describe?
> > > > > > > The D in `using D = ...` `typedef ... D` does not have a TypeLoc 
> > > > > > > (at least not one that is visited). Therefore we use the 
> > > > > > > VisitTypedefNameDecl (line 121) to get the location of `D` to be 
> > > > > > > able to highlight it. And we just send the typeLocs typeptr to 
> > > > > > > addType (which is a Pointer for `using D = T*;`)...
> > > > > > > 
> > > > > > > But maybe we should get the underlying type before we call 
> > > > > > > addType with TypePtr? Just a while loop on line 123 basically 
> > > > > > > (can we have multiple PointerTypes nested in each other actually?)
> > > > > > > 
> > > > > > > Even if we keep it in addType the comment is actually wrong, 
> > > > > > > because it obviously works when for the actual "type occurrences" 
> > > > > > > for `D` (so will fix that no matter what). This recursion will 
> > > > > > > just make us add more duplicate tokens...
> > > > > > Could we investigate why `RecursiveASTVisitor` does not visit the 
> > > > > > `TypeLoc` of a corresponding decl?
> > > > > > Here's the code from `RecursiveASTVisitor.h` that should do the 
> > > > > > trick:
> > > > > > ```
> > > > > > DEF_TRAVERSE_DECL(TypeAliasDecl, {
> > > > > >   TRY_TO(TraverseTypeLoc(D->getTypeSourceInfo()->getTypeLoc()));
> > > > > >   // We shouldn't traverse D->getTypeForDecl(); it's a result of
> > > > > >   // declaring the type alias, not something that was written in the
> > > > > >   // source.
> > > > > > })
> > > > > > ```
> > > > > > 
> > > > > > If it doesn't, we are probably holding it wrong.
> > > > > There just doesn't seem to be a TypeLoc for the typedef'ed Decl.  We 
> > > > > can get the `T*` TypeLoc (with 
> > > > > `D->getTypeSourceInfo()->getTypeLoc()`). But there isn't one for `D`. 
> > > > > Even the `D->getTypeForDecl` returns null.
> > > > > 
> > > > > And I have no idea where I'd even start debugging that. Or if it's 
> > > > > even a bug
> > > > > 
> > > > I may have misinterpreted the patch. Are we trying to add highlightings 
> > > > for the names of using aliases here? E.g. for the following range:
> > > > ```
> > > > template 
> > > > struct Foo {
> > > >   using [[D]] = T**;
> > > > };
> > > > ```
> > > > 
> > > > Why isn't this handled in `VisitNamedDecl`?
> > > > We don't seem to call this function for `TypedefNameDecl` at all and it 
> > > > actually weird. Is this because we attempt to highlight typedefs as 
> > > > their underlying types?
> > > So currently using aliases and typedefs are highlighted the same as the 
> > > underlying type (in most cases). One case where they aren't is when the 
> > > underlying type is a template parameter (which is what this patch is 
> > > trying to solve).
> > > 
> > > 
> > > > Why isn't this handled in VisitNamedDecl?
> > > 
> > > The Decl is actually visited in `VisitNamedDecl`, however as it is a 
> > > `TypeAliasDecl` which we do not have a check for in the addToken function 
> > > it will not get highlighted in that visit.
> > > 
> > > Actually, could add a check for `TypeAliasDecl` in `addToken` (should 
> > > probably be a check for `TypedefNameDecl` to cover both `using ...` and 
> > > `typedef ...`) and move the code from the `VisitTypedefNameDecl` to the 
> > > `addToken` function inside that check instead.
> > > 
> > > 
> > > 
> > > > We don't seem to call this function for TypedefNameDecl at all and it 
> > > > actually weird. Is this because we attempt to highlight typedefs as 
> > > > their underlying types?
> > > 
> > > 
> > > Don't understand what you mean. What function? 
> > > So currently using aliases and typedefs are highlighted the same as the 
> > > underlying type (in most cases). 
> > Thanks for clarifying this. This is where my confusion is coming from.
> > A few question to try understanding the approach taken (sorry if that's too 
> > detailed, I am probably missing the context here)
> > - What do we fallback to? From my reading of the code, we do not highlight 
> > them at all if the underlying type is not one of the predefined cases.
> > - Why are pointers and **l-value** references special? What about arrays, 
> > r-value references, function types, pack expansions, etc.?
> > 
> > > Don't understand what you mean. What 

[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/lib/Index/IndexingAction.cpp:77
+IndexCtx->getDataConsumer().setPreprocessor(PP);
+PP->addPPCallbacks(std::make_unique(IndexCtx));
+  }

The fact that we call `addPPCallbacks` **after** creating `ASTContext` is a bit 
concerning.

This wouldn't probably cause any problems in practice, but generally changing 
preprocessor by the time `ASTContext` is already created seems dangerous (what 
if something there ever starts depending on the state of the preprocessor?)

I think this concern is easily elevated if we change Preprocessor and call 
addPPCallbacks on `CreateASTConsumer(CompilerInvocation& CI)`. That would mean 
we have a separate function that sets up the preprocessor and creates 
`IndexASTConsumer` and it should be exposed to the clients.

Have you considered this approach? Would it work?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66877/new/

https://reviews.llvm.org/D66877



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r370200 - [clang-tidy] Fix the potential infinite loop in recordIsTriviallyDefaultConstructible.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Aug 28 06:45:53 2019
New Revision: 370200

URL: http://llvm.org/viewvc/llvm-project?rev=370200&view=rev
Log:
[clang-tidy] Fix the potential infinite loop in 
recordIsTriviallyDefaultConstructible.

Summary:
The recordIsTriviallyDefaultConstructible may cause an infinite loop when
running on an ill-formed decl.

Reviewers: gribozavr

Subscribers: nemanjai, xazax.hun, kbarton, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66874

Added:

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp

Modified: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp?rev=370200&r1=370199&r2=370200&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp Wed Aug 28 06:45:53 
2019
@@ -54,6 +54,10 @@ bool recordIsTriviallyDefaultConstructib
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // It is impossible to detemine whether an ill-formed decl is trivially
+  // constructible.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())

Added: 
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp?rev=370200&view=auto
==
--- 
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
 (added)
+++ 
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
 Wed Aug 28 06:45:53 2019
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s 
cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' 
[clang-diagnostic-error]
+  int a = 10;
+};


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66874: [clang-tidy] Fix the potential infinite loop in recordIsTriviallyDefaultConstructible.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370200: [clang-tidy] Fix the potential infinite loop in… 
(authored by hokein, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66874?vs=217628&id=217629#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66874/new/

https://reviews.llvm.org/D66874

Files:
  clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
  
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp


Index: 
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- 
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
+++ 
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s 
cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' 
[clang-diagnostic-error]
+  int a = 10;
+};
Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,10 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // It is impossible to detemine whether an ill-formed decl is trivially
+  // constructible.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())


Index: clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-type-member-init-no-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %check_clang_tidy -expect-clang-tidy-error %s cppcoreguidelines-pro-type-member-init %t
+
+struct X {
+  X x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: error: field has incomplete type 'X' [clang-diagnostic-error]
+  int a = 10;
+};
Index: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
===
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
@@ -54,6 +54,10 @@
   // Non-C++ records are always trivially constructible.
   if (!ClassDecl)
 return true;
+  // It is impossible to detemine whether an ill-formed decl is trivially
+  // constructible.
+  if (RecordDecl.isInvalidDecl())
+return false;
   // A class with a user-provided default constructor is not trivially
   // constructible.
   if (ClassDecl->hasUserProvidedDefaultConstructor())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r370202 - [clangd] Cleans up the semantic highlighting resources if clangd stops.

[Johan Vikstrom via cfe-commits]

Author: jvikstrom
Date: Wed Aug 28 06:46:22 2019
New Revision: 370202

URL: http://llvm.org/viewvc/llvm-project?rev=370202&view=rev
Log:
[clangd] Cleans up the semantic highlighting resources if clangd stops.

Summary: Disposes of the vscode listeners when clangd crashes and reuses the 
old highlighter when it restarts. The reason for reusing the highlighter is 
because this way the highlightings will not disappear as we won't have to 
dispose of them.

Reviewers: hokein, ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66743

Modified:
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts

clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts

clang-tools-extra/trunk/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Modified: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts?rev=370202&r1=370201&r2=370202&view=diff
==
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts 
(original)
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts Wed 
Aug 28 06:46:22 2019
@@ -112,14 +112,6 @@ export function activate(context: vscode
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
   clangdClient.registerFeature(semanticHighlightingFeature);
-  // The notification handler must be registered after the client is ready or
-  // the client will crash.
-  clangdClient.onReady().then(
-  () => clangdClient.onNotification(
-  semanticHighlighting.NotificationType,
-  semanticHighlightingFeature.handleNotification.bind(
-  semanticHighlightingFeature)));
-
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
   context.subscriptions.push(vscode.commands.registerCommand(
@@ -149,9 +141,14 @@ export function activate(context: vscode
   clangdClient.onNotification(
   'textDocument/clangd.fileStatus',
   (fileStatus) => { status.onFileUpdated(fileStatus); });
+  clangdClient.onNotification(
+  semanticHighlighting.NotificationType,
+  semanticHighlightingFeature.handleNotification.bind(
+  semanticHighlightingFeature));
 } else if (newState == vscodelc.State.Stopped) {
   // Clear all cached statuses when clangd crashes.
   status.clear();
+  semanticHighlightingFeature.dispose();
 }
   })
   // An empty place holder for the activate command, otherwise we'll get an

Modified: 
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts?rev=370202&r1=370201&r2=370202&view=diff
==
--- 
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
 (original)
+++ 
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
 Wed Aug 28 06:46:22 2019
@@ -56,6 +56,8 @@ export class SemanticHighlightingFeature
   scopeLookupTable: string[][];
   // The object that applies the highlightings clangd sends.
   highlighter: Highlighter;
+  // Any disposables that should be cleaned up when clangd crashes.
+  private subscriptions: vscode.Disposable[] = [];
   fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
 // Extend the ClientCapabilities type and add semantic highlighting
 // capability to the object.
@@ -88,14 +90,15 @@ export class SemanticHighlightingFeature
 // otherwise it could try to update the themeRuleMatcher without the
 // highlighter being created.
 this.highlighter = new Highlighter(this.scopeLookupTable);
+this.subscriptions.push(vscode.Disposable.from(this.highlighter));
 this.loadCurrentTheme();
 // Event handling for handling with TextDocuments/Editors lifetimes.
-vscode.window.onDidChangeVisibleTextEditors(
+this.subscriptions.push(vscode.window.onDidChangeVisibleTextEditors(
 (editors: vscode.TextEditor[]) =>
 editors.forEach((e) => this.highlighter.applyHighlights(
-e.document.uri.toString(;
-vscode.workspace.onDidCloseTextDocument(
-(doc) => this.highlighter.removeFileHighlightings(doc.uri.toString()));
+e.document.uri.toString();
+this.subscriptions.push(vscode.workspace.onDidCloseTextDocument(
+(doc) => 
this.highlighter.removeFileHighlightings(doc.uri.toString(;
   }
 
   handleNotification(params: SemanticHighlightingParams) {
@@ -103,6 +106,11 @@ export class Sema

[PATCH] D66743: [clangd] Cleans up the semantic highlighting resources if clangd crashes.

[Johan Vikström via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370202: [clangd] Cleans up the semantic highlighting 
resources if clangd stops. (authored by jvikstrom, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66743?vs=217396&id=217630#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66743/new/

https://reviews.llvm.org/D66743

Files:
  clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
  
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/trunk/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -56,6 +56,8 @@
   scopeLookupTable: string[][];
   // The object that applies the highlightings clangd sends.
   highlighter: Highlighter;
+  // Any disposables that should be cleaned up when clangd crashes.
+  private subscriptions: vscode.Disposable[] = [];
   fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
 // Extend the ClientCapabilities type and add semantic highlighting
 // capability to the object.
@@ -88,14 +90,15 @@
 // otherwise it could try to update the themeRuleMatcher without the
 // highlighter being created.
 this.highlighter = new Highlighter(this.scopeLookupTable);
+this.subscriptions.push(vscode.Disposable.from(this.highlighter));
 this.loadCurrentTheme();
 // Event handling for handling with TextDocuments/Editors lifetimes.
-vscode.window.onDidChangeVisibleTextEditors(
+this.subscriptions.push(vscode.window.onDidChangeVisibleTextEditors(
 (editors: vscode.TextEditor[]) =>
 editors.forEach((e) => this.highlighter.applyHighlights(
-e.document.uri.toString(;
-vscode.workspace.onDidCloseTextDocument(
-(doc) => this.highlighter.removeFileHighlightings(doc.uri.toString()));
+e.document.uri.toString();
+this.subscriptions.push(vscode.workspace.onDidCloseTextDocument(
+(doc) => this.highlighter.removeFileHighlightings(doc.uri.toString(;
   }
 
   handleNotification(params: SemanticHighlightingParams) {
@@ -103,6 +106,11 @@
 (line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
 this.highlighter.highlight(params.textDocument.uri, lines);
   }
+  // Disposes of all disposable resources used by this object.
+  public dispose() {
+this.subscriptions.forEach((d) => d.dispose());
+this.subscriptions = [];
+  }
 }
 
 // Converts a string of base64 encoded tokens into the corresponding array of
@@ -138,6 +146,13 @@
   constructor(scopeLookupTable: string[][]) {
 this.scopeLookupTable = scopeLookupTable;
   }
+  public dispose() {
+this.files.clear();
+this.decorationTypes.forEach((t) => t.dispose());
+// Dispose must not be not called multiple times if initialize is
+// called again.
+this.decorationTypes = [];
+  }
   // This function must be called at least once or no highlightings will be
   // done. Sets the theme that is used when highlighting. Also triggers a
   // recolorization for all current highlighters. Should be called whenever the
@@ -174,6 +189,27 @@
 this.applyHighlights(fileUri);
   }
 
+  // Applies all the highlightings currently stored for a file with fileUri.
+  public applyHighlights(fileUri: string) {
+if (!this.files.has(fileUri))
+  // There are no highlightings for this file, must return early or will get
+  // out of bounds when applying the decorations below.
+  return;
+if (!this.decorationTypes.length)
+  // Can't apply any decorations when there is no theme loaded.
+  return;
+// This must always do a full re-highlighting due to the fact that
+// TextEditorDecorationType are very expensive to create (which makes
+// incremental updates infeasible). For this reason one
+// TextEditorDecorationType is used per scope.
+const ranges = this.getDecorationRanges(fileUri);
+vscode.window.visibleTextEditors.forEach((e) => {
+  if (e.document.uri.toString() !== fileUri)
+return;
+  this.decorationTypes.forEach((d, i) => e.setDecorations(d, ranges[i]));
+});
+  }
+
   // Called when a text document is closed. Removes any highlighting entries for
   // the text document that was closed.
   public removeFileHighlightings(fileUri: string) {
@@ -207,27 +243,6 @@
 });
 return decorations;
   }
-
-  // Applies all the highlightings currently stored for a file with fileUri.
-  public applyHighlights(fileUri: stri

[PATCH] D66556: [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D66556#1648807 , @aganea wrote:

> In D66556#1648591 , @hans wrote:
>
> > In D66556#1648118 , @dexonsmith 
> > wrote:
> >
> > > In D66556#1648109 , @rnk wrote:
> > >
> > > > I'm not sure what happens, but I see you added .gitattributes. I'd 
> > > > commit it as is. Buildbots using svn will keep working. You can check 
> > > > that the monorepo has the right line endings afterwards, and try again 
> > > > if not.
> > >
> > >
> > > SGTM.
> >
> >
> > This broke users of the monorepo, where the file would show up as having 
> > changed locally, and with no way to reset it. I'm guessing that's because 
> > it was checked in with LF endings and then because of the .gitattributes 
> > file, it changes at checkout. I think the correct solution would be to 
> > check in the file with CRLF endings and not set any attributes or stuff. 
> > (Though this is all super confusing and I might have got it wrong.)
> >
> > I've deleted the test file in r370175 to unblock development in the 
> > meantime.
>
>
> It already had CRLF endings locally before commit.
>  It is strange, the file shows up as having CRLF endings in the old revision 
> in git, before the revert: 
> https://raw.githubusercontent.com/llvm/llvm-project/9774a2ba279aea35f166b8ca489d0e8292026c38/clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
>  The same kind of problem occured in rL311683 
> , later fixed by rL311732 
> .
>  If you're fine with that, I'll do the same thing: remove `.gitattributes` 
> and just re-commit the file 
> `minimize_source_to_dependency_directives_invalid_error.c` with 
> `svn:eol-style CRLF`.


Yes, I think that should work. (But these things always break in surprising 
ways.)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66556/new/

https://reviews.llvm.org/D66556



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66878: [Index] Stopped wrapping FrontendActions in libIndex and its users

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Overall LG, thanks! Not sure why we need to keep `IndexingOptions` everywhere, 
though, see the relevant comment.




Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:217
   std::unique_ptr Includes;
+  index::IndexingOptions Opts;
   std::unique_ptr PragmaHandler;

Are these option ever used? Do we need to keep them alive for the lifetime of 
the action?
Might be worth a comment.



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:227
+  std::shared_ptr DataConsumer;
+  index::IndexingOptions Opts;
   CommentHandler *PragmaHandler;

Same here, we do not seem to use `Opts`, but still store them. To keep them 
alive?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66878/new/

https://reviews.llvm.org/D66878



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 217631.
jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

Update according to comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64375/new/

https://reviews.llvm.org/D64375

Files:
  clang/docs/OpenMPSupport.rst

Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -2,12 +2,12 @@
 
   
 .none { background-color: #FF }
-.partial { background-color: #99 }
+.part { background-color: #99 }
 .good { background-color: #CCFF99 }
   
 
 .. role:: none
-.. role:: partial
+.. role:: part
 .. role:: good
 
 .. contents::
@@ -17,7 +17,7 @@
 OpenMP Support
 ==
 
-Clang supports the following OpenMP 5.0 features
+Clang supports the following OpenMP 5.0 features (see also `OpenMP implementation details`_):
 
 * The `reduction`-based clauses in the `task` and `target`-based directives.
 
@@ -37,7 +37,7 @@
 Clang fully supports OpenMP 4.5. Clang supports offloading to X86_64, AArch64,
 PPC64[LE] and has `basic support for Cuda devices`_.
 
-* #pragma omp declare simd: :partial:`Partial`.  We support parsing/semantic
+* #pragma omp declare simd: :part:`Partial`.  We support parsing/semantic
   analysis + generation of special attributes for X86 target, but still
   missing the LLVM pass for vectorization.
 
@@ -129,3 +129,132 @@
   In some cases the local variables are actually allocated in the global memory,
   but the debug info may be not aware of it.
 
+
+.. _OpenMP implementation details:
+
+OpenMP 5.0 Implementation Details
+-
+
+The following table provides a quick overview over various OpenMP 5.0 features
+and their implementation status. Please contact *openmp-dev* at
+*lists.llvm.org* for more information or if you want to help with the
+implementation.
+
++--+--+--++
+|Category  | Feature  | Status   | Reviews|
++==+==+==++
+| loop extension   | support != in the canonical loop form| :good:`done` | D54441 |
++--+--+--++
+| loop extension   | #pragma omp loop (directive) | :none:`unclaimed`||
++--+--+--++
+| loop extension   | collapse imperfectly nested loop | :none:`unclaimed`||
++--+--+--++
+| loop extension   | collapse non-rectangular nested loop | :part:`worked on`||
++--+--+--++
+| loop extension   | C++ range-base for loop  | :none:`unclaimed`||
++--+--+--++
+| loop extension   | clause: nosimd for SIMD directives   | :none:`unclaimed`||
++--+--+--++
+| loop extension   | inclusive scan extension (matching C++17 PSTL)   | :none:`unclaimed`||
++--+--+--++
+| memory mangagement   | memory allocators| :good:`done` | r341687,r357929

[PATCH] D66881: [clangd][vscode] Don't leak the resources

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: jvikstrom.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.

We miss a few places where we need to add them to the subscriptions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66881

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -133,9 +133,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({ newState }) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +151,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -133,9 +133,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({ newState }) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +151,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66406: [clangd] Update themeRuleMatcher when color theme changes in vscode extension.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 217638.
jvikstrom added a comment.

Updated to new master.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66406/new/

https://reviews.llvm.org/D66406

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts


Index: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -5,6 +5,11 @@
 import * as vscodelc from 'vscode-languageclient';
 import * as vscodelct from 'vscode-languageserver-types';
 
+function getCurrentThemeName() {
+  return vscode.workspace.getConfiguration('workbench')
+  .get('colorTheme');
+}
+
 // Parameters for the semantic highlighting (server-side) push notification.
 // Mirrors the structure in the semantic highlighting proposal for LSP.
 interface SemanticHighlightingParams {
@@ -56,6 +61,8 @@
   scopeLookupTable: string[][];
   // The object that applies the highlightings clangd sends.
   highlighter: Highlighter;
+  // The current color theme used for colorization.
+  private currentColorThemeName: string;
   // Any disposables that should be cleaned up when clangd crashes.
   private subscriptions: vscode.Disposable[] = [];
   fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
@@ -70,9 +77,9 @@
   }
 
   async loadCurrentTheme() {
-const themeRuleMatcher = new ThemeRuleMatcher(
-await loadTheme(vscode.workspace.getConfiguration('workbench')
-.get('colorTheme')));
+this.currentColorThemeName = getCurrentThemeName();
+const themeRuleMatcher =
+new ThemeRuleMatcher(await loadTheme(this.currentColorThemeName));
 this.highlighter.initialize(themeRuleMatcher);
   }
 
@@ -91,6 +98,16 @@
 // highlighter being created.
 this.highlighter = new Highlighter(this.scopeLookupTable);
 this.subscriptions.push(vscode.Disposable.from(this.highlighter));
+// Adds a listener to reload the theme when it changes.
+this.subscriptions.push(
+vscode.workspace.onDidChangeConfiguration((conf) => {
+  if (!conf.affectsConfiguration('workbench'))
+// Configuration could not have affected the current colorTheme.
+return;
+  const newColorTheme = getCurrentThemeName();
+  if (newColorTheme !== this.currentColorThemeName)
+this.loadCurrentTheme();
+}));
 this.loadCurrentTheme();
 // Event handling for handling with TextDocuments/Editors lifetimes.
 this.subscriptions.push(vscode.window.onDidChangeVisibleTextEditors(


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -5,6 +5,11 @@
 import * as vscodelc from 'vscode-languageclient';
 import * as vscodelct from 'vscode-languageserver-types';
 
+function getCurrentThemeName() {
+  return vscode.workspace.getConfiguration('workbench')
+  .get('colorTheme');
+}
+
 // Parameters for the semantic highlighting (server-side) push notification.
 // Mirrors the structure in the semantic highlighting proposal for LSP.
 interface SemanticHighlightingParams {
@@ -56,6 +61,8 @@
   scopeLookupTable: string[][];
   // The object that applies the highlightings clangd sends.
   highlighter: Highlighter;
+  // The current color theme used for colorization.
+  private currentColorThemeName: string;
   // Any disposables that should be cleaned up when clangd crashes.
   private subscriptions: vscode.Disposable[] = [];
   fillClientCapabilities(capabilities: vscodelc.ClientCapabilities) {
@@ -70,9 +77,9 @@
   }
 
   async loadCurrentTheme() {
-const themeRuleMatcher = new ThemeRuleMatcher(
-await loadTheme(vscode.workspace.getConfiguration('workbench')
-.get('colorTheme')));
+this.currentColorThemeName = getCurrentThemeName();
+const themeRuleMatcher =
+new ThemeRuleMatcher(await loadTheme(this.currentColorThemeName));
 this.highlighter.initialize(themeRuleMatcher);
   }
 
@@ -91,6 +98,16 @@
 // highlighter being created.
 this.highlighter = new Highlighter(this.scopeLookupTable);
 this.subscriptions.push(vscode.Disposable.from(this.highlighter));
+// Adds a listener to reload the theme when it changes.
+this.subscriptions.push(
+vscode.workspace.onDidChangeConfiguration((conf) => {
+  if (!conf.affectsConfiguration('workbench'))
+// Configuration could not have affected the current colorTheme.
+return;
+  const newColorTheme = getCurrentThemeName(

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D63640#1648700 , @Tyker wrote:

> Sorry for the long wait.
>
> Changes:
>
> - Rebased on current master
> - Duplicated test file so that it runs for both importing


Thanks for addressing the issues. The ASTImporter related changes look good to 
me now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63640/new/

https://reviews.llvm.org/D63640



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/docs/OpenMPSupport.rst:204
++--+--+--++
+| device extension | clause: device_type   
   | :part:`worked on`| 
   |
++--+--+--++

This feature is implemented already


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64375/new/

https://reviews.llvm.org/D64375



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66881: [clangd][vscode] Don't leak the resources

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added a comment.

Oh, was not aware we needed to dispose the extension as well.

I think you should probably add the SemanticHighlightingFeature to the 
context.subscriptions as well, right? (because I didn't when I did the cleanup 
patch for it, sorry)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66881/new/

https://reviews.llvm.org/D66881



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr marked 2 inline comments as done.
gribozavr added inline comments.



Comment at: clang/lib/Index/IndexingAction.cpp:77
+IndexCtx->getDataConsumer().setPreprocessor(PP);
+PP->addPPCallbacks(std::make_unique(IndexCtx));
+  }

ilya-biryukov wrote:
> The fact that we call `addPPCallbacks` **after** creating `ASTContext` is a 
> bit concerning.
> 
> This wouldn't probably cause any problems in practice, but generally changing 
> preprocessor by the time `ASTContext` is already created seems dangerous 
> (what if something there ever starts depending on the state of the 
> preprocessor?)
> 
> I think this concern is easily elevated if we change Preprocessor and call 
> addPPCallbacks on `CreateASTConsumer(CompilerInvocation& CI)`. That would 
> mean we have a separate function that sets up the preprocessor and creates 
> `IndexASTConsumer` and it should be exposed to the clients.
> 
> Have you considered this approach? Would it work?
It does feel a bit weird, but we shouldn't have started parsing before calling 
`Initialize` on the `ASTConsumer`. Therefore I agree with you that it won't 
cause problems in practice.

Calling `addPPCallbacks` in `FrontendAction` is against the goal of this patch 
set. The goal is to encapsulate as much as possible in the `IndexASTConsumer`, 
because they compose well, unlike `FrontendAction`s. Therefore, requiring 
customization in `FrontendAction` is not possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66877/new/

https://reviews.llvm.org/D66877



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66878: [Index] Stopped wrapping FrontendActions in libIndex and its users

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr marked 4 inline comments as done.
gribozavr added inline comments.



Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:217
   std::unique_ptr Includes;
+  index::IndexingOptions Opts;
   std::unique_ptr PragmaHandler;

ilya-biryukov wrote:
> Are these option ever used? Do we need to keep them alive for the lifetime of 
> the action?
> Might be worth a comment.
They are passed in through the constructor, and consumed by 
`index::createIndexingASTConsumer` in `CreateASTConsumer`. So they need to be 
stored in a member variable between those two calls.



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:227
+  std::shared_ptr DataConsumer;
+  index::IndexingOptions Opts;
   CommentHandler *PragmaHandler;

ilya-biryukov wrote:
> Same here, we do not seem to use `Opts`, but still store them. To keep them 
> alive?
Same as in the other comment -- `Opts` are passed in through the constructor of 
`IndexAction`, and consumed in `IndexAction::CreateASTConsumer`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66878/new/

https://reviews.llvm.org/D66878



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66883: PR42045: Fix diagnosing enqueue_kernel call with too few args

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh created this revision.
svenvh added a reviewer: Anastasia.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The `err_typecheck_call_too_few_args diagnostic` takes arguments, but
none were provided causing clang to crash when attempting to diagnose
an enqueue_kernel call with too few arguments.


Repository:
  rC Clang

https://reviews.llvm.org/D66883

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/SemaOpenCL/cl20-device-side-enqueue.cl


Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -158,6 +158,8 @@
   enqueue_kernel(default_queue, flags, ndrange, 1, &event_wait_list, &evt); // 
expected-error{{illegal call to enqueue_kernel, incorrect argument types}}
 
   enqueue_kernel(default_queue, flags, ndrange, 1, 1); // 
expected-error{{illegal call to enqueue_kernel, incorrect argument types}}
+
+  enqueue_kernel(default_queue, ndrange, ^{}); // expected-error{{too few 
arguments to function call, expected at least 4, have 3}}
 }
 
 // Diagnostic tests for get_kernel_work_group_size and allowed block parameter 
types in dynamic parallelism.
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -629,7 +629,9 @@
   unsigned NumArgs = TheCall->getNumArgs();
 
   if (NumArgs < 4) {
-S.Diag(TheCall->getBeginLoc(), diag::err_typecheck_call_too_few_args);
+S.Diag(TheCall->getBeginLoc(),
+   diag::err_typecheck_call_too_few_args_at_least)
+<< 0 << 4 << NumArgs;
 return true;
   }
 


Index: clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
===
--- clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -158,6 +158,8 @@
   enqueue_kernel(default_queue, flags, ndrange, 1, &event_wait_list, &evt); // expected-error{{illegal call to enqueue_kernel, incorrect argument types}}
 
   enqueue_kernel(default_queue, flags, ndrange, 1, 1); // expected-error{{illegal call to enqueue_kernel, incorrect argument types}}
+
+  enqueue_kernel(default_queue, ndrange, ^{}); // expected-error{{too few arguments to function call, expected at least 4, have 3}}
 }
 
 // Diagnostic tests for get_kernel_work_group_size and allowed block parameter types in dynamic parallelism.
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -629,7 +629,9 @@
   unsigned NumArgs = TheCall->getNumArgs();
 
   if (NumArgs < 4) {
-S.Diag(TheCall->getBeginLoc(), diag::err_typecheck_call_too_few_args);
+S.Diag(TheCall->getBeginLoc(),
+   diag::err_typecheck_call_too_few_args_at_least)
+<< 0 << 4 << NumArgs;
 return true;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66872: [clangd] SelectionTree should mark a node as fully-selected if the only claimed tokens were early-claimed.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 217645.
sammccall added a comment.

remove printfs


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66872/new/

https://reviews.llvm.org/D66872

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -562,6 +562,9 @@
 struct Visible {};
   }
 }
+template  class Allocator;
+template > class Vec{};
+using VecInt = Vec;
   )cpp";
 
   EXPECT_AVAILABLE("^a^u^t^o^ i = 0;");
@@ -597,6 +600,11 @@
   // replace array types
   EXPECT_EQ(apply(R"cpp(au^to x = "test")cpp"),
 R"cpp(const char * x = "test")cpp");
+  // Default template parameters aren't printed.
+  EXPECT_EQ(apply(R"cpp(au^to x = Vec();)cpp"),
+R"cpp(Vec x = Vec();)cpp");
+  EXPECT_EQ(apply(R"cpp(au^to x = VecInt();)cpp"),
+R"cpp(VecInt x = VecInt();)cpp");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -329,6 +329,8 @@
 #define ECHO(X) X
 ECHO(EC^HO([[$C[[int]]) EC^HO(a]]));
   ]])cpp",
+  R"cpp( $C[[^$C[[int]] a^]]; )cpp",
+  R"cpp( $C[[^$C[[int]] a = $C[[5]]^]]; )cpp",
   };
   for (const char *C : Cases) {
 Annotations Test(C);
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -60,13 +60,13 @@
 
   // Associates any tokens overlapping [Begin, End) with an AST node.
   // Tokens that were already claimed by another AST node are not claimed again.
-  // Returns whether the node is selected in the sense of SelectionTree.
-  SelectionTree::Selection claim(unsigned Begin, unsigned End) {
+  // Updates Result if the node is selected in the sense of SelectionTree.
+  void claim(unsigned Begin, unsigned End, SelectionTree::Selection &Result) {
 assert(Begin <= End);
 
 // Fast-path for missing the selection entirely.
 if (Begin >= SelEnd || End <= SelBegin)
-  return SelectionTree::Unselected;
+  return;
 
 // We will consider the range (at least partially) selected if it hit any
 // selected and previously unclaimed token.
@@ -97,9 +97,13 @@
   }
 }
 
-if (!ClaimedAnyToken)
-  return SelectionTree::Unselected;
-return PartialSelection ? SelectionTree::Partial : SelectionTree::Complete;
+// If some tokens were previously claimed (Result != Unselected), we may
+// upgrade from Partial->Complete, even if no new tokens were claimed.
+// Important for [[int a]].
+if (ClaimedAnyToken || Result) {
+  Result = std::max(Result, PartialSelection ? SelectionTree::Partial
+ : SelectionTree::Complete);
+}
   }
 
 private:
@@ -161,7 +165,9 @@
 assert(V.Stack.size() == 1 && "Unpaired push/pop?");
 assert(V.Stack.top() == &V.Nodes.front());
 // We selected TUDecl if tokens were unclaimed (or the file is empty).
-if (V.Nodes.size() == 1 || V.Claimed.claim(Begin, End)) {
+SelectionTree::Selection UnclaimedTokens = SelectionTree::Unselected;
+V.Claimed.claim(Begin, End, UnclaimedTokens);
+if (UnclaimedTokens || V.Nodes.size() == 1) {
   StringRef FileContent = AST.getSourceManager().getBufferData(File);
   // Don't require the trailing newlines to be selected.
   bool SelectedAll = Begin == 0 && End >= FileContent.rtrim().size();
@@ -317,10 +323,11 @@
 Nodes.emplace_back();
 Nodes.back().ASTNode = std::move(Node);
 Nodes.back().Parent = Stack.top();
-// Early hit detection never selects the whole node.
 Stack.push(&Nodes.back());
-Nodes.back().Selected =
-claimRange(Early) ? SelectionTree::Partial : SelectionTree::Unselected;
+claimRange(Early, Nodes.back().Selected);
+// Early hit detection never selects the whole node.
+if (Nodes.back().Selected)
+  Nodes.back().Selected = SelectionTree::Partial;
   }
 
   // Pops a node off the ancestor stack, and finalizes it. Pairs with push().
@@ -328,8 +335,7 @@
   void pop() {
 Node &N = *Stack.top();
 dlog("{1}pop: {0}", printNodeToString(N.ASTNode, PrintPolicy), indent(-1));
-if (auto Sel = claimRange(N.ASTNode.getSourceRange()))
-  N.Selected = Sel;
+claimRange(N.ASTNode.getSourceRange(), N.Selected);
 if (N.Selected || !N.Children.empty()) {
   // Attach to the tree.
   N.Parent->Children.push_back(&N);
@@ -359,9 +3

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

I think we should go ahead and land this. The only points that I'd really like 
to see fixed is `shared_ptr`, mostly because it's a strong signal there's 
something complicated going on and I don't think (or hope) there is!




Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:326
+std::string spellType(QualType TypeInfo) {
+  return TypeInfo.getUnqualifiedType().getNonReferenceType().getAsString();
+};

sammccall wrote:
> use `printType` from AST.h?
> 
> (You'll want to drop qualifiers/refs before calling that, but it's not at all 
> obvious from the function name here that they're dropped, so that should be 
> at the callsite anyway)
Second half is not done: the suggestion is that it's really confusing where 
spellType is called that it drops qualifiers, so just I'd inline this into the 
callsite.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:332
+  if (WithTypeAndQualifiers) {
+if (IsConst)
+  Result += "const ";

SureYeaah wrote:
> kadircet wrote:
> > why don't we infer this const from QualType ?
> In a future patch we will want to use heuristics like has the variable been 
> assigned, was it passed as a non-const reference, was its address taken, etc. 
I think the point is that the QualType in parameter could/should represent the 
*parameter* type, not the type of the variable being captured.
e.g. it could be  `const std::string&` even if the original variable was just 
`std::string`.

This seems the more natural model (the struct is called Parameter, not 
CapturedVariable or Argument) but there may be reasons not to do this.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+  std::shared_ptr ExtZone;
+};

SureYeaah wrote:
> kadircet wrote:
> > why do you need a shared_ptr here?
> getExtractionZone creates an Optional which needs to persist 
> from prepare to apply. Is there a better way to store a reference to the 
> ExtractionZone instance inside ExtractFunction?
shared ownership is less common and harder to reason about than exclusive 
ownership

Generally we prefer deciding where ownership should reside (T or Optional or 
unique_ptr), and where you should have an unowned reference (T& or T*).

In this case, findExtractionZone() should ideally return 
`Optional`, after prepare() the ExtractFunction class should 
own it (as a Optional), and apply should pass it around as a 
const ExtractionZone&.

If it turns out ExtractionZone isn't a movable type, then 
`Optional` won't work and you'll need 
`unique_ptr` instead. But shared_ptr shouldn't be needed 
regardless.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:119
+  case SelectionTree::Selection::Partial:
+// Treat Partially selected VarDecl as completely selected since
+// SelectionTree doesn't always select VarDecls correctly.

D66872 has the fix if you'd rather avoid these workarounds



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146
+  SourceRange EnclosingFuncRange;
+  std::set RootStmts;
+  SourceLocation getInsertionPoint() const {

llvm::DenseSet

std::set is a pretty terrible data structure unless you really really need 
order.
(Lots of allocations, lots of pointer-chasing)



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:238
+  // Convert RootStmt Nodes to Stmts and insert in RootStmts.
+  llvm::transform(ExtZone->Parent->Children,
+  std::inserter(ExtZone->RootStmts, 
ExtZone->RootStmts.begin()),

this is also:
```
for (const SelectionTree::Node *N)
  ExtZone->RootStmts.insert(N->ASTNode.get());
```
which is shorter and (I think) less obfuscated


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:165
+else
+  SR.setEnd(ChildFileRange->getEnd());
+  }

SureYeaah wrote:
> kadircet wrote:
> > I suppose this relies on the fact that "AST contains the nodes ordered by 
> > their begin location"? Could you add an assertion for that?
> I've removed the loop, should I still add this?
no need



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:225
+// enclosingFunction.
+std::shared_ptr getExtractionZone(const Node *CommonAnc,
+  const SourceManager &SM,

SureYeaah wrote:
> SureYeaah wrote:
> > sammccall wrote:
> > > kadircet wrote:
> > > > why is this function returning a shared_ptr ?
> > > avoid shared_ptr unless there's a strong reason. 
> > > `Optional` seems fine here?
> > And store a unique_ptr to the optional in ExtractFunction?
> Because ExtractFunction needs to store a pointer/reference to ExtractionZone 
> somehow in prepare and access it in apply.
Let's figure it out in the `ExtractFunction` this can still return an 
`Optional` which you can convert into a pointer/reference later 
on if need be?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:547
+private:
+  std::shared_ptr ExtZone;
+};

SureYeaah wrote:
> kadircet wrote:
> > why do you need a shared_ptr here?
> getExtractionZone creates an Optional which needs to persist 
> from prepare to apply. Is there a better way to store a reference to the 
> ExtractionZone instance inside ExtractFunction?
Then why not just store `Optional` ?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:146
+  SourceRange EnclosingFuncRange;
+  std::set RootStmts;
+  SourceLocation getInsertionPoint() const {

lifetime of this field looks complicated can you add some comments on how/when 
it is initialized and maybe enforce immutability ?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:150
+  }
+  bool isRootStmt(const Stmt *S) const;
+  // The last root statement is important to decide where we need to insert a

it seems like you are rather using it to decide whether a statement is inside 
the zone or not?

Could you rather rename it to reflect that and add some comments?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:169
+  // FIXME: Support extraction from methods.
+  if (CurNode->ASTNode.get())
+return nullptr;

nit:
```
if (isa(Func))
```



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:172
+  // FIXME: Support extraction from templated functions.
+  if (CurNode->Parent->ASTNode.get())
+return nullptr;

nit:
```
if(isa(Func))
```



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346
+ZoneRelative DeclaredIn;
+// index of the declaration or first reference
+unsigned DeclIndex;

i think you mean `index of the first reference to this decl` ?



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:486
+// (this includes the case of recursive call to EnclosingFunc in Zone).
+if (!VD || dyn_cast_or_null(DeclInfo.TheDecl))
+  return false;

nit `isa` instead of `dyn_cast`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66884: Removed dead code from clang/AST/NSAPI.h

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66884

Files:
  clang/include/clang/AST/NSAPI.h
  clang/lib/AST/NSAPI.cpp


Index: clang/lib/AST/NSAPI.cpp
===
--- clang/lib/AST/NSAPI.cpp
+++ clang/lib/AST/NSAPI.cpp
@@ -75,17 +75,6 @@
   return NSStringSelectors[MK];
 }
 
-Optional
-NSAPI::getNSStringMethodKind(Selector Sel) const {
-  for (unsigned i = 0; i != NumNSStringMethods; ++i) {
-NSStringMethodKind MK = NSStringMethodKind(i);
-if (Sel == getNSStringSelector(MK))
-  return MK;
-  }
-
-  return None;
-}
-
 Selector NSAPI::getNSArraySelector(NSArrayMethodKind MK) const {
   if (NSArraySelectors[MK].isNull()) {
 Selector Sel;
Index: clang/include/clang/AST/NSAPI.h
===
--- clang/include/clang/AST/NSAPI.h
+++ clang/include/clang/AST/NSAPI.h
@@ -55,9 +55,6 @@
   /// The Objective-C NSString selectors.
   Selector getNSStringSelector(NSStringMethodKind MK) const;
 
-  /// Return NSStringMethodKind if \param Sel is such a selector.
-  Optional getNSStringMethodKind(Selector Sel) const;
-
   /// Returns true if the expression \param E is a reference of
   /// "NSUTF8StringEncoding" enum constant.
   bool isNSUTF8StringEncodingConstant(const Expr *E) const {


Index: clang/lib/AST/NSAPI.cpp
===
--- clang/lib/AST/NSAPI.cpp
+++ clang/lib/AST/NSAPI.cpp
@@ -75,17 +75,6 @@
   return NSStringSelectors[MK];
 }
 
-Optional
-NSAPI::getNSStringMethodKind(Selector Sel) const {
-  for (unsigned i = 0; i != NumNSStringMethods; ++i) {
-NSStringMethodKind MK = NSStringMethodKind(i);
-if (Sel == getNSStringSelector(MK))
-  return MK;
-  }
-
-  return None;
-}
-
 Selector NSAPI::getNSArraySelector(NSArrayMethodKind MK) const {
   if (NSArraySelectors[MK].isNull()) {
 Selector Sel;
Index: clang/include/clang/AST/NSAPI.h
===
--- clang/include/clang/AST/NSAPI.h
+++ clang/include/clang/AST/NSAPI.h
@@ -55,9 +55,6 @@
   /// The Objective-C NSString selectors.
   Selector getNSStringSelector(NSStringMethodKind MK) const;
 
-  /// Return NSStringMethodKind if \param Sel is such a selector.
-  Optional getNSStringMethodKind(Selector Sel) const;
-
   /// Returns true if the expression \param E is a reference of
   /// "NSUTF8StringEncoding" enum constant.
   bool isNSUTF8StringEncodingConstant(const Expr *E) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66883: PR42045: Fix diagnosing enqueue_kernel call with too few args

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66883/new/

https://reviews.llvm.org/D66883



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66738: [clangd] Added highlighting for structured bindings.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 217652.
jvikstrom marked 6 inline comments as done.
jvikstrom added a comment.

Abandoned trying to highlight the same as the bound decl.

Reasoning: If you have a reference to a parameter we are not trying to 
highlight the reference as a parameter, the reference is highlighted as a 
variable. Don't really think bindings should be different.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66738/new/

https://reviews.llvm.org/D66738

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -431,6 +431,21 @@
 assert($Variable[[x]] != $Variable[[y]]);
 assert($Variable[[x]] != $Function[[f]]());
   }
+)cpp",
+R"cpp(
+  struct $Class[[S]] {
+$Primitive[[float]] $Field[[Value]];
+$Class[[S]] *$Field[[Next]];
+  };
+  $Class[[S]] $Variable[[Global]][2] = {$Class[[S]](), $Class[[S]]()};
+  $Primitive[[void]] $Function[[f]]($Class[[S]] $Parameter[[P]]) {
+$Primitive[[int]] $Variable[[A]][2] = {1,2};
+auto [$Variable[[B1]], $Variable[[B2]]] = $Variable[[A]];
+auto [$Variable[[G1]], $Variable[[G2]]] = $Variable[[Global]];
+$Class[[auto]] [$Variable[[P1]], $Variable[[P2]]] = $Parameter[[P]];
+// Highlights references to BindingDecls.
+$Variable[[B1]]++;
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -229,6 +229,10 @@
   addToken(Loc, HighlightingKind::Variable);
   return;
 }
+if (isa(D)) {
+  addToken(Loc, HighlightingKind::Variable);
+  return;
+}
 if (isa(D)) {
   addToken(Loc, HighlightingKind::Function);
   return;


Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -431,6 +431,21 @@
 assert($Variable[[x]] != $Variable[[y]]);
 assert($Variable[[x]] != $Function[[f]]());
   }
+)cpp",
+R"cpp(
+  struct $Class[[S]] {
+$Primitive[[float]] $Field[[Value]];
+$Class[[S]] *$Field[[Next]];
+  };
+  $Class[[S]] $Variable[[Global]][2] = {$Class[[S]](), $Class[[S]]()};
+  $Primitive[[void]] $Function[[f]]($Class[[S]] $Parameter[[P]]) {
+$Primitive[[int]] $Variable[[A]][2] = {1,2};
+auto [$Variable[[B1]], $Variable[[B2]]] = $Variable[[A]];
+auto [$Variable[[G1]], $Variable[[G2]]] = $Variable[[Global]];
+$Class[[auto]] [$Variable[[P1]], $Variable[[P2]]] = $Parameter[[P]];
+// Highlights references to BindingDecls.
+$Variable[[B1]]++;
+  }
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -229,6 +229,10 @@
   addToken(Loc, HighlightingKind::Variable);
   return;
 }
+if (isa(D)) {
+  addToken(Loc, HighlightingKind::Variable);
+  return;
+}
 if (isa(D)) {
   addToken(Loc, HighlightingKind::Function);
   return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r370035 - [ReleaseNotes] MemorySanitizer support of ASLR on FreeBSD

[Hans Wennborg via cfe-commits]

Merged to release_90 in r370213.

On Tue, Aug 27, 2019 at 12:02 PM David Carlier via cfe-commits
 wrote:
>
> Author: devnexen
> Date: Tue Aug 27 03:04:03 2019
> New Revision: 370035
>
> URL: http://llvm.org/viewvc/llvm-project?rev=370035&view=rev
> Log:
> [ReleaseNotes] MemorySanitizer support of ASLR on FreeBSD
>
> Reviewers: sylvestre.ledru, kcc
>
> Reviewed By: sylvestre.ledru
>
> Differential Revision: https://reviews.llvm.org/D66792
>
> Modified:
> cfe/trunk/docs/MemorySanitizer.rst
>
> Modified: cfe/trunk/docs/MemorySanitizer.rst
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/MemorySanitizer.rst?rev=370035&r1=370034&r2=370035&view=diff
> ==
> --- cfe/trunk/docs/MemorySanitizer.rst (original)
> +++ cfe/trunk/docs/MemorySanitizer.rst Tue Aug 27 03:04:03 2019
> @@ -204,6 +204,9 @@ Limitations
>non-position-independent executables, and could fail on some Linux
>kernel versions with disabled ASLR. Refer to documentation for older 
> versions
>for more details.
> +* MemorySanitizer might be incompatible with position-independent executables
> +  from FreeBSD 13 but there is a check done at runtime and throws a warning
> +  in this case.
>
>  Current Status
>  ==
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66406: [clangd] Update themeRuleMatcher when color theme changes in vscode extension.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

please update the description of the patch.




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:104
+vscode.workspace.onDidChangeConfiguration((conf) => {
+  if (!conf.affectsConfiguration('workbench'))
+// Configuration could not have affected the current colorTheme.

does `conf.affectsConfiguration('workbench.colorTheme')` work?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66406/new/

https://reviews.llvm.org/D66406



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r370214 - [OPENMP][Analysis] Add analysis of the map clauses.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Wed Aug 28 07:55:08 2019
New Revision: 370214

URL: http://llvm.org/viewvc/llvm-project?rev=370214&view=rev
Log:
[OPENMP][Analysis] Add analysis of the map clauses.

Summary:
Added basic analysis of map clauses. Only map clauses with to and tofrom
map type must be analyzed since all other map types (alloc, delete, etc.) do 
not require to use the value of the initial variable, instead they create the 
new copy of the variable.

Reviewers: NoQ

Subscribers: guansong, cfe-commits, kkwli0, caomhin

Tags: #clang

Differential Revision: https://reviews.llvm.org/D8

Modified:
cfe/trunk/include/clang/AST/OpenMPClause.h
cfe/trunk/test/Analysis/cfg-openmp.cpp
cfe/trunk/test/OpenMP/target_data_messages.c
cfe/trunk/test/OpenMP/target_enter_data_map_messages.c
cfe/trunk/test/OpenMP/target_map_messages.cpp
cfe/trunk/test/OpenMP/target_parallel_for_map_messages.cpp
cfe/trunk/test/OpenMP/target_parallel_for_simd_map_messages.cpp
cfe/trunk/test/OpenMP/target_parallel_map_messages.cpp
cfe/trunk/test/OpenMP/target_simd_map_messages.cpp
cfe/trunk/test/OpenMP/target_teams_distribute_map_messages.cpp
cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp

cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
cfe/trunk/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
cfe/trunk/test/OpenMP/target_teams_map_messages.cpp

Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=370214&r1=370213&r2=370214&view=diff
==
--- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
+++ cfe/trunk/include/clang/AST/OpenMPClause.h Wed Aug 28 07:55:08 2019
@@ -5025,12 +5025,17 @@ public:
   }
 
   child_range used_children() {
+if (MapType == OMPC_MAP_to || MapType == OMPC_MAP_tofrom)
+  return child_range(reinterpret_cast(varlist_begin()),
+ reinterpret_cast(varlist_end()));
 return child_range(child_iterator(), child_iterator());
   }
   const_child_range used_children() const {
-return const_child_range(const_child_iterator(), const_child_iterator());
+auto Children = const_cast(this)->used_children();
+return const_child_range(Children.begin(), Children.end());
   }
 
+
   static bool classof(const OMPClause *T) {
 return T->getClauseKind() == OMPC_map;
   }

Modified: cfe/trunk/test/Analysis/cfg-openmp.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/cfg-openmp.cpp?rev=370214&r1=370213&r2=370214&view=diff
==
--- cfe/trunk/test/Analysis/cfg-openmp.cpp (original)
+++ cfe/trunk/test/Analysis/cfg-openmp.cpp Wed Aug 28 07:55:08 2019
@@ -9,7 +9,8 @@ void xxx(int argc) {
 // CHECK-NEXT:   4: int rd;
 // CHECK-NEXT:   5: int lin;
 // CHECK-NEXT:   6: int step;
-  int x, cond, fp, rd, lin, step;
+// CHECK-NEXT:   7: int map;
+  int x, cond, fp, rd, lin, step, map;
 // CHECK-NEXT:   [[#ATOM:]]: x
 // CHECK-NEXT:   [[#ATOM+1]]: [B1.[[#ATOM]]] (ImplicitCastExpr, 
LValueToRValue, int)
 // CHECK-NEXT:   [[#ATOM+2]]: argc
@@ -219,10 +220,10 @@ void xxx(int argc) {
   : argc) if(cond) firstprivate(fp) reduction(-:rd)
   argc = x;
 // CHECK-NEXT:  [[#TPF:]]:
-// CHECK-SAME:  [B1.[[#TPF+13]]]
-// CHECK-NEXT:  [[#TPF+1]]: [B1.[[#TPF+13]]] (ImplicitCastExpr, 
LValueToRValue, int)
-// CHECK-NEXT:  [[#TPF+2]]: [B1.[[#TPF+12]]]
-// CHECK-NEXT:  [[#TPF+3]]: [B1.[[#TPF+12]]] = [B1.[[#TPF+1]]]
+// CHECK-SAME:  [B1.[[#TPF+14]]]
+// CHECK-NEXT:  [[#TPF+1]]: [B1.[[#TPF+14]]] (ImplicitCastExpr, 
LValueToRValue, int)
+// CHECK-NEXT:  [[#TPF+2]]: [B1.[[#TPF+13]]]
+// CHECK-NEXT:  [[#TPF+3]]: [B1.[[#TPF+13]]] = [B1.[[#TPF+1]]]
 // CHECK-NEXT:  [[#TPF+4]]: cond
 // CHECK-NEXT:  [[#TPF+5]]: [B1.[[#TPF+4]]] (ImplicitCastExpr, LValueToRValue, 
int)
 // CHECK-NEXT:  [[#TPF+6]]: [B1.[[#TPF+5]]] (ImplicitCastExpr, 
IntegralToBoolean, _Bool)
@@ -231,19 +232,20 @@ void xxx(int argc) {
 // CHECK-NEXT:  [[#TPF+9]]: lin
 // CHECK-NEXT:  [[#TPF+10]]: step
 // CHECK-NEXT:  [[#TPF+11]]: [B1.[[#TPF+10]]] (ImplicitCastExpr, 
LValueToRValue, int)
-// CHECK-NEXT:  [[#TPF+12]]: argc
-// CHECK-NEXT:  [[#TPF+13]]: x
-// CHECK-NEXT:  [[#TPF+14]]: #pragma omp target parallel for if(parallel: 
cond) firstprivate(fp) reduction(max: rd) linear(lin: step)
+// CHECK-NEXT:  [[#TPF+12]]: map
+// CHECK-NEXT:  [[#TPF+13]]: argc
+// CHECK-NEXT:  [[#TPF+14]]: x
+// CHECK-NEXT:  [[#TPF+15]]: #pragma omp target parallel for if(parallel: 
cond) firstprivate(fp) reduction(max: rd) linear(lin: step) map(tofrom: map)
 // CHECK-NEXT:for (int i = 0; i < 10; ++i)
 // CHECK-NEXT:[B1.[[#TPF+3]]];
-#pragma omp target parallel for if(parallel:cond) firstprivate(fp) 
reduction(max:rd) linear(lin: step)
+#pragma omp target parallel for if(para

[PATCH] D66668: [OPENMP][Analysis] Add analysis of the map clauses.

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370214: [OPENMP][Analysis] Add analysis of the map clauses. 
(authored by ABataev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D8/new/

https://reviews.llvm.org/D8

Files:
  cfe/trunk/include/clang/AST/OpenMPClause.h
  cfe/trunk/test/Analysis/cfg-openmp.cpp
  cfe/trunk/test/OpenMP/target_data_messages.c
  cfe/trunk/test/OpenMP/target_enter_data_map_messages.c
  cfe/trunk/test/OpenMP/target_map_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_for_map_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_for_simd_map_messages.cpp
  cfe/trunk/test/OpenMP/target_parallel_map_messages.cpp
  cfe/trunk/test/OpenMP/target_simd_map_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_map_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
  
cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
  cfe/trunk/test/OpenMP/target_teams_map_messages.cpp

Index: cfe/trunk/include/clang/AST/OpenMPClause.h
===
--- cfe/trunk/include/clang/AST/OpenMPClause.h
+++ cfe/trunk/include/clang/AST/OpenMPClause.h
@@ -5025,12 +5025,17 @@
   }
 
   child_range used_children() {
+if (MapType == OMPC_MAP_to || MapType == OMPC_MAP_tofrom)
+  return child_range(reinterpret_cast(varlist_begin()),
+ reinterpret_cast(varlist_end()));
 return child_range(child_iterator(), child_iterator());
   }
   const_child_range used_children() const {
-return const_child_range(const_child_iterator(), const_child_iterator());
+auto Children = const_cast(this)->used_children();
+return const_child_range(Children.begin(), Children.end());
   }
 
+
   static bool classof(const OMPClause *T) {
 return T->getClauseKind() == OMPC_map;
   }
Index: cfe/trunk/test/OpenMP/target_simd_map_messages.cpp
===
--- cfe/trunk/test/OpenMP/target_simd_map_messages.cpp
+++ cfe/trunk/test/OpenMP/target_simd_map_messages.cpp
@@ -9,6 +9,13 @@
   return argc;
 }
 
+void xxx(int argc) {
+  int map; // expected-note {{initialize the variable 'map' to silence this warning}}
+#pragma omp target simd map(to: map) // expected-warning {{variable 'map' is uninitialized when used here}}
+  for (int i = 0; i < 10; ++i)
+;
+}
+
 struct S1; // expected-note 2 {{declared here}}
 extern S1 a;
 class S2 {
Index: cfe/trunk/test/OpenMP/target_teams_distribute_map_messages.cpp
===
--- cfe/trunk/test/OpenMP/target_teams_distribute_map_messages.cpp
+++ cfe/trunk/test/OpenMP/target_teams_distribute_map_messages.cpp
@@ -9,6 +9,13 @@
   return argc;
 }
 
+void xxx(int argc) {
+  int map; // expected-note {{initialize the variable 'map' to silence this warning}}
+#pragma omp target teams distribute map(map) // expected-warning {{variable 'map' is uninitialized when used here}}
+  for (int i = 0; i < 10; ++i)
+;
+}
+
 struct S1; // expected-note 2 {{declared here}}
 extern S1 a;
 class S2 {
Index: cfe/trunk/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
===
--- cfe/trunk/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
+++ cfe/trunk/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
@@ -9,6 +9,13 @@
   return argc;
 }
 
+void xxx(int argc) {
+  int map; // expected-note {{initialize the variable 'map' to silence this warning}}
+#pragma omp target teams distribute simd map(map) // expected-warning {{variable 'map' is uninitialized when used here}}
+  for (int i = 0; i < 10; ++i)
+;
+}
+
 struct S1; // expected-note 2 {{declared here}}
 extern S1 a;
 class S2 {
Index: cfe/trunk/test/OpenMP/target_parallel_map_messages.cpp
===
--- cfe/trunk/test/OpenMP/target_parallel_map_messages.cpp
+++ cfe/trunk/test/OpenMP/target_parallel_map_messages.cpp
@@ -9,6 +9,13 @@
   return argc;
 }
 
+void xxx(int argc) {
+  int map; // expected-note {{initialize the variable 'map' to silence this warning}}
+#pragma omp target parallel map(tofrom: map) // expected-warning {{variable 'map' is uninitialized when used here}}
+  for (int i = 0; i < 10; ++i)
+;
+}
+
 struct S1; // expected-note 2 {{declared here}}
 extern S1 a;
 class S2 {
Index: cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
===
--- cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
+++ cfe/trunk/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
@@ -9,6 +9,13 @@
   re

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D66733#1647964 , @steakhal wrote:

> @Szelethus The mispositioned report message was my fault. I used a different 
> version of clang for the analysis and to upload the results, which resulted 
> in some mispositioned reports.
>  I've fixed the linked CodeChecker instance.


If you want to see how the analyzer behaves, I advise to keep an LLVM 
repository purely for analysis that you're not ever refreshing, and a separate 
one for building your clang :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66881: [clangd][vscode] Don't leak the resources

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D66881#1649044 , @jvikstrom wrote:

> Oh, was not aware we needed to dispose the extension as well.
>
> I think you should probably add the SemanticHighlightingFeature to the 
> context.subscriptions as well, right? (because I didn't when I did the 
> cleanup patch for it, sorry)


good catch, done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66881/new/

https://reviews.llvm.org/D66881



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66881: [clangd][vscode] Don't leak the resources

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 217657.
hokein added a comment.

dispose the semanticHighlighting feature as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66881/new/

https://reviews.llvm.org/D66881

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r370218 - [clangd][vscode] Don't leak the resources

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Aug 28 08:09:04 2019
New Revision: 370218

URL: http://llvm.org/viewvc/llvm-project?rev=370218&view=rev
Log:
[clangd][vscode] Don't leak the resources

Summary: We miss a few places where we need to add them to the subscriptions.

Reviewers: jvikstrom

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66881

Modified:
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts

Modified: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts?rev=370218&r1=370217&r2=370218&view=diff
==
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts 
(original)
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts Wed 
Aug 28 08:09:04 2019
@@ -111,6 +111,8 @@ export function activate(context: vscode
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@ export function activate(context: vscode
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@ export function activate(context: vscode
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:601
+  llvm::inconvertibleErrorCode(),
+  "File contents differ on disk for %s, please save", FilePath.data());
+}

you seem to be checking this both here and in clangdlspserver. Why?



Comment at: clang-tools-extra/clangd/SourceCode.h:42
 
+/// Represents a set of edits generated for a single file.
+struct Edit {

nit: drop "represents"



Comment at: clang-tools-extra/clangd/SourceCode.h:42
 
+/// Represents a set of edits generated for a single file.
+struct Edit {

sammccall wrote:
> nit: drop "represents"
nit: this could also describe Replacements, vector, etc. Motivate 
this class a little more?



Comment at: clang-tools-extra/clangd/SourceCode.h:215
 
+/// Formats the edits in \p ApplyEdits and generates TextEdits from those.
+/// Ensures the files in FS are consistent with the files used while generating

not sure what "generates TextEdits from those" refers to.

Could this function be called "reformatEdits" or "formatAroundEdits"?



Comment at: clang-tools-extra/clangd/SourceCode.h:220
+llvm::Error formatAndGenerateEdits(llvm::vfs::FileSystem *FS,
+   llvm::StringMap &ApplyEdits,
+   llvm::StringRef MainFilePath,

This signature is confusing: we pass code in *three* different ways (FS, Edits, 
and MainFileCode). Much of this is because we put the loop (and therefore all 
special cases) inside this function.
The logic around the way the VFS is mapped for the main file vs others doesn't 
really belong in this layer. Neither does "please save"...

It seems this wants to be something simpler/smaller like `reformatEdit(const 
Edit&, const Style&)` that could be called from ClangdServer. There's probably 
another helper like `checkApplies(const Edit&, VFS*)` that would go in 
ClangdLSPServer.




Comment at: clang-tools-extra/clangd/refactor/Tweak.h:74
+/// A mapping from absolute file path to edits.
+llvm::Optional> ApplyEdits;
 

what's the difference between `None` and an empty map?



Comment at: clang-tools-extra/clangd/refactor/Tweak.h:76
 
-static Effect applyEdit(tooling::Replacements R) {
+void addEdit(StringRef FilePath, Edit Ed) {
+  assert(ApplyEdits);

(if the null map case goes away, this can too)



Comment at: clang-tools-extra/clangd/refactor/Tweak.h:81
+
+static Effect applyEdit(StringRef FilePath, Edit Ed) {
   Effect E;

This greatly increases the burden of callers of this function, who mostly want 
to edit the current file.
 - need to provide a fully-qualified name (I don't think the implementations in 
this patch actually do that)
 - need to construct an Edit

can we provide an `Effect mainFileEdit(const SourceManager&, Replacements)`? 
and maybe `Effect fileEdit(const SourceManager&, FileID, Replacements)` though 
maybe the latter doesn't cover enough cases to pull its weight.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66637/new/

https://reviews.llvm.org/D66637



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66881: [clangd][vscode] Don't leak the resources

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370218: [clangd][vscode] Don't leak the resources 
(authored by hokein, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66881?vs=217657&id=217659#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66881/new/

https://reviews.llvm.org/D66881

Files:
  clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r370219 - Try fixing CRLF issues in Git with [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Alexandre Ganea via cfe-commits]

Author: aganea
Date: Wed Aug 28 08:14:37 2019
New Revision: 370219

URL: http://llvm.org/viewvc/llvm-project?rev=370219&view=rev
Log:
Try fixing CRLF issues in Git with [clang-scan-deps] Minimizer: Correctly 
handle multi-line content with CR+LF line endings

Differential Revision: https://reviews.llvm.org/D66556

Added:

cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c   
(with props)
Removed:
cfe/trunk/.gitattributes

Removed: cfe/trunk/.gitattributes
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/.gitattributes?rev=370218&view=auto
==
--- cfe/trunk/.gitattributes (original)
+++ cfe/trunk/.gitattributes (removed)
@@ -1,3 +0,0 @@
-# Windows line ending tests
-test/Lexer/minimize_source_to_dependency_directives_invalid_error.c text 
eol=crlf
-test/FixIt/fixit-newline-style.c text eol=crlf

Added: 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c?rev=370219&view=auto
==
--- 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c 
(added)
+++ 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c 
Wed Aug 28 08:14:37 2019
@@ -0,0 +1,16 @@
+// Test CF+LF are properly handled along with quoted, multi-line #error
+// RUN: %clang_cc1 -DOTHER -print-dependency-directives-minimized-source %s 
2>&1 | FileCheck %s
+
+#ifndef TEST
+#error "message \
+   more message \
+   even more"
+#endif
+
+#ifdef OTHER
+#include 
+#endif
+
+// CHECK:  #ifdef OTHER
+// CHECK-NEXT: #include 
+// CHECK-NEXT: #endif

Propchange: 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
--
svn:eol-style = CRLF


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65433: [clangd] DefineInline action availability checks

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:59
+
+void checkAvailable(StringRef ID, llvm::StringRef Input, bool Available,
+const llvm::StringMap &AdditionalFiles) {

This function, and all the other helpers in this file, are actually imminently 
going away in favor of TweakTesting.h.
Sorry it's been stuck in my backlog for a couple of weeks...

We should work out how to do that stuff there.
One approach is to give TweakTest a `StringMap ExtraFiles` and 
`StringMap EditedFiles` that excludes main, so you'd write
```
ExtraFiles["foo.h"] = "...";
EXPECT_EQ(apply("..."), "...");
EXPECT_THAT(EditedFiles, ElementsAre(Pair(testPath("foo.h"), "...");
```
or something like this.

(Whatever we do, we should bear in mind the fact that out-line will mean we 
need to supply an index too)



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:631
+
+  // Should it also trigger on NestedNameSpecifier? i.e "Bar::"
+  [[void [[Bar::[[b^a^z() [[{

I don't think so



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:653
+
+TEST_F(DefineInlineTest, NoDecl) {
+  checkNotAvailable(TweakID, R"cpp(

nit: NoForwardDecl



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:683
+})cpp",
+ {{"a.h", "void bar(); void foo(int test);"}});
+

FWIW, I find these params rather cryptic - it'd be nice to have an API for the 
helpers that is explicit about this being files/filenames


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65433/new/

https://reviews.llvm.org/D65433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66556: [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

rL370219  and rG3c307370c8f8 



Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66556/new/

https://reviews.llvm.org/D66556



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

[Balázs Benics via Phabricator via cfe-commits]

steakhal marked 2 inline comments as done.
steakhal added a comment.

Fixes for @NoQ's comments.
I will update the patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon added a comment.

I would tend to write this function in the following form:

  // set up lower bound and upper bound
  if (r > upperBound) r = upperBound;
  if (!(r >= lowerBound)) r = lowerBound; // NaN is mapped to l.b.
  return static_cast(r);

I prefer to avoid the explicit trunc call, since that's the defined behavior of 
the `static_cast` once the value is in-range, anyway.




Comment at: include/math.h:1573
+  enum { _Bits = numeric_limits<_IntT>::digits - 
numeric_limits<_FloatT>::digits };
+  static const _IntT value = numeric_limits<_IntT>::max() >> _Bits << _Bits;
+};

zoecarver wrote:
> What's the reasoning behind shifting something forward and back? Shouldn't 
> this always negate the other operation? 
This function doesn't quite do what it says on the tin; it considers the number 
of significand bits used for the floating-point type, but not the exponent 
range. This doesn't matter for double, because double's exponent range is much, 
much larger than any integer type, but it does matter for types like float16 
(largest representable value is 65504)--when it's added as a standard 
floating-point type at some future point, this will introduce subtle bugs.

You should be able to work around this by converting `value` to  `_FloatT`, 
taking the minimum of the result and numeric_limits::max, and converting back.

This also assumes that _FloatT has radix == 2, which I do not believe is 
actually implied by `is_floating_point == true`. Please add a static assert for 
that so that future decimal types don't use this template.



Comment at: include/math.h:1582
+  const _RealT __trunc_r = __builtin_trunc(__r);
+  if (__trunc_r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+return _Lim::max();

zoecarver wrote:
> Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`
Why isn't this just `__trunc_r > _MaxVal`?



Comment at: include/math.h:1584
+return _Lim::max();
+  } else if (__trunc_r <= _Lim::lowest()) {
+return _Lim::min();

This has a subtle assumption that `_IntT` is two's-complement and `_FloatT` has 
`radix=2`, so that the implicit conversion that occurs in the comparison is 
exact. The radix should be a static assert; does libc++ care about 
non-two's-complement at all?

Just from a clarity perspective, I would personally make the conversion 
explicit.



Comment at: include/math.h:1586
+return _Lim::min();
+  }
+  return static_cast<_IntT>(__trunc_r);

If I'm reading right, NaNs will fall through the above two comparisons and 
invoke UB on the static_cast below. I suspect that's not the desired behavior. 
What is the intended result for NaN?


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66836/new/

https://reviews.llvm.org/D66836



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon added inline comments.



Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:36
+  {static_cast(Lim::max()) + 1, Lim::max(), false},
+  {static_cast(Lim::max()) + 1024, Lim::max(), false},
+  };

Probably should test `nextafter(static_cast(Lim::max()), INFINITY)` 
here instead.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66836/new/

https://reviews.llvm.org/D66836



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 217665.
steakhal added a comment.

Changes:

- Flag option marked as 'enabled by default'.
- Reformat all the test cases for C, C++ and Obj C.
- Now uses `-verify=tags` approach.
- Fixes checker documentation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66733/new/

https://reviews.llvm.org/D66733

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/dead-stores.cpp
  clang/test/Analysis/dead-stores.m

Index: clang/test/Analysis/dead-stores.m
===
--- clang/test/Analysis/dead-stores.m
+++ clang/test/Analysis/dead-stores.m
@@ -1,5 +1,4 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -analyzer-checker=deadcode.DeadStores,osx.cocoa.RetainCount -fblocks -verify -Wno-objc-root-class %s
-// expected-no-diagnostics
 
 typedef signed char BOOL;
 typedef unsigned int NSUInteger;
@@ -72,7 +71,8 @@
 
 @implementation Rdar7947686_B
 - (id) init {
-  id x = (self = [super init]); // no-warning
+  id x = (self = [super init]);
+  // expected-warning@-1 {{Although the value stored to 'self'}}
   return x;
 }
 @end
Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -1,15 +1,26 @@
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11 -analyzer-checker=deadcode.DeadStores -verify -Wno-unreachable-code %s
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11 -analyzer-store=region -analyzer-checker=deadcode.DeadStores -verify -Wno-unreachable-code %s
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -verify=non-nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
+// RUN:  -analyzer-store=region -analyzer-checker=deadcode.DeadStores   \
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -Wno-unreachable-code -verify=non-nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -verify=non-nested,nested %s
 
 //===--===//
 // Basic dead store checking (but in C++ mode).
 //===--===//
 
 int j;
+int make_int();
 void test1() {
   int x = 4;
-
-  x = x + 1; // expected-warning{{never read}}
+  x = x + 1; // non-nested-warning {{never read}}
 
   switch (j) {
   case 1:
@@ -17,6 +28,11 @@
 (void)x;
 break;
   }
+
+  int y;
+  (void)y;
+  if ((y = make_int())) // nested-warning {{Although the value stored}}
+return;
 }
 
 //===--===//
@@ -25,6 +41,7 @@
 
 class Test2 {
   int &x;
+
 public:
   Test2(int &y) : x(y) {}
   ~Test2() { ++x; }
@@ -66,17 +83,17 @@
 //===--===//
 
 void test3_a(int x) {
-   x = x + 1; // expected-warning{{never read}}
+  x = x + 1; // non-nested-warning {{never read}}
 }
 
 void test3_b(int &x) {
-  x = x + 1; // no-warninge
+  x = x + 1; // no-warning
 }
 
 void test3_c(int x) {
   int &y = x;
-  // Shows the limitation of dead stores tracking.  The write is really
-  // dead since the value cannot escape the function.
+  // Shows the limitation of dead stores tracking. The write is really dead
+  // since the value cannot escape the function.
   ++y; // no-warning
 }
 
@@ -94,7 +111,7 @@
 //===--===//
 
 static void test_new(unsigned n) {
-  char **p = new char* [n]; // expected-warning{{never read}}
+  char **p = new char *[n]; // non-nested-warning {{never read}}
 }
 
 //===--===//
@@ -102,11 +119,11 @@
 //===--===//
 
 namespace foo {
-  int test_4(int x) {
-x = 2; // expected-warning{{Value stored to 'x' is never read}}
-x = 2;
-return x;
-  }
+int test_4(int x) {
+  x = 2; // non-nested-warning {{Value stored to 'x' is never read}}
+  x = 2;
+  return x;
+}
 }
 
 //===--===//
@@ -119,42 +136,39 @@
   try {
 x = 2; // no-warning
 test_5_Aux();
-  }
-  catch (int z) {
+  } catch (int z) {
 

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Looks like there will be no more comments. If so, I will update the first part 
https://reviews.llvm.org/D65130 which adds clang-offload-wrapper tool with the 
latest changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64943/new/

https://reviews.llvm.org/D64943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits