[clang-tools-extra] r368070 - [clang-doc] Add index in each info html file

[Diego Astiazaran via cfe-commits]

Author: diegoastiazaran
Date: Tue Aug  6 11:31:46 2019
New Revision: 368070

URL: http://llvm.org/viewvc/llvm-project?rev=368070&view=rev
Log:
[clang-doc] Add index in each info html file

An index structure is created while generating the output file for each
info. This structure is parsed to JSON and written to a file in the
output directory. The html for the index is not rendered by clang-doc. A
Javascript file is included in the output directory, this will the JSON
file and insert HTML elements into the file.

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

Added:
clang-tools-extra/trunk/clang-doc/assets/
clang-tools-extra/trunk/clang-doc/assets/clang-doc-default-stylesheet.css
clang-tools-extra/trunk/clang-doc/assets/index.js
clang-tools-extra/trunk/unittests/clang-doc/GeneratorTest.cpp
Removed:
clang-tools-extra/trunk/clang-doc/stylesheets/
Modified:
clang-tools-extra/trunk/clang-doc/Generators.cpp
clang-tools-extra/trunk/clang-doc/Generators.h
clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
clang-tools-extra/trunk/clang-doc/MDGenerator.cpp
clang-tools-extra/trunk/clang-doc/Representation.cpp
clang-tools-extra/trunk/clang-doc/Representation.h
clang-tools-extra/trunk/clang-doc/YAMLGenerator.cpp
clang-tools-extra/trunk/clang-doc/tool/CMakeLists.txt
clang-tools-extra/trunk/clang-doc/tool/ClangDocMain.cpp
clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.cpp
clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp

Modified: clang-tools-extra/trunk/clang-doc/Generators.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/Generators.cpp?rev=368070&r1=368069&r2=368070&view=diff
==
--- clang-tools-extra/trunk/clang-doc/Generators.cpp (original)
+++ clang-tools-extra/trunk/clang-doc/Generators.cpp Tue Aug  6 11:31:46 2019
@@ -57,6 +57,57 @@ std::string getTagType(TagTypeKind AS) {
   llvm_unreachable("Unknown TagTypeKind");
 }
 
+bool Generator::createResources(ClangDocContext &CDCtx) { return true; }
+
+// A function to add a reference to Info in Idx.
+// Given an Info X with the following namespaces: [B,A]; a reference to X will
+// be added in the children of a reference to B, which should be also a child 
of
+// a reference to A, where A is a child of Idx.
+//   Idx
+//|-- A
+//|--B
+//   |--X
+// If the references to the namespaces do not exist, they will be created. If
+// the references already exist, the same one will be used.
+void Generator::addInfoToIndex(Index &Idx, const doc::Info *Info) {
+  // Index pointer that will be moving through Idx until the first parent
+  // namespace of Info (where the reference has to be inserted) is found.
+  Index *I = &Idx;
+  // The Namespace vector includes the upper-most namespace at the end so the
+  // loop will start from the end to find each of the namespaces.
+  for (const auto &R : llvm::reverse(Info->Namespace)) {
+// Look for the current namespace in the children of the index I is
+// pointing.
+auto It = std::find(I->Children.begin(), I->Children.end(), R.USR);
+if (It != I->Children.end()) {
+  // If it is found, just change I to point the namespace refererence 
found.
+  I = &*It;
+} else {
+  // If it is not found a new reference is created
+  I->Children.emplace_back(R.USR, R.Name, R.RefType, R.Path);
+  // I is updated with the reference of the new namespace reference
+  I = &I->Children.back();
+}
+  }
+  // Look for Info in the vector where it is supposed to be; it could already
+  // exist if it is a parent namespace of an Info already passed to this
+  // function.
+  auto It = std::find(I->Children.begin(), I->Children.end(), Info->USR);
+  if (It == I->Children.end()) {
+// If it is not in the vector it is inserted
+I->Children.emplace_back(Info->USR, Info->extractName(), Info->IT,
+ Info->Path);
+  } else {
+// If it not in the vector we only check if Path and Name are not empty
+// because if the Info was included by a namespace it may not have those
+// values.
+if (It->Path.empty())
+  It->Path = Info->Path;
+if (It->Name.empty())
+  It->Name = Info->extractName();
+  }
+}
+
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the generators.
 extern volatile int YAMLGeneratorAnchorSource;

Modified: clang-tools-extra/trunk/clang-doc/Generators.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/Generators.h?rev=368070&r1=368069&r2=368070&view=diff
==
--- clang-tools-extra/trunk/clang-doc/Generators.h (original)
+++ clang-tools-extra/trunk/cla

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Just for reference, this patch still doesn't reuse the FileManager across 
invocations in a thread. We expect to get even better performance once we reuse 
it, but I'm going have to improve its API first.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63907



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:117
+std::mutex CacheLock;
+llvm::StringMap> Cache;
+  };

aganea wrote:
> Why not use a bump allocator here? (it would be per-thread) On Windows the 
> heap is always thread-safe, which induce a lock in `malloc` for each new 
> entry. You could also avoid the usage of `unique_ptr` by the same occasion:
> 
> `llvm::StringMap SpecificBumpPtrAllocator> Cache;`
> 
> //(unless you're planning on removing entries from the cache later on?)//
Good idea, I switched to a bump pointer allocator (I don't think I can use a 
specific one, as that would cause the values to be destroyed twice). I'm not 
planning on removing entries from the cache, no.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:76
+CachedFileSystemEntry
+CachedFileSystemEntry::createDirectoryEntry(llvm::vfs::Status Stat) {
+  assert(Stat.isDirectory() && "not a directory!");

aganea wrote:
> `llvm::vfs::Status &&Stat` to avoid a copy?
The copy should already be avoided, as I move the argument when passing in, and 
when it's assigned to the result.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63907



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 213664.
arphaman marked 6 inline comments as done.
arphaman added a comment.

Address review comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63907

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/lib/Tooling/DependencyScanning/CMakeLists.txt
  clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  
clang/test/ClangScanDeps/Inputs/frameworks/Framework.framework/Headers/Framework.h
  
clang/test/ClangScanDeps/Inputs/frameworks/Framework.framework/PrivateHeaders/PrivateHeader.h
  clang/test/ClangScanDeps/Inputs/header_stat_before_open_cdb.json
  clang/test/ClangScanDeps/Inputs/vfsoverlay.yaml
  clang/test/ClangScanDeps/Inputs/vfsoverlay_cdb.json
  clang/test/ClangScanDeps/header_stat_before_open.m
  clang/test/ClangScanDeps/regular_cdb.cpp
  clang/test/ClangScanDeps/vfsoverlay.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "llvm/Support/InitLLVM.h"
@@ -45,9 +46,10 @@
   ///
   /// \param Compilations The reference to the compilation database that's
   /// used by the clang tool.
-  DependencyScanningTool(const tooling::CompilationDatabase &Compilations,
+  DependencyScanningTool(DependencyScanningService &Service,
+ const tooling::CompilationDatabase &Compilations,
  SharedStream &OS, SharedStream &Errs)
-  : Compilations(Compilations), OS(OS), Errs(Errs) {}
+  : Worker(Service), Compilations(Compilations), OS(OS), Errs(Errs) {}
 
   /// Computes the dependencies for the given file and prints them out.
   ///
@@ -80,6 +82,20 @@
 
 llvm::cl::OptionCategory DependencyScannerCategory("Tool options");
 
+static llvm::cl::opt ScanMode(
+"mode",
+llvm::cl::desc("The preprocessing mode used to compute the dependencies"),
+llvm::cl::values(
+clEnumValN(ScanningMode::MinimizedSourcePreprocessing,
+   "preprocess-minimized-sources",
+   "The set of dependencies is computed by preprocessing the "
+   "source files that were minimized to only include the "
+   "contents that might affect the dependencies"),
+clEnumValN(ScanningMode::CanonicalPreprocessing, "preprocess",
+   "The set of dependencies is computed by preprocessing the "
+   "unmodified source files")),
+llvm::cl::init(ScanningMode::MinimizedSourcePreprocessing));
+
 llvm::cl::opt
 NumThreads("j", llvm::cl::Optional,
llvm::cl::desc("Number of worker threads to use (default: use "
@@ -136,12 +152,14 @@
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
   SharedStream DependencyOS(llvm::outs());
+
+  DependencyScanningService Service(ScanMode);
   unsigned NumWorkers =
   NumThreads == 0 ? llvm::hardware_concurrency() : NumThreads;
   std::vector> WorkerTools;
   for (unsigned I = 0; I < NumWorkers; ++I)
 WorkerTools.push_back(llvm::make_unique(
-*AdjustingCompilations, DependencyOS, Errs));
+Service, *AdjustingCompilations, DependencyOS, Errs));
 
   std::vector WorkerThreads;
   std::atomic HadErrors(false);
Index: clang/test/ClangScanDeps/vfsoverlay.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/vfsoverlay.cpp
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/vfsoverlay.cpp
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/vfsoverlay.yaml > %t.dir/vfsoverlay.yaml
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/vfsoverlay_cdb.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | \
+// RUN:   FileCheck %s
+
+#include "not_real.h"
+
+// CHECK: clang-scan-deps dependency
+// CHECK-NEXT: vfsoverlay.cpp
+// CHECK-NEXT: Inputs{{/|\\}}header.h
Index: clang/test/ClangScanDeps/regular_cdb.cpp

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

[Steven Noonan via Phabricator via cfe-commits]

tycho added a comment.

OK, that makes sense. So this change would not enforce `-O2`/`-O3` for the 
bitcode emission, but would enforce one of the two for the LTO phase.

This may be a stupid question, but aren't there some optimization passes that 
can emit functions during the LTO phase that weren't in the initial bitcode 
compile? If so, wouldn't those miss out on getting the `-Os`/`-Oz` function 
attributes applied?

What would happen after D63976  is applied 
with a funny command line like `clang -Oz -flto -o foo a.o b.c`, where one (or 
all) of the input arguments is a source file? Would it invoke the bitcode 
compile of the source files with the requested `-Oz` and then invoke the LTO 
linker plugin with `-O2`/`-O3`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63976



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

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

> I assume I might be missing something here, though, since someone mentioned 
> this above (I don't understand the response to it though).

There are two invocations in LTO: the first phase where we compile from source 
to bitcode, and the second phase which is invoked by the linker.

Phase 1: compile to bitcode
===

clang++ -Os -flto foo.c -o foo.o
clang++ -O2 -flto bar.c -o bar.o

Phase 2: LTO and CodeGen


clang++  bar.o foo.o

When compiling foo.c, we use Os which add a function attribute to make each 
function in foo.o as such. Functions in bar.o won't have the same attributes.
During LTO we merge everything but functions keep their attributes, the 
optimization passes can adjust their heuristics to honor the fact that 
functions from foo.c will be "optimized for size" and the function from bar.c 
will be optimized "normally".

Specifying an optimization level for the LTO phase is a also bit awkward since 
the LTO optimization pipeline is already very different from the non-LTO one: 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp#L1012

What remains are CodeGen heuristics, and there might still have some global 
flags in use: 
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/CodeGen.h#L51
 (note nothing specific to Os/Oz though), still in use for example for 
generating FMAs on AArch64 apparently: 
https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp#L55

This is why specifying Oz/Os during LTO can be very confusing for the user: it 
would change very few things in the process without actually making function 
from bar.c having the right function attributes (nothing would override the 
lack of attribute as far as I know): `clang++ -flto -Oz bar.o foo.o` would 
*not* add the Oz annotation on functions defined in bar.o.

> I'm curious why we would want to force -O2/-O3 instead of just allowing 
> -Os/-Oz to be used with LTO.

So I hope it is more clear that I don't think we're forcing O2 
/O3 
 on LTO users, but it isn't obvious 
to expose a consistent UI for these flags with respect to LTO without being 
surprising to the user in some way.
(do you know that LTO use to be -O4?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D63976



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

[PATCH] D64696: Adds a warning when an inline Doxygen comment has no argument

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

Unfortunately I'm not able to quickly find the Doxygen output of Swift online. 
When I process:
`A limited variant of \c @objc that's used for classes with generic ancestry.`
with Doxygen I get:
`A limited variant of that's used for classes with generic ancestry.`
Since the `@` is used for commands I think they should be escaped like:
include/swift/AST/Decl.h: /// such as \c \@testable and \c \@usableFromInline 
into account.

Can you verify whether the Doxygen output for the Swift documentation is 
correct without escaping the `\c @foo` ? Or do you have a link to the online 
version?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64696



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

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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368062: [Syntax] Do not add a node for 'eof' into 
the tree (authored by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64576?vs=209270&id=213650#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64576

Files:
  cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
  cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp


Index: cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
===
--- cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
+++ cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
@@ -58,8 +58,11 @@
   /// Finish building the tree and consume the root node.
   syntax::TranslationUnit *finalize() && {
 auto Tokens = Arena.tokenBuffer().expandedTokens();
+assert(!Tokens.empty());
+assert(Tokens.back().kind() == tok::eof);
+
 // Build the root of the tree, consuming all the children.
-Pending.foldChildren(Tokens,
+Pending.foldChildren(Tokens.drop_back(),
  new (Arena.allocator()) syntax::TranslationUnit);
 
 return cast(std::move(Pending).finalize());
@@ -96,10 +99,11 @@
   /// Ensures that added nodes properly nest and cover the whole token stream.
   struct Forest {
 Forest(syntax::Arena &A) {
-  // FIXME: do not add 'eof' to the tree.
-
+  assert(!A.tokenBuffer().expandedTokens().empty());
+  assert(A.tokenBuffer().expandedTokens().back().kind() == tok::eof);
   // Create all leaf nodes.
-  for (auto &T : A.tokenBuffer().expandedTokens())
+  // Note that we do not have 'eof' in the tree.
+  for (auto &T : A.tokenBuffer().expandedTokens().drop_back())
 Trees.insert(Trees.end(),
  {&T, NodeAndRole{new (A.allocator()) syntax::Leaf(&T)}});
 }
Index: cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp
===
--- cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp
+++ cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp
@@ -138,15 +138,14 @@
 | `-CompoundStatement
 |   |-2: {
 |   `-3: }
-|-TopLevelDeclaration
-| |-void
-| |-foo
-| |-(
-| |-)
-| `-CompoundStatement
-|   |-2: {
-|   `-3: }
-`-
+`-TopLevelDeclaration
+  |-void
+  |-foo
+  |-(
+  |-)
+  `-CompoundStatement
+|-2: {
+`-3: }
 )txt"},
   };
 


Index: cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
===
--- cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
+++ cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
@@ -58,8 +58,11 @@
   /// Finish building the tree and consume the root node.
   syntax::TranslationUnit *finalize() && {
 auto Tokens = Arena.tokenBuffer().expandedTokens();
+assert(!Tokens.empty());
+assert(Tokens.back().kind() == tok::eof);
+
 // Build the root of the tree, consuming all the children.
-Pending.foldChildren(Tokens,
+Pending.foldChildren(Tokens.drop_back(),
  new (Arena.allocator()) syntax::TranslationUnit);
 
 return cast(std::move(Pending).finalize());
@@ -96,10 +99,11 @@
   /// Ensures that added nodes properly nest and cover the whole token stream.
   struct Forest {
 Forest(syntax::Arena &A) {
-  // FIXME: do not add 'eof' to the tree.
-
+  assert(!A.tokenBuffer().expandedTokens().empty());
+  assert(A.tokenBuffer().expandedTokens().back().kind() == tok::eof);
   // Create all leaf nodes.
-  for (auto &T : A.tokenBuffer().expandedTokens())
+  // Note that we do not have 'eof' in the tree.
+  for (auto &T : A.tokenBuffer().expandedTokens().drop_back())
 Trees.insert(Trees.end(),
  {&T, NodeAndRole{new (A.allocator()) syntax::Leaf(&T)}});
 }
Index: cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp
===
--- cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp
+++ cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp
@@ -138,15 +138,14 @@
 | `-CompoundStatement
 |   |-2: {
 |   `-3: }
-|-TopLevelDeclaration
-| |-void
-| |-foo
-| |-(
-| |-)
-| `-CompoundStatement
-|   |-2: {
-|   `-3: }
-`-
+`-TopLevelDeclaration
+  |-void
+  |-foo
+  |-(
+  |-)
+  `-CompoundStatement
+|-2: {
+`-3: }
 )txt"},
   };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r368062 - [Syntax] Do not add a node for 'eof' into the tree

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Tue Aug  6 10:07:58 2019
New Revision: 368062

URL: http://llvm.org/viewvc/llvm-project?rev=368062&view=rev
Log:
[Syntax] Do not add a node for 'eof' into the tree

Summary:
While useful as a sentinel value when iterating over tokens, having
'eof' in the tree, seems to do more harm than good.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: javed.absar, kristof.beyls, cfe-commits

Tags: #clang

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

Modified:
cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp

Modified: cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp?rev=368062&r1=368061&r2=368062&view=diff
==
--- cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp (original)
+++ cfe/trunk/lib/Tooling/Syntax/BuildTree.cpp Tue Aug  6 10:07:58 2019
@@ -58,8 +58,11 @@ public:
   /// Finish building the tree and consume the root node.
   syntax::TranslationUnit *finalize() && {
 auto Tokens = Arena.tokenBuffer().expandedTokens();
+assert(!Tokens.empty());
+assert(Tokens.back().kind() == tok::eof);
+
 // Build the root of the tree, consuming all the children.
-Pending.foldChildren(Tokens,
+Pending.foldChildren(Tokens.drop_back(),
  new (Arena.allocator()) syntax::TranslationUnit);
 
 return cast(std::move(Pending).finalize());
@@ -96,10 +99,11 @@ private:
   /// Ensures that added nodes properly nest and cover the whole token stream.
   struct Forest {
 Forest(syntax::Arena &A) {
-  // FIXME: do not add 'eof' to the tree.
-
+  assert(!A.tokenBuffer().expandedTokens().empty());
+  assert(A.tokenBuffer().expandedTokens().back().kind() == tok::eof);
   // Create all leaf nodes.
-  for (auto &T : A.tokenBuffer().expandedTokens())
+  // Note that we do not have 'eof' in the tree.
+  for (auto &T : A.tokenBuffer().expandedTokens().drop_back())
 Trees.insert(Trees.end(),
  {&T, NodeAndRole{new (A.allocator()) syntax::Leaf(&T)}});
 }

Modified: cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp?rev=368062&r1=368061&r2=368062&view=diff
==
--- cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/Syntax/TreeTest.cpp Tue Aug  6 10:07:58 2019
@@ -138,15 +138,14 @@ void foo() {}
 | `-CompoundStatement
 |   |-2: {
 |   `-3: }
-|-TopLevelDeclaration
-| |-void
-| |-foo
-| |-(
-| |-)
-| `-CompoundStatement
-|   |-2: {
-|   `-3: }
-`-
+`-TopLevelDeclaration
+  |-void
+  |-foo
+  |-(
+  |-)
+  `-CompoundStatement
+|-2: {
+`-3: }
 )txt"},
   };
 


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

[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Shaurya Gupta via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368058: Fixed toHalfOpenFileRange assertion fail (authored 
by SureYeaah, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65754?vs=213614&id=213645#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65754

Files:
  clang-tools-extra/trunk/clangd/SourceCode.cpp
  clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp

Index: clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp
@@ -460,15 +460,22 @@
 #define FOO(X, Y) int Y = ++X
 #define BAR(X) X + 1
 #define ECHO(X) X
+
+#define BUZZ BAZZ(ADD)
+#define BAZZ(m) m(1)
+#define ADD(a) int f = a + 1;
 template
 class P {};
-void f() {
+
+int main() {
   $a[[P a]];
   $b[[int b = 1]];
   $c[[FOO(b, c)]]; 
   $d[[FOO(BAR(BAR(b)), d)]];
   // FIXME: We might want to select everything inside the outer ECHO.
   ECHO(ECHO($e[[int) ECHO(e]]));
+  // Shouldn't crash.
+  $f[[BUZZ]];
 }
   )cpp");
 
@@ -495,6 +502,7 @@
   CheckRange("c");
   CheckRange("d");
   CheckRange("e");
+  CheckRange("f");
 }
 
 } // namespace
Index: clang-tools-extra/trunk/clangd/SourceCode.cpp
===
--- clang-tools-extra/trunk/clangd/SourceCode.cpp
+++ clang-tools-extra/trunk/clangd/SourceCode.cpp
@@ -296,14 +296,52 @@
  E1 < E2 ? R2.getEnd() : R1.getEnd());
 }
 
-// Returns the tokenFileRange for a given Location as a Token Range
+// Check if two locations have the same file id.
+static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
+   const SourceManager &SM) {
+  return SM.getFileID(Loc1) == SM.getFileID(Loc2);
+}
+
+// Find an expansion range (not necessarily immediate) the ends of which are in
+// the same file id.
+static SourceRange
+getExpansionTokenRangeInSameFile(SourceLocation Loc, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+  SourceRange ExpansionRange =
+  toTokenRange(SM.getImmediateExpansionRange(Loc), SM, LangOpts);
+  // Fast path for most common cases.
+  if (inSameFile(ExpansionRange.getBegin(), ExpansionRange.getEnd(), SM))
+return ExpansionRange;
+  // Record the stack of expansion locations for the beginning, keyed by FileID.
+  llvm::DenseMap BeginExpansions;
+  for (SourceLocation Begin = ExpansionRange.getBegin(); Begin.isValid();
+   Begin = Begin.isFileID()
+   ? SourceLocation()
+   : SM.getImmediateExpansionRange(Begin).getBegin()) {
+BeginExpansions[SM.getFileID(Begin)] = Begin;
+  }
+  // Move up the stack of expansion locations for the end until we find the
+  // location in BeginExpansions with that has the same file id.
+  for (SourceLocation End = ExpansionRange.getEnd(); End.isValid();
+   End = End.isFileID() ? SourceLocation()
+: toTokenRange(SM.getImmediateExpansionRange(End),
+   SM, LangOpts)
+  .getEnd()) {
+auto It = BeginExpansions.find(SM.getFileID(End));
+if (It != BeginExpansions.end())
+  return {It->second, End};
+  }
+  llvm_unreachable(
+  "We should able to find a common ancestor in the expansion tree.");
+}
+// Returns the file range for a given Location as a Token Range
 // This is quite similar to getFileLoc in SourceManager as both use
 // getImmediateExpansionRange and getImmediateSpellingLoc (for macro IDs).
 // However:
 // - We want to maintain the full range information as we move from one file to
 //   the next. getFileLoc only uses the BeginLoc of getImmediateExpansionRange.
-// - We want to split '>>' tokens as the lexer parses the '>>' in template
-//   instantiations as a '>>' instead of a '>'.
+// - We want to split '>>' tokens as the lexer parses the '>>' in nested
+//   template instantiations as a '>>' instead of two '>'s.
 // There is also getExpansionRange but it simply calls
 // getImmediateExpansionRange on the begin and ends separately which is wrong.
 static SourceRange getTokenFileRange(SourceLocation Loc,
@@ -311,16 +349,19 @@
  const LangOptions &LangOpts) {
   SourceRange FileRange = Loc;
   while (!FileRange.getBegin().isFileID()) {
-assert(!FileRange.getEnd().isFileID() &&
-   "Both Begin and End should be MacroIDs.");
 if (SM.isMacroArgExpansion(FileRange.getBegin())) {
-  FileRange.setBegin(SM.getImmediateSpellingLoc(FileRange.getBegin()));
-  FileRange.setEnd(SM.getImmediateSpellingLoc(FileRan

[clang-tools-extra] r368058 - Fixed toHalfOpenFileRange assertion fail

[Shaurya Gupta via cfe-commits]

Author: sureyeaah
Date: Tue Aug  6 10:01:12 2019
New Revision: 368058

URL: http://llvm.org/viewvc/llvm-project?rev=368058&view=rev
Log:
Fixed toHalfOpenFileRange assertion fail

Summary:
- Added new function that gets Expansion range with both ends in the same file.
- Fixes the crash at https://github.com/clangd/clangd/issues/113

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

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/SourceCode.cpp
clang-tools-extra/trunk/clangd/unittests/SourceCodeTests.cpp

Modified: clang-tools-extra/trunk/clangd/SourceCode.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/SourceCode.cpp?rev=368058&r1=368057&r2=368058&view=diff
==
--- clang-tools-extra/trunk/clangd/SourceCode.cpp (original)
+++ clang-tools-extra/trunk/clangd/SourceCode.cpp Tue Aug  6 10:01:12 2019
@@ -296,14 +296,52 @@ static SourceRange unionTokenRange(Sourc
  E1 < E2 ? R2.getEnd() : R1.getEnd());
 }
 
-// Returns the tokenFileRange for a given Location as a Token Range
+// Check if two locations have the same file id.
+static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
+   const SourceManager &SM) {
+  return SM.getFileID(Loc1) == SM.getFileID(Loc2);
+}
+
+// Find an expansion range (not necessarily immediate) the ends of which are in
+// the same file id.
+static SourceRange
+getExpansionTokenRangeInSameFile(SourceLocation Loc, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+  SourceRange ExpansionRange =
+  toTokenRange(SM.getImmediateExpansionRange(Loc), SM, LangOpts);
+  // Fast path for most common cases.
+  if (inSameFile(ExpansionRange.getBegin(), ExpansionRange.getEnd(), SM))
+return ExpansionRange;
+  // Record the stack of expansion locations for the beginning, keyed by 
FileID.
+  llvm::DenseMap BeginExpansions;
+  for (SourceLocation Begin = ExpansionRange.getBegin(); Begin.isValid();
+   Begin = Begin.isFileID()
+   ? SourceLocation()
+   : SM.getImmediateExpansionRange(Begin).getBegin()) {
+BeginExpansions[SM.getFileID(Begin)] = Begin;
+  }
+  // Move up the stack of expansion locations for the end until we find the
+  // location in BeginExpansions with that has the same file id.
+  for (SourceLocation End = ExpansionRange.getEnd(); End.isValid();
+   End = End.isFileID() ? SourceLocation()
+: toTokenRange(SM.getImmediateExpansionRange(End),
+   SM, LangOpts)
+  .getEnd()) {
+auto It = BeginExpansions.find(SM.getFileID(End));
+if (It != BeginExpansions.end())
+  return {It->second, End};
+  }
+  llvm_unreachable(
+  "We should able to find a common ancestor in the expansion tree.");
+}
+// Returns the file range for a given Location as a Token Range
 // This is quite similar to getFileLoc in SourceManager as both use
 // getImmediateExpansionRange and getImmediateSpellingLoc (for macro IDs).
 // However:
 // - We want to maintain the full range information as we move from one file to
 //   the next. getFileLoc only uses the BeginLoc of getImmediateExpansionRange.
-// - We want to split '>>' tokens as the lexer parses the '>>' in template
-//   instantiations as a '>>' instead of a '>'.
+// - We want to split '>>' tokens as the lexer parses the '>>' in nested
+//   template instantiations as a '>>' instead of two '>'s.
 // There is also getExpansionRange but it simply calls
 // getImmediateExpansionRange on the begin and ends separately which is wrong.
 static SourceRange getTokenFileRange(SourceLocation Loc,
@@ -311,16 +349,19 @@ static SourceRange getTokenFileRange(Sou
  const LangOptions &LangOpts) {
   SourceRange FileRange = Loc;
   while (!FileRange.getBegin().isFileID()) {
-assert(!FileRange.getEnd().isFileID() &&
-   "Both Begin and End should be MacroIDs.");
 if (SM.isMacroArgExpansion(FileRange.getBegin())) {
-  FileRange.setBegin(SM.getImmediateSpellingLoc(FileRange.getBegin()));
-  FileRange.setEnd(SM.getImmediateSpellingLoc(FileRange.getEnd()));
+  FileRange = unionTokenRange(
+  SM.getImmediateSpellingLoc(FileRange.getBegin()),
+  SM.getImmediateSpellingLoc(FileRange.getEnd()), SM, LangOpts);
+  assert(inSameFile(FileRange.getBegin(), FileRange.getEnd(), SM));
 } else {
-  SourceRange ExpansionRangeForBegin = toTokenRange(
-  SM.getImmediateExpansionRange(FileRange.getBegin()), SM, LangOpts);
-  SourceRange ExpansionRangeForEnd = toTokenRange(
-  SM.getImmediateExpansionRange(FileRange.getEnd()), SM, LangOpts);
+  SourceRange ExpansionRangeForBegin =
+  getExpansionTokenRangeInSameFile(FileRange.getBegin(), 

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

[Sergey Dmitriev via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Driver/Action.h:74
 OffloadUnbundlingJobClass,
+OffloadWrapperJobClass,
 

ABataev wrote:
> Do we really need this new kind of job here, can we use bundler instead?
Well, I can probably try to reuse bundling action for wrapping, but I think it 
will just complicate the logic. Wrapping logically differs from bundling and 
wrapping is done by a different tool, so I think it is natural to add a 
distinct action class for it.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9748
+  // If we have offloading in the current module, we need to emit the entries.
   createOffloadEntriesAndInfoMetadata();
 }

ABataev wrote:
> Do not emit it for the devices and simd only mode. Also, would be good to 
> assert if no devices triples were specified.
Offload entries are actually emitted both for host and target compilations. I 
have added a check for OpenMP simd mode to 
createOffloadingBinaryDescriptorRegistration().



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:1470
+  /// was emitted in the current module.
+  virtual void emitOffloadTables();
 

ABataev wrote:
> Ithink, you can drop `virtual` here and remove overridden version from the 
> CGOpenMPRuntimeSimd. Instead, just check for OpenMP simd mode in the original 
> function and just early exit in this case.
Ok. Without virtual I do not see much reasons for adding new function which 
just calls createOffloadEntriesAndInfoMetadata(), so instead I have just made 
createOffloadEntriesAndInfoMetadata() public and added a check for OpenMP simd 
mode to this function.


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

[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Sam McCall via Phabricator via cfe-commits]

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

Sorry, should have accepted this in the last round. Great stuff, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65754



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

[PATCH] D65776: [Clang] Pragma vectorize_predicate implies vectorize

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D65776#1615834 , @Meinersbur wrote:

> Mmmh, I would have expected this to work the same way as `vectorize_width`. 
> According to the docs:
>
> > The following example implicitly enables vectorization and interleaving by 
> > specifying a vector width and interleaving count:
> >  `#pragma clang loop vectorize_width(2) interleave_count(2)`
> >  `for(...) {`
> >  ` ...`
> >  `}`
>
> However, `vectorize_width` does not automatically set 
> `llvm.loop.vectorize.enable`. Neither does `llvm.loop.vectorize.width` > 1 
> imply `LoopVectorizeHints::getForce()`. At some places they are checked 
> together, such as `LoopVectorizeHints::allowReordering()`. Other places, 
> notably `LoopVectorizationCostModel::selectVectorizationFactor()`, only 
> queries `getForce()`. That is, `vectorize_width(2)` does not implicitly force 
> vectorization in practice.


The current behavior with respect to vectorize_width seems a bit 
counterintuitive. The docs say `vectorize_width implicitly enables 
vectorization`, I would expect it to behave as if `vectorize(enable)` was also 
passed. There's also a PR about that mismatch: 
https://bugs.llvm.org/show_bug.cgi?id=27643

IMO it would make sense to have the more specific pragmas imply 
vectorize(enable) here (or update the docs accordingly).


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

https://reviews.llvm.org/D65776



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

[PATCH] D65690: [clang-doc] Add index in each info html file

[Julie Hockett via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D65690



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

Re: AST for For loop

[Monalisa Rout via cfe-commits]

Awesome, thanks!

On Tue, Aug 6, 2019 at 5:18 PM Aaron Ballman  wrote:

> On Tue, Aug 6, 2019 at 11:15 AM Monalisa Rout via cfe-commits
>  wrote:
> >
> > Hello,
> > While dumping the AST for For loop, Why do I get  <<>> ??
> >
> > Source code:
> > void func()
> > {
> > for (int i = 0; i < 5; i++)
> > break;
> > }
> >
> > AST
> >
> > FunctionDecl 0x4a4ac10
> 
> line:2:6 func 'void ()'
> > `-CompoundStmt 0x4a4ae1c 
> >   `-ForStmt 0x4a4adf8 
> > |-DeclStmt 0x4a4ad38 
> > | `-VarDecl 0x4a4acd0  col:11 used i 'int' cinit
> > |   `-IntegerLiteral 0x4a4ad10  'int' 0
> > |-<<>>
> > |-BinaryOperator 0x4a4ada8  'int' '<'
> > | |-ImplicitCastExpr 0x4a4ad98  'int' 
> > | | `-DeclRefExpr 0x4a4ad50  'int' lvalue Var 0x4a4acd0 'i'
> 'int'
> > | `-IntegerLiteral 0x4a4ad70  'int' 5
> > |-UnaryOperator 0x4a4ade0  'int' postfix '++'
> > | `-DeclRefExpr 0x4a4adc0  'int' lvalue Var 0x4a4acd0 'i'
> 'int'
> > `-BreakStmt 0x4a4adf0 
>
> The AST dumps child nodes by calling children() on a given AST node.
> The ForStmt node always has five child nodes, which are (in order):
> init statement, condition variable, condition, increment, and body.
> Some of those may be NULL depending on the format of the for loop
> (your for loop example has no condition variable, for instance).
>
> HTH!
>
> ~Aaron
>
> >
> > Regards,
> > Mona
> > ___
> > 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] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D65065#1617079 , @gribozavr wrote:

> `-Weverything` is not recommended for anything except compiler testing, and 
> similarly with clang-tidy, enabling all checkers is not a good idea. I don't 
> think improving this particular point will make enabling all checks more 
> usable.


That is beyond the point here (and is very arguable).
Even just enabling all `cert-*` checks will result in this behavior.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368052: [AST] Traverse attributes inside DEF_TRAVERSE_DECL 
macro (authored by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64907?vs=213558&id=213624#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64907

Files:
  cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
  cfe/trunk/unittests/AST/CMakeLists.txt
  cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp

Index: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
===
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
@@ -722,12 +722,6 @@
 break;
 #include "clang/AST/DeclNodes.inc"
   }
-
-  // Visit any attributes attached to this declaration.
-  for (auto *I : D->attrs()) {
-if (!getDerived().TraverseAttr(I))
-  return false;
-  }
   return true;
 }
 
@@ -1407,6 +1401,11 @@
 { CODE; }  \
 if (ReturnValue && ShouldVisitChildren)\
   TRY_TO(TraverseDeclContextHelper(dyn_cast(D))); \
+if (ReturnValue) { \
+  /* Visit any attributes attached to this declaration. */ \
+  for (auto *I : D->attrs())   \
+TRY_TO(getDerived().TraverseAttr(I));  \
+}  \
 if (ReturnValue && getDerived().shouldTraversePostOrder()) \
   TRY_TO(WalkUpFrom##DECL(D)); \
 return ReturnValue;\
Index: cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp
===
--- cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp
+++ cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp
@@ -0,0 +1,106 @@
+//===- unittest/AST/RecursiveASTVisitorTest.cpp ---===//
+//
+// 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 "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock-generated-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace clang;
+using ::testing::ElementsAre;
+
+namespace {
+class ProcessASTAction : public clang::ASTFrontendAction {
+public:
+  ProcessASTAction(llvm::unique_function Process)
+  : Process(std::move(Process)) {
+assert(this->Process);
+  }
+
+  std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) {
+class Consumer : public ASTConsumer {
+public:
+  Consumer(llvm::function_ref Process)
+  : Process(Process) {}
+
+  void HandleTranslationUnit(ASTContext &Ctx) override { Process(Ctx); }
+
+private:
+  llvm::function_ref Process;
+};
+
+return llvm::make_unique(Process);
+  }
+
+private:
+  llvm::unique_function Process;
+};
+
+enum class VisitEvent {
+  StartTraverseFunction,
+  EndTraverseFunction,
+  StartTraverseAttr,
+  EndTraverseAttr
+};
+
+class CollectInterestingEvents
+: public RecursiveASTVisitor {
+public:
+  bool TraverseFunctionDecl(FunctionDecl *D) {
+Events.push_back(VisitEvent::StartTraverseFunction);
+bool Ret = RecursiveASTVisitor::TraverseFunctionDecl(D);
+Events.push_back(VisitEvent::EndTraverseFunction);
+
+return Ret;
+  }
+
+  bool TraverseAttr(Attr *A) {
+Events.push_back(VisitEvent::StartTraverseAttr);
+bool Ret = RecursiveASTVisitor::TraverseAttr(A);
+Events.push_back(VisitEvent::EndTraverseAttr);
+
+return Ret;
+  }
+
+  std::vector takeEvents() && { return std::move(Events); }
+
+private:
+  std::vector Events;
+};
+
+std::vector collectEvents(llvm::StringRef Code) {
+  CollectInterestingEvents Visitor;
+  clang::tooling::runToolOnCode(
+  new ProcessASTAction(
+  [&](clang::ASTContext &Ctx) { Visitor.TraverseAST(Ctx); }),
+  Code);
+  return std::move(Visitor).takeEvents();
+}
+} // namespace
+
+TEST(RecursiveASTVisitorTest, AttributesInsideDecls) {
+  /// Check attributes are traversed ins

r368052 - [AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Tue Aug  6 08:46:12 2019
New Revision: 368052

URL: http://llvm.org/viewvc/llvm-project?rev=368052&view=rev
Log:
[AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

Summary:
Instead of traversing inside the TraverseDecl() function.
Previously the attributes were traversed after Travese(Some)Decl
returns.

Logically attributes are properties of particular Decls and should be
traversed alongside other "child" nodes.

None of the tests relied on this behavior, hopefully this is an indication
that the change is relatively safe.

This change started with a discussion on cfe-dev, for details see:
https://lists.llvm.org/pipermail/cfe-dev/2019-July/062899.html

Reviewers: rsmith, gribozavr

Reviewed By: gribozavr

Subscribers: mgorny, cfe-commits

Tags: #clang

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

Added:
cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp
Modified:
cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
cfe/trunk/unittests/AST/CMakeLists.txt

Modified: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=368052&r1=368051&r2=368052&view=diff
==
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h (original)
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h Tue Aug  6 08:46:12 2019
@@ -722,12 +722,6 @@ bool RecursiveASTVisitor::Trave
 break;
 #include "clang/AST/DeclNodes.inc"
   }
-
-  // Visit any attributes attached to this declaration.
-  for (auto *I : D->attrs()) {
-if (!getDerived().TraverseAttr(I))
-  return false;
-  }
   return true;
 }
 
@@ -1407,6 +1401,11 @@ bool RecursiveASTVisitor::Trave
 { CODE; }  
\
 if (ReturnValue && ShouldVisitChildren)
\
   TRY_TO(TraverseDeclContextHelper(dyn_cast(D))); 
\
+if (ReturnValue) { 
\
+  /* Visit any attributes attached to this declaration. */ 
\
+  for (auto *I : D->attrs())   
\
+TRY_TO(getDerived().TraverseAttr(I));  
\
+}  
\
 if (ReturnValue && getDerived().shouldTraversePostOrder()) 
\
   TRY_TO(WalkUpFrom##DECL(D)); 
\
 return ReturnValue;
\

Modified: cfe/trunk/unittests/AST/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/CMakeLists.txt?rev=368052&r1=368051&r2=368052&view=diff
==
--- cfe/trunk/unittests/AST/CMakeLists.txt (original)
+++ cfe/trunk/unittests/AST/CMakeLists.txt Tue Aug  6 08:46:12 2019
@@ -26,6 +26,7 @@ add_clang_unittest(ASTTests
   Language.cpp
   NamedDeclPrinterTest.cpp
   OMPStructuredBlockTest.cpp
+  RecursiveASTVisitorTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   StructuralEquivalenceTest.cpp

Added: cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp?rev=368052&view=auto
==
--- cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp (added)
+++ cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp Tue Aug  6 08:46:12 2019
@@ -0,0 +1,106 @@
+//===- unittest/AST/RecursiveASTVisitorTest.cpp 
---===//
+//
+// 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 "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock-generated-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace clang;
+using ::testing::ElementsAre;
+
+namespace {
+class ProcessASTAction : public clang::ASTFrontendAction {
+public:
+  ProcessASTAction(llvm::unique_function Process)
+  : Process(std::move(Process)) {
+assert(this->Process);
+  }
+
+  std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) {
+class Consumer : public ASTConsumer {
+public:
+  Consumer(llvm::function_ref Process)
+  : Process(P

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7077
+//   someContainer.add(std::move(localOWner));
+//   return p;
+if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) &&

gribozavr wrote:
> Why is it a false positive? `std::move` left memory owned by `localOwner` in 
> unspecified state.
I saw user code relying on the semantics of certain classes. E.g. they assume 
if a `std::unique_ptr` is moved the pointee is still in place, so it is safe to 
return a reference to the pointee. Do you think those cases should be diagnosed 
too?


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

https://reviews.llvm.org/D64256



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

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

`-Weverything` is not recommended for anything except compiler testing, and 
similarly with clang-tidy, enabling all checkers is not a good idea. I don't 
think improving this particular point will make enabling all checks more usable.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

[PATCH] D63907: [clang-scan-deps] Implementation of dependency scanner over minimized sources

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for the update Alex! Just a few more comments and we should be good to 
go:




Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h:117
+std::mutex CacheLock;
+llvm::StringMap> Cache;
+  };

Why not use a bump allocator here? (it would be per-thread) On Windows the heap 
is always thread-safe, which induce a lock in `malloc` for each new entry. You 
could also avoid the usage of `unique_ptr` by the same occasion:

`llvm::StringMap> Cache;`

//(unless you're planning on removing entries from the cache later on?)//



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:76
+CachedFileSystemEntry
+CachedFileSystemEntry::createDirectoryEntry(llvm::vfs::Status Stat) {
+  assert(Stat.isDirectory() && "not a directory!");

`llvm::vfs::Status &&Stat` to avoid a copy?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:103
+  auto It =
+  Shard.Cache.try_emplace(Key, std::unique_ptr());
+  auto &Ptr = It.first->getValue();

`Shard.Cache.try_emplace(Key)` will provide a default constructed value if it's 
not there.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:117
+  // Check the local cache first.
+  if (const auto *Entry = getCachedEntry(Filename))
+return Entry->getStatus();

`CachedFileSystemEntry *Entry` ?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:198
+  // Check the local cache first.
+  if (const auto *Entry = getCachedEntry(Filename))
+return createFile(Entry);

CachedFileSystemEntry *Entry ?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:148
+  RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
+  if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
+DepFS = new DependencyScanningFilesystem(Service.getSharedCache(), RealFS);

arphaman wrote:
> aganea wrote:
> > arphaman wrote:
> > > aganea wrote:
> > > > Can we not use caching all the time?
> > > We want to have a mode where it's as close to the regular `clang -E` 
> > > invocation as possible for correctness CI and testing. We also haven't 
> > > evaluated if the cost of keeping non-minimized sources in memory, so it 
> > > might be too expensive for practical use? I can add a third option that 
> > > caches but doesn't minimize though as a follow-up patch though 
> > > 
> > Yes that would be nice. As for the size taken in RAM, it would be only .H 
> > files, right? For Clang+LLVM+LLD I'm counting about 40 MB. But indeed with 
> > a large project, that would be about 1.5 GB of .H. Although I doubt all 
> > these files will be loaded at once in memory (I'll check)
> > 
> > As for the usage: Fastbuild works like distcc (plain mode, not pump) so we 
> > were also planning on extracting the fully preprocessed output, not only 
> > the dependencies. There's one use-case where we need to preprocess locally, 
> > then send the preprocessed output remotely for compilation. And another 
> > use-case where we only want to extract the dependency list, compute a 
> > digest, to retrieve the OBJ from a network cache.
> Right now it doesn't differentiate between .H and other files, but we could 
> teach it do have a header only filter for the cache.
No worries, I was simply wondering about the size in memory.


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

https://reviews.llvm.org/D63907



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

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D65065#1617031 , @gribozavr wrote:

> > This suggestion would result another strange behavior: if the user disables 
> > cert-err09-cpp because he or she doesn't want to see its reports, the other 
> > one (cert-err61-cpp) will still report the issue. So he or she has to 
> > disable both (or as many aliases it has).
>
> That seems to be the case regardless of the implementation strategy in this 
> patch.
>
> > This could be another advantage of this patch, since it would be annoying 
> > to see a message in which a GCC-style checker warns about an LLVM-style 
> > violation.
>
> I don't understand the scenario. I think people wouldn't enable both checkers 
> in the first place.


They can silently get enabled if you follow the -Weverything approach - enable 
all checks, and selectively disable.
Current clang-tidy check aliasing really looks misdesigned..


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6581
+if (!Callee->getIdentifier()) {
+  auto OO = Callee->getOverloadedOperator();
+  return OO == OverloadedOperatorKind::OO_Subscript ||

gribozavr wrote:
> xazax.hun wrote:
> > gribozavr wrote:
> > > mgehre wrote:
> > > > xazax.hun wrote:
> > > > > If we want to relax the warnings to give more results we could extend 
> > > > > the checking of these overloaded operators for annotated types. But 
> > > > > this would imply that the user need to have the expected semantics 
> > > > > for those types and can only suppress false positives by removing 
> > > > > some gsl:::owner/poinnter annotations.
> > > > I see those options:
> > > > - Either gsl::Owner implies some specific form of those operators (and 
> > > > if that does not hold for a class, then one should not annotate it with 
> > > > gsl::Owner)
> > > > - or gsl::Owner only implies some specific behavior for the 
> > > > "gsl::Pointer constructed from gsl::Owner" case and everything else 
> > > > requires additional annotation
> > > > I expect that we will experiment a bit in the future to see what works 
> > > > well for real-world code.
> > > I understand the difficulty, but I don't think it is appropriate to 
> > > experiment by ourselves -- these attributes are defined in a spec, and if 
> > > something is not clear, the spec should be clarified.
> > This is exactly what is going to happen but I think it would be unfortunate 
> > to stall the progress until the new version of the spec materializes. The 
> > idea is to keep the implementations and the specs in sync, but as Herb has 
> > other projects too, it takes some time to channel the experience back into 
> > the spec. As the current version of the warnings found true positives in 
> > real world projects and we have yet to see any false positives I would 
> > prefer to move forward to maximize utility.  
> > 
> > 
> I don't understand how different implementations can ever converge in that 
> case.
> 
> If this language extension is not sufficiently designed yet, maybe it is not 
> ready for inclusion in Clang?
> 
> 
The MSVC implementation does not support user defined annotations yet, so we 
are the first one to ask such questions like is it valid for an user to 
annotate a type as gsl::Pointer and have an overloaded deref operator with 
functionality other than accessing the pointee. We already forwarded these 
concerns to Herb, and he promised to clarify these things in the paper. Once it 
is clarified, MSVC will also follow it. Since this code will not reach the 
wider audience until Clang 10 is released and it is pretty easy to change this 
detail I do not see the justification to postpone the inclusion.

If we postpone the inclusion over and over we will never get enough experience 
from real world users to ever have enough confidence. 


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

https://reviews.llvm.org/D65127



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

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> This suggestion would result another strange behavior: if the user disables 
> cert-err09-cpp because he or she doesn't want to see its reports, the other 
> one (cert-err61-cpp) will still report the issue. So he or she has to disable 
> both (or as many aliases it has).

That seems to be the case regardless of the implementation strategy in this 
patch.

> This could be another advantage of this patch, since it would be annoying to 
> see a message in which a GCC-style checker warns about an LLVM-style 
> violation.

I don't understand the scenario. I think people wouldn't enable both checkers 
in the first place.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

[PATCH] D65663: [analyzer] ConditionBRVisitor: Fix HTML PathDiagnosticPopUpPieces

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Here is an example of the new `MemberExpr::getBase()` based report:
F9736772: report-Driver.cpp-operator()-6-1.html 





Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2420
+  if (!IsAssuming) {
+PathDiagnosticLocation Loc(BExpr->getLHS(), BRC.getSourceManager(), LCtx);
 return std::make_shared(Loc, Message);

NoQ wrote:
> Just curious, can `BExpr->getLHS()` potentially still be a multi-line 
> expression? Or are we making sure it's always a `DeclRefExpr`/`MemberExpr`?
> 
> In case of `MemberExpr` i'm pretty sure you can fit a newline before/after 
> `.` or `->`.
Previously I have focused on `DeclRefExpr` and `MemberExpr` value-evaluation, 
so that there the expression must be one of them. Because we have no 
`getField()` method for obtaining the field of the `MemberExpr`, I have picked 
`getBase()`.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2512
   const LocationContext *LCtx = N->getLocationContext();
-  PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx);
+  PathDiagnosticLocation Loc(ME, BRC.getSourceManager(), LCtx);
   if (!Loc.isValid() || !Loc.asLocation().isValid())

NoQ wrote:
> It looks like you forgot to make this change popup-piece-specific. I think we 
> should add our note to the whole condition in case of event pieces.
Hm, it probably makes sense. At this level 'Cond' and 'ME' are equals as I saw, 
but may in crazy conditions they are not.


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

https://reviews.llvm.org/D65663



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

[PATCH] D65663: [analyzer] ConditionBRVisitor: Fix HTML PathDiagnosticPopUpPieces

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 213610.
Charusso marked 4 inline comments as done.
Charusso added a comment.

- Fix.


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

https://reviews.llvm.org/D65663

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
  clang/test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist
  clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
  clang/test/Analysis/Inputs/expected-plists/inline-plist.c.plist
  clang/test/Analysis/Inputs/expected-plists/objc-radar17039661.m.plist
  clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist

Index: clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -2513,7 +2513,7 @@
 
 
  line96
- col13
+ col8
  file0
 

@@ -2735,7 +2735,7 @@
 
 
  line96
- col13
+ col8
  file0
 

@@ -3554,7 +3554,7 @@
 
 
  line127
- col14
+ col9
  file0
 

@@ -3776,7 +3776,7 @@
 
 
  line127
- col14
+ col9
  file0
 

Index: clang/test/Analysis/Inputs/expected-plists/objc-radar17039661.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/objc-radar17039661.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/objc-radar17039661.m.plist
@@ -836,7 +836,7 @@
 
 
  line38
- col37
+ col20
  file0
 

Index: clang/test/Analysis/Inputs/expected-plists/inline-plist.c.plist
===
--- clang/test/Analysis/Inputs/expected-plists/inline-plist.c.plist
+++ clang/test/Analysis/Inputs/expected-plists/inline-plist.c.plist
@@ -548,7 +548,7 @@
 
 
  line45
- col12
+ col7
  file0
 

Index: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
===
--- clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -2727,7 +2727,7 @@
 
 
  line146
- col13
+ col8
  file0
 

@@ -2949,7 +2949,7 @@
 
 
  line146
- col13
+ col8
  file0
 

@@ -3929,7 +3929,7 @@
 
 
  line178
- col14
+ col9
  file0
 

@@ -4185,7 +4185,7 @@
 
 
  line178
- col14
+ col9
  file0
 

@@ -4281,7 +4281,7 @@
 
 
  line181
- col14
+ col9
  file0
 

@@ -8087,7 +8087,7 @@
  location
  
   line267
-  col18
+  col19
   file0
  
  ranges
@@ -8095,7 +8095,7 @@

 
  line267
- col18
+ col19
  file0
 
 
@@ -8119,12 +8119,12 @@
  
   
line267
-   col18
+   col19
file0
   
   
line267
-   col18
+   col22
file0
   
  
@@ -11983,12 +11983,12 @@
  
   
line457
-   col9
+   col10
file0
   
   
line457
-   col9
+   col14
file0
   
  
@@ -12000,7 +12000,7 @@
  location
  
   line457
-  col9
+  col10
   file0
  
  ranges
@@ -12008,7 +12008,7 @@

 
  line457
- col9
+ col10
  file0
 
 
@@ -12032,12 +12032,12 @@
  
   
line457
-   col9
+   col10
file0
   
   
line457
-   col9
+   col14
file0
   
  
@@ -12244,12 +12244,12 @@
  
   
line457
-   col9
+   col10
file0
   
   
line457
-   col9
+   col14
file0
   
  
@@ -12261,7 +12261,7 @@
  location
  
   line457
-  col9
+  col10
   file0
  
  ranges
@@ -12269,7 +12269,7 @@

 
  line457
- col9
+ col10
  file0
 
 
@@ -12293,12 +12293,12 @@
  
   
line457
-   col9
+   col10
file0
 

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Tibor Brunner via Phabricator via cfe-commits]

bruntib added a comment.

This suggestion would result another strange behavior: if the user disables 
cert-err09-cpp because he or she doesn't want to see its reports, the other one 
(cert-err61-cpp) will still report the issue. So he or she has to disable both 
(or as many aliases it has).

As far as I know the idea behind aliases is that a checker can be registered 
with different options on different names. For example a code style checker can 
be registered with different names (e.g. GCC-style, LLVM-style, etc.) with 
different checker options on code styles. This could be another advantage of 
this patch, since it would be annoying to see a message in which a GCC-style 
checker warns about an LLVM-style violation.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

Re: AST for For loop

[Aaron Ballman via cfe-commits]

On Tue, Aug 6, 2019 at 11:15 AM Monalisa Rout via cfe-commits
 wrote:
>
> Hello,
> While dumping the AST for For loop, Why do I get  <<>> ??
>
> Source code:
> void func()
> {
> for (int i = 0; i < 5; i++)
> break;
> }
>
> AST
>
> FunctionDecl 0x4a4ac10 
>  line:2:6 
> func 'void ()'
> `-CompoundStmt 0x4a4ae1c 
>   `-ForStmt 0x4a4adf8 
> |-DeclStmt 0x4a4ad38 
> | `-VarDecl 0x4a4acd0  col:11 used i 'int' cinit
> |   `-IntegerLiteral 0x4a4ad10  'int' 0
> |-<<>>
> |-BinaryOperator 0x4a4ada8  'int' '<'
> | |-ImplicitCastExpr 0x4a4ad98  'int' 
> | | `-DeclRefExpr 0x4a4ad50  'int' lvalue Var 0x4a4acd0 'i' 'int'
> | `-IntegerLiteral 0x4a4ad70  'int' 5
> |-UnaryOperator 0x4a4ade0  'int' postfix '++'
> | `-DeclRefExpr 0x4a4adc0  'int' lvalue Var 0x4a4acd0 'i' 'int'
> `-BreakStmt 0x4a4adf0 

The AST dumps child nodes by calling children() on a given AST node.
The ForStmt node always has five child nodes, which are (in order):
init statement, condition variable, condition, increment, and body.
Some of those may be NULL depending on the format of the for loop
(your for loop example has no condition variable, for instance).

HTH!

~Aaron

>
> Regards,
> Mona
> ___
> 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

AST for For loop

[Monalisa Rout via cfe-commits]

Hello,
While dumping the AST for For loop, Why do I get  <<>> ??

Source code:
void func()
{
for (int i = 0; i < 5; i++)
break;
}

AST

FunctionDecl 0x4a4ac10

line:2:6 func 'void ()'
`-CompoundStmt 0x4a4ae1c 
  `-ForStmt 0x4a4adf8 
|-DeclStmt 0x4a4ad38 
| `-VarDecl 0x4a4acd0  col:11 used i 'int' cinit
|   `-IntegerLiteral 0x4a4ad10  'int' 0
|-<<>>
|-BinaryOperator 0x4a4ada8  'int' '<'
| |-ImplicitCastExpr 0x4a4ad98  'int' 
| | `-DeclRefExpr 0x4a4ad50  'int' lvalue Var 0x4a4acd0 'i'
'int'
| `-IntegerLiteral 0x4a4ad70  'int' 5
|-UnaryOperator 0x4a4ade0  'int' postfix '++'
| `-DeclRefExpr 0x4a4adc0  'int' lvalue Var 0x4a4acd0 'i' 'int'
`-BreakStmt 0x4a4adf0 

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

LLVM Types

[Monalisa Rout via cfe-commits]

Hello,
Can I dump LLVM Type hierarchies somehow ??

Types which are declared in this file ("clang/AST/Type.h
"  )

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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi marked 6 inline comments as done.
plotfi added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

gribozavr wrote:
> I don't see where this logic is implemented.
> 
> Also, LLVM is often built with assertions disabled, so "asserts" can't be a 
> part of the contract.
Please be more specific. What logic are you talking about? 



Comment at: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:153
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 

gribozavr wrote:
> No need to repeat the condition in the `&& "..."` part. assert does that by 
> default. Use an extra string to explain the assertion to the reader.
This is std assert. Are you referring to a different assert? If @compnerd is ok 
with changing the assert into an llvm::Expected I can do that. 



Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

gribozavr wrote:
> Why? This is a test -- ignoring errors would only hide them from developers.
Please see how llvm::Expected works. This does not ignore or hide anything. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704



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

[PATCH] D54565: Introduce `-Wctad` as a subgroup of `-Wc++14-compat`

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Oh, I see this review might be dead in the water; I'm not sure why I was just 
added as a reviewer to it (by a non-author, no less).

I personally have no problem with pulling out specific features as sub-groups 
of the compatibility warning.  I agree with Richard, though, that if the real 
goal is to warn about "unsafe" uses of this feature then a more tailored 
warning would be better.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54565



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

[PATCH] D54565: Introduce `-Wctad` as a subgroup of `-Wc++14-compat`

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I doubt "CTAD" is going to survive as a term that people recognize and 
remember, and I don't think `-Wclass-template-argument-deduction` is 
outrageously wordy compared to some of our other diagnostic groups.  With that 
change, this would be fine with me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54565



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

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Producing the message two times is worse user experience than producing one. 
Most users don't care which checker produced the message. However, the output 
should be deterministic. Therefore, a better fix would be making deduplication 
deterministic, instead of printing the message twice.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Tibor Brunner via Phabricator via cfe-commits]

bruntib added a comment.

Not exactly. The problem is that it is non-deterministic which checker reports 
the issue. For example before this patch, in the test file above there was only 
one report. However, sometimes the report message is:

throw expression should throw anonymous temporary values instead 
[cert-err09-cpp]

and sometimes the message is:

throw expression should throw anonymous temporary values instead 
[cert-err61-cpp]

(note the content of the square bracket)
So after this patch both of these lines will be emitted.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

[Puyan Lotfi via Phabricator via cfe-commits]

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



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

gribozavr wrote:
> plotfi wrote:
> > jkorous wrote:
> > > jkorous wrote:
> > > > IIUC this is silently dropping errors. We should print the error here.
> > > Ah, my bad - I just took a better look at `Expected<>` and you're right.
> > Nah, the way llvm::Expected works is that if the error isn't consumed then 
> > it will blow up in the destructor. So if it is an error, returning will 
> > cause the destructor to crash the program and print the error implicitly. 
> > Very nice error handling mechanism you ask me :-) 
> And crashing would be much better in a test. The test should test the 
> DirectoryWatcher, not just be graceful about error handling.
Test was in a deadlock, see commit message. This fixes that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704



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

[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Shaurya Gupta via Phabricator via cfe-commits]

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

use fileID instead of hash in map


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65754

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -460,15 +460,22 @@
 #define FOO(X, Y) int Y = ++X
 #define BAR(X) X + 1
 #define ECHO(X) X
+
+#define BUZZ BAZZ(ADD)
+#define BAZZ(m) m(1)
+#define ADD(a) int f = a + 1;
 template
 class P {};
-void f() {
+
+int main() {
   $a[[P a]];
   $b[[int b = 1]];
   $c[[FOO(b, c)]]; 
   $d[[FOO(BAR(BAR(b)), d)]];
   // FIXME: We might want to select everything inside the outer ECHO.
   ECHO(ECHO($e[[int) ECHO(e]]));
+  // Shouldn't crash.
+  $f[[BUZZ]];
 }
   )cpp");
 
@@ -495,6 +502,7 @@
   CheckRange("c");
   CheckRange("d");
   CheckRange("e");
+  CheckRange("f");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -296,14 +296,52 @@
  E1 < E2 ? R2.getEnd() : R1.getEnd());
 }
 
-// Returns the tokenFileRange for a given Location as a Token Range
+// Check if two locations have the same file id.
+static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
+   const SourceManager &SM) {
+  return SM.getFileID(Loc1) == SM.getFileID(Loc2);
+}
+
+// Find an expansion range (not necessarily immediate) the ends of which are in
+// the same file id.
+static SourceRange
+getExpansionTokenRangeInSameFile(SourceLocation Loc, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+  SourceRange ExpansionRange =
+  toTokenRange(SM.getImmediateExpansionRange(Loc), SM, LangOpts);
+  // Fast path for most common cases.
+  if (inSameFile(ExpansionRange.getBegin(), ExpansionRange.getEnd(), SM))
+return ExpansionRange;
+  // Record the stack of expansion locations for the beginning, keyed by FileID.
+  llvm::DenseMap BeginExpansions;
+  for (SourceLocation Begin = ExpansionRange.getBegin(); Begin.isValid();
+   Begin = Begin.isFileID()
+   ? SourceLocation()
+   : SM.getImmediateExpansionRange(Begin).getBegin()) {
+BeginExpansions[SM.getFileID(Begin)] = Begin;
+  }
+  // Move up the stack of expansion locations for the end until we find the
+  // location in BeginExpansions with that has the same file id.
+  for (SourceLocation End = ExpansionRange.getEnd(); End.isValid();
+   End = End.isFileID() ? SourceLocation()
+: toTokenRange(SM.getImmediateExpansionRange(End),
+   SM, LangOpts)
+  .getEnd()) {
+auto It = BeginExpansions.find(SM.getFileID(End));
+if (It != BeginExpansions.end())
+  return {It->second, End};
+  }
+  llvm_unreachable(
+  "We should able to find a common ancestor in the expansion tree.");
+}
+// Returns the file range for a given Location as a Token Range
 // This is quite similar to getFileLoc in SourceManager as both use
 // getImmediateExpansionRange and getImmediateSpellingLoc (for macro IDs).
 // However:
 // - We want to maintain the full range information as we move from one file to
 //   the next. getFileLoc only uses the BeginLoc of getImmediateExpansionRange.
-// - We want to split '>>' tokens as the lexer parses the '>>' in template
-//   instantiations as a '>>' instead of a '>'.
+// - We want to split '>>' tokens as the lexer parses the '>>' in nested
+//   template instantiations as a '>>' instead of two '>'s.
 // There is also getExpansionRange but it simply calls
 // getImmediateExpansionRange on the begin and ends separately which is wrong.
 static SourceRange getTokenFileRange(SourceLocation Loc,
@@ -311,16 +349,19 @@
  const LangOptions &LangOpts) {
   SourceRange FileRange = Loc;
   while (!FileRange.getBegin().isFileID()) {
-assert(!FileRange.getEnd().isFileID() &&
-   "Both Begin and End should be MacroIDs.");
 if (SM.isMacroArgExpansion(FileRange.getBegin())) {
-  FileRange.setBegin(SM.getImmediateSpellingLoc(FileRange.getBegin()));
-  FileRange.setEnd(SM.getImmediateSpellingLoc(FileRange.getEnd()));
+  FileRange = unionTokenRange(
+  SM.getImmediateSpellingLoc(FileRange.getBegin()),
+  SM.getImmediateSpellingLoc(FileRange.getEnd()), SM, LangOpts);
+  assert(inSameFile(FileRange.getBegin(), FileRange.getEnd(), SM));

[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:316
+  // Expand begin of Expansion range till the top and map with file id hash.
+  llvm::DenseMap BeginExpansions;
+  SourceRange BeginExpansion = ExpansionRange.getBegin();

sammccall wrote:
> DenseMap
> 
> Why are the values SourceRange here and not just SourceLocation?
meant to say here: I think FileID is a legitimate key in a DenseMap, no need 
for unsigned

(the DenseMapInfo specialization exists, which should make it 
work)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65754



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

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

So the problem you're trying to solve is that the output is non-deterministic, 
is that correct?

> However, it is random which checker name is included in the warning.

If that is indeed the problem, then I think the solution should be making the 
output deterministic -- not printing the message multiple times...


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D65065



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

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6581
+if (!Callee->getIdentifier()) {
+  auto OO = Callee->getOverloadedOperator();
+  return OO == OverloadedOperatorKind::OO_Subscript ||

xazax.hun wrote:
> gribozavr wrote:
> > mgehre wrote:
> > > xazax.hun wrote:
> > > > If we want to relax the warnings to give more results we could extend 
> > > > the checking of these overloaded operators for annotated types. But 
> > > > this would imply that the user need to have the expected semantics for 
> > > > those types and can only suppress false positives by removing some 
> > > > gsl:::owner/poinnter annotations.
> > > I see those options:
> > > - Either gsl::Owner implies some specific form of those operators (and if 
> > > that does not hold for a class, then one should not annotate it with 
> > > gsl::Owner)
> > > - or gsl::Owner only implies some specific behavior for the "gsl::Pointer 
> > > constructed from gsl::Owner" case and everything else requires additional 
> > > annotation
> > > I expect that we will experiment a bit in the future to see what works 
> > > well for real-world code.
> > I understand the difficulty, but I don't think it is appropriate to 
> > experiment by ourselves -- these attributes are defined in a spec, and if 
> > something is not clear, the spec should be clarified.
> This is exactly what is going to happen but I think it would be unfortunate 
> to stall the progress until the new version of the spec materializes. The 
> idea is to keep the implementations and the specs in sync, but as Herb has 
> other projects too, it takes some time to channel the experience back into 
> the spec. As the current version of the warnings found true positives in real 
> world projects and we have yet to see any false positives I would prefer to 
> move forward to maximize utility.  
> 
> 
I don't understand how different implementations can ever converge in that case.

If this language extension is not sufficiently designed yet, maybe it is not 
ready for inclusion in Clang?




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

https://reviews.llvm.org/D65127



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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:283
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

plotfi wrote:
> jkorous wrote:
> > jkorous wrote:
> > > IIUC this is silently dropping errors. We should print the error here.
> > Ah, my bad - I just took a better look at `Expected<>` and you're right.
> Nah, the way llvm::Expected works is that if the error isn't consumed then it 
> will blow up in the destructor. So if it is an error, returning will cause 
> the destructor to crash the program and print the error implicitly. Very nice 
> error handling mechanism you ask me :-) 
And crashing would be much better in a test. The test should test the 
DirectoryWatcher, not just be graceful about error handling.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704



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

[PATCH] D65704: DirectoryWatcher::create: Adding better error handling.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h:102
 
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start

I don't see where this logic is implemented.

Also, LLVM is often built with assertions disabled, so "asserts" can't be a 
part of the contract.



Comment at: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp:153
 dispatch_queue_t Queue) {
-  if (Path.empty())
-return nullptr;
+  assert(!Path.empty() && "Path.empty()");
 

No need to repeat the condition in the `&& "..."` part. assert does that by 
default. Use an extra string to explain the assertion to the reader.



Comment at: cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp:287
   /*waitForInitialSync=*/true);
+  if (!DW) return;
 

Why? This is a test -- ignoring errors would only hide them from developers.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65704



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

[PATCH] D65693: [driver][riscv] Support riscv64-linux-gnu multiarch paths

[Alex Bradbury via Phabricator via cfe-commits]

asb requested changes to this revision.
asb added a comment.
This revision now requires changes to proceed.

Many thanks for the patch. Could you please add some tests for this behaviour? 
I imagine you'll want to add a new directory in test/Driver/Inputs with a 
Debian tree skeleton. See D63497  for a 
relevant example.

Also, when submitting patches please try to generate the patch with -U9 
(see here 
) so we 
can see full context in the Phabricator Web UI.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65693



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

Re: [clang-tools-extra] r368019 - [clangd] Compute scopes eagerly in IncludeFixer

[Ilya Biryukov via cfe-commits]

+Hans Wennborg , could we merge this into the release?

On Tue, Aug 6, 2019 at 1:36 PM Ilya Biryukov via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: ibiryukov
> Date: Tue Aug  6 04:37:50 2019
> New Revision: 368019
>
> URL: http://llvm.org/viewvc/llvm-project?rev=368019&view=rev
> Log:
> [clangd] Compute scopes eagerly in IncludeFixer
>
> Summary:
> Computing lazily leads to crashes. In particular, computing scopes may
> produce diagnostics (from inside template instantiations) and we
> currently do it when processing another diagnostic, which leads to
> crashes.
>
> Moreover, we remember and access 'Scope*' when computing scopes. This
> might lead to invalid memory access if the Scope is deleted by the time
> we run the delayed computation. We did not actually construct an example
> when this happens, though.
>
> From the VCS and review history, it seems the optimization was
> introduced in the initial version without a mention of any performance
> benchmarks justifying the performance gains. This led me to a
> conclusion that the optimization was premature, so removing it to avoid
> crashes seems like the right trade-off at that point.
>
> Reviewers: sammccall
>
> Reviewed By: sammccall
>
> Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D65796
>
> Modified:
> clang-tools-extra/trunk/clangd/IncludeFixer.cpp
> clang-tools-extra/trunk/clangd/IncludeFixer.h
> clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
>
> Modified: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=368019&r1=368018&r2=368019&view=diff
>
> ==
> --- clang-tools-extra/trunk/clangd/IncludeFixer.cpp (original)
> +++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp Tue Aug  6 04:37:50
> 2019
> @@ -16,6 +16,7 @@
>  #include "index/Symbol.h"
>  #include "clang/AST/Decl.h"
>  #include "clang/AST/DeclBase.h"
> +#include "clang/AST/DeclarationName.h"
>  #include "clang/AST/NestedNameSpecifier.h"
>  #include "clang/AST/Type.h"
>  #include "clang/Basic/Diagnostic.h"
> @@ -34,6 +35,7 @@
>  #include "llvm/ADT/DenseMap.h"
>  #include "llvm/ADT/None.h"
>  #include "llvm/ADT/Optional.h"
> +#include "llvm/ADT/StringExtras.h"
>  #include "llvm/ADT/StringRef.h"
>  #include "llvm/ADT/StringSet.h"
>  #include "llvm/Support/Error.h"
> @@ -301,6 +303,24 @@ llvm::Optional extr
>return Result;
>  }
>
> +/// Returns all namespace scopes that the unqualified lookup would visit.
> +std::vector
> +collectAccessibleScopes(Sema &Sem, const DeclarationNameInfo &Typo, Scope
> *S,
> +Sema::LookupNameKind LookupKind) {
> +  std::vector Scopes;
> +  VisitedContextCollector Collector;
> +  Sem.LookupVisibleDecls(S, LookupKind, Collector,
> + /*IncludeGlobalScope=*/false,
> + /*LoadExternal=*/false);
> +
> +  Scopes.push_back("");
> +  for (const auto *Ctx : Collector.takeVisitedContexts()) {
> +if (isa(Ctx))
> +  Scopes.push_back(printNamespaceScope(*Ctx));
> +  }
> +  return Scopes;
> +}
> +
>  class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {
>  public:
>UnresolvedNameRecorder(llvm::Optional
> &LastUnresolvedName)
> @@ -321,48 +341,30 @@ public:
>  if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr))
>return clang::TypoCorrection();
>
> -// This is not done lazily because `SS` can get out of scope and it's
> -// relatively cheap.
>  auto Extracted = extractUnresolvedNameCheaply(
>  SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,
>  static_cast(LookupKind) ==
>  Sema::LookupNameKind::LookupNestedNameSpecifierName);
>  if (!Extracted)
>return TypoCorrection();
> -auto CheapUnresolved = std::move(*Extracted);
> +
>  UnresolvedName Unresolved;
> -Unresolved.Name = CheapUnresolved.Name;
> +Unresolved.Name = Extracted->Name;
>  Unresolved.Loc = Typo.getBeginLoc();
> -
> -if (!CheapUnresolved.ResolvedScope && !S) // Give up if no scope
> available.
> +if (!Extracted->ResolvedScope && !S) // Give up if no scope available.
>return TypoCorrection();
>
> -auto *Sem = SemaPtr; // Avoid capturing `this`.
> -Unresolved.GetScopes = [Sem, CheapUnresolved, S, LookupKind]() {
> -  std::vector Scopes;
> -  if (CheapUnresolved.ResolvedScope) {
> -Scopes.push_back(*CheapUnresolved.ResolvedScope);
> -  } else {
> -assert(S);
> -// No scope specifier is specified. Collect all accessible scopes
> in the
> -// context.
> -VisitedContextCollector Collector;
> -Sem->LookupVisibleDecls(
> -S, static_cast(LookupKind), Collector,
> -/*IncludeGlobalScope=*/false,
> -/*LoadExternal=*/false);
> -
> -

[PATCH] D65510: [clangd] Fix implicit template instatiations appearing as topLevelDecls.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

One important comment about somehow distinguishing multiple decls with the same 
name.




Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:110
+template
+void f(T) {}
+void s() {

jvikstrom wrote:
> ilya-biryukov wrote:
> > Could you also check that:
> > 
> > 1.  explicit specializations are present
> > ```
> > template <>
> > void f(bool) {}
> > ```
> > 2. explicit instantiations are absent
> > ```
> > template void f(bool);
> > ```
> > 3. partial specializations are present (they should not be affected by this 
> > change, but testing those here seems appropriate)
> > ```
> > template 
> > struct vector {};
> > 
> > template 
> > struct vector {}; // partial specialization, should be present
> > ```
> > 4. template variables and classes are also handled:
> > ```
> > template 
> > T foo = 10; // (!) requires C++17
> > ```
> Explicit instantiations are present in topLevelDecls though, otherwise 
> RecursiveASTVisitor would not traverse them (so adding a test to make sure 
> explicit instantiations are included in toplevel).
> 
> Also adding a test to SemanticHighlighting to make sure that explicit 
> instantiations are visited in that (is some other RecursiveASTVisitor usage I 
> should add this to instead?)
> 
Yes, sorry, I forgot about the results of your investigation yesterday.
Having them in top-level decls seems fine, just wanted to make sure we actually 
test it.
LG



Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:134
+template
+struct V {};
+

Also add a full specialization (IIRC, they are represented completely 
differently in the AST):
```
template <>
struct V {};
```



Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:137
+template
+T foo = T(10);
+int i = foo;

Also add a partial and a full template specializations for the variable 
declaration:
```
template 
int foo = 0;

template <>
int foo = 0;
```



Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:144
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),
+  ElementsAre(DeclNamed("f"), DeclNamed("f"), DeclNamed("f"),
+  DeclNamed("V"), DeclNamed("V"), DeclNamed("foo"),

Is there a way to check we are seeing the explicit instantiations? (e.g. by 
looking at template arguments?)

It's not clear whether multiple `DeclNamed("foo")` refer to the decls we expect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65510



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

[PATCH] D63976: Allow clang -Os and -Oz to work with -flto and lld

[Steven Noonan via Phabricator via cfe-commits]

tycho added a comment.

Two things:

- I'm curious why we would want to force `-O2`/`-O3` instead of just allowing 
`-Os`/`-Oz` to be used with LTO. Is optimizing for size combined with LTO 
really that unusual? I've built several projects using GCC with `-Os -flto` and 
the size reduction over plain `-Os` or `-O2 -flto` has been substantial enough 
to warrant that combination on release builds as well. I assume I might be 
missing something here, though, since someone mentioned this above (I don't 
understand the response to it though).
- This change is missing a fix for the option parsing in 
`tools/gold/gold-plugin.cpp` (`option::process_plugin_option`), which also 
complains about the `-Os`/`-Oz` arguments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63976



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

Re: r367889 - [docs] document -Weveything more betterer

[Nico Weber via cfe-commits]

Thanks!

On Mon, Aug 5, 2019 at 3:58 PM JF Bastien  wrote:

> Ugh this is silly… fixed again.
>
> On Aug 5, 2019, at 12:55 PM, Nico Weber  wrote:
>
> Still sad, now with a different message:
> http://lab.llvm.org:8011/builders/clang-sphinx-docs/builds/45220
>
> /home/buildbot/llvm-build-dir/clang-sphinx-docs/llvm/src/tools/clang/docs/UsersManual.rst:1002:
> WARNING: unknown option: -Wall
>
> On Mon, Aug 5, 2019 at 3:46 PM JF Bastien  wrote:
>
>> Fixed. I guess we should document those...
>>
>> On Aug 5, 2019, at 10:51 AM, Nico Weber  wrote:
>>
>> This breaks the sphinx bot:
>>
>>
>> http://lab.llvm.org:8011/builders/clang-sphinx-docs/builds/45204/steps/docs-clang-html/logs/stdio
>>
>> Warning, treated as error:
>> /home/buildbot/llvm-build-dir/clang-sphinx-docs/llvm/src/tools/clang/docs/UsersManual.rst:995:
>> WARNING: unknown option: -Wno-c++98-compat
>>
>> On Mon, Aug 5, 2019 at 12:52 PM JF Bastien via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: jfb
>>> Date: Mon Aug  5 09:53:45 2019
>>> New Revision: 367889
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=367889&view=rev
>>> Log:
>>> [docs] document -Weveything more betterer
>>>
>>> Reviewers: aaron.ballman
>>>
>>> Subscribers: jkorous, dexonsmith, cfe-commits
>>>
>>> Tags: #clang
>>>
>>> Differential Revision: https://reviews.llvm.org/D65706
>>>
>>> Modified:
>>> cfe/trunk/docs/UsersManual.rst
>>>
>>> Modified: cfe/trunk/docs/UsersManual.rst
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=367889&r1=367888&r2=367889&view=diff
>>>
>>> ==
>>> --- cfe/trunk/docs/UsersManual.rst (original)
>>> +++ cfe/trunk/docs/UsersManual.rst Mon Aug  5 09:53:45 2019
>>> @@ -992,13 +992,24 @@ is treated as a system header.
>>>  Enabling All Diagnostics
>>>  ^
>>>
>>> -In addition to the traditional ``-W`` flags, one can enable **all**
>>> -diagnostics by passing :option:`-Weverything`. This works as expected
>>> -with
>>> -:option:`-Werror`, and also includes the warnings from
>>> :option:`-pedantic`.
>>> +In addition to the traditional ``-W`` flags, one can enable **all**
>>> diagnostics
>>> +by passing :option:`-Weverything`. This works as expected with
>>> +:option:`-Werror`, and also includes the warnings from
>>> :option:`-pedantic`. Some
>>> +diagnostics contradict each other, therefore, users of
>>> :option:`-Weverything`
>>> +often disable many diagnostics such as :option:`-Wno-c++98-compat`
>>> +:option:`-Wno-c++-compat` because they contradict recent C++ standards.
>>>
>>> -Note that when combined with :option:`-w` (which disables all
>>> warnings), that
>>> -flag wins.
>>> +Since :option:`-Weverything` enables every diagnostic, we generally
>>> don't
>>> +recommend using it. :option:`-Wall` :option:`-Wextra` are a better
>>> choice for
>>> +most projects. Using :option:`-Weverything` means that updating your
>>> compiler is
>>> +more difficult because you're exposed to experimental diagnostics which
>>> might be
>>> +of lower quality than the default ones. If you do use
>>> :option:`-Weverything`
>>> +then we advise that you address all new compiler diagnostics as they
>>> get added
>>> +to Clang, either by fixing everything they find or explicitly disabling
>>> that
>>> +diagnostic with its corresponding `Wno-` option.
>>> +
>>> +Note that when combined with :option:`-w` (which disables all warnings),
>>> +disabling all warnings wins.
>>>
>>>  Controlling Static Analyzer Diagnostics
>>>  ^^^
>>>
>>>
>>> ___
>>> 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] D65510: [clangd] Fix implicit template instatiations appearing as topLevelDecls.

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

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



Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:110
+template
+void f(T) {}
+void s() {

ilya-biryukov wrote:
> Could you also check that:
> 
> 1.  explicit specializations are present
> ```
> template <>
> void f(bool) {}
> ```
> 2. explicit instantiations are absent
> ```
> template void f(bool);
> ```
> 3. partial specializations are present (they should not be affected by this 
> change, but testing those here seems appropriate)
> ```
> template 
> struct vector {};
> 
> template 
> struct vector {}; // partial specialization, should be present
> ```
> 4. template variables and classes are also handled:
> ```
> template 
> T foo = 10; // (!) requires C++17
> ```
Explicit instantiations are present in topLevelDecls though, otherwise 
RecursiveASTVisitor would not traverse them (so adding a test to make sure 
explicit instantiations are included in toplevel).

Also adding a test to SemanticHighlighting to make sure that explicit 
instantiations are visited in that (is some other RecursiveASTVisitor usage I 
should add this to instead?)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65510



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

[PATCH] D65510: [clangd] Fix implicit template instatiations appearing as topLevelDecls.

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

jvikstrom updated this revision to Diff 213608.
jvikstrom marked an inline comment as done.
jvikstrom added a comment.

Added tests for making sure explicit specializations, explicit instantiations, 
partial instantiations, explicit declarations and template variables are in 
topLevelDecls. Also added a test to SemanticHighlightingTests to make sure that 
explicit instantiations are being traversed (so they aren't accidentaly removed 
from topLevelDecls).

Also added comment for the two added functions in AST.h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65510

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/unittests/ClangdUnitTests.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
@@ -249,6 +249,12 @@
 
   template
   void $Function[[foo]]($TemplateParameter[[T]] ...);
+)cpp",
+R"cpp(
+  template 
+  struct $Class[[Tmpl]] {$TemplateParameter[[T]] $Field[[x]] = 0;};
+  extern template struct $Class[[Tmpl]];
+  template struct $Class[[Tmpl]];
 )cpp"};
   for (const auto &TestCase : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
@@ -103,6 +103,49 @@
   EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
 }
 
+TEST(ClangdUnitTest, DoesNotGetImplicitTemplateTopDecls) {
+  TestTU TU;
+  TU.Code = R"cpp(
+template
+void f(T) {}
+void s() {
+  f(10UL);
+}
+  )cpp";
+
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),
+  ElementsAre(DeclNamed("f"), DeclNamed("s")));
+}
+
+TEST(ClangdUnitTest,
+ GetsExplicitInstantiationAndSpecializationTemplateTopDecls) {
+  TestTU TU;
+  TU.Code = R"cpp(
+template 
+void f(T) {}
+template<>
+void f(bool);
+template void f(double);
+
+template 
+struct V {};
+template
+struct V {};
+
+template
+T foo = T(10);
+int i = foo;
+double d = foo;
+  )cpp";
+
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),
+  ElementsAre(DeclNamed("f"), DeclNamed("f"), DeclNamed("f"),
+  DeclNamed("V"), DeclNamed("V"), DeclNamed("foo"),
+  DeclNamed("i"), DeclNamed("d")));
+}
+
 TEST(ClangdUnitTest, TokensAfterPreamble) {
   TestTU TU;
   TU.AdditionalFiles["foo.h"] = R"(
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1674,13 +1674,6 @@
   }
 };
 
-template  bool isExplicitTemplateSpecialization(const NamedDecl &ND) {
-  if (const auto *TD = dyn_cast(&ND))
-if (TD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
-  return true;
-  return false;
-}
-
 } // namespace
 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
@@ -1783,9 +1776,7 @@
   };
   // We only complete symbol's name, which is the same as the name of the
   // *primary* template in case of template specializations.
-  if (isExplicitTemplateSpecialization(ND) ||
-  isExplicitTemplateSpecialization(ND) ||
-  isExplicitTemplateSpecialization(ND))
+  if (isExplicitTemplateSpecialization(&ND))
 return false;
 
   if (InTopLevelScope(ND))
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -9,6 +9,7 @@
 #include "ClangdUnit.h"
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
+#include "AST.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "Headers.h"
@@ -19,8 +20,11 @@
 #include "index/CanonicalIncludes.h"
 #include "index/Index.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -70,6 +74,9 @@
   auto &SM = D->getASTContext().getSourceManager();
   if (!isInsideMainFile(D->getLocation(), SM))
  

[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 213599.
SureYeaah marked 8 inline comments as done.
SureYeaah added a comment.

Addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65754

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -460,15 +460,22 @@
 #define FOO(X, Y) int Y = ++X
 #define BAR(X) X + 1
 #define ECHO(X) X
+
+#define BUZZ BAZZ(ADD)
+#define BAZZ(m) m(1)
+#define ADD(a) int f = a + 1;
 template
 class P {};
-void f() {
+
+int main() {
   $a[[P a]];
   $b[[int b = 1]];
   $c[[FOO(b, c)]]; 
   $d[[FOO(BAR(BAR(b)), d)]];
   // FIXME: We might want to select everything inside the outer ECHO.
   ECHO(ECHO($e[[int) ECHO(e]]));
+  // Shouldn't crash.
+  $f[[BUZZ]];
 }
   )cpp");
 
@@ -495,6 +502,7 @@
   CheckRange("c");
   CheckRange("d");
   CheckRange("e");
+  CheckRange("f");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -296,14 +296,52 @@
  E1 < E2 ? R2.getEnd() : R1.getEnd());
 }
 
-// Returns the tokenFileRange for a given Location as a Token Range
+// Check if two locations have the same file id.
+static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
+   const SourceManager &SM) {
+  return SM.getFileID(Loc1) == SM.getFileID(Loc2);
+}
+
+// Find an expansion range (not necessarily immediate) the ends of which are in
+// the same file id.
+static SourceRange
+getExpansionTokenRangeInSameFile(SourceLocation Loc, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+  SourceRange ExpansionRange =
+  toTokenRange(SM.getImmediateExpansionRange(Loc), SM, LangOpts);
+  // Fast path for most common cases.
+  if (inSameFile(ExpansionRange.getBegin(), ExpansionRange.getEnd(), SM))
+return ExpansionRange;
+  // Record the stack of expansion locations for the beginning, keyed by FileID.
+  llvm::DenseMap BeginExpansions;
+  for (SourceLocation Begin = ExpansionRange.getBegin(); Begin.isValid();
+   Begin = Begin.isFileID()
+   ? SourceLocation()
+   : SM.getImmediateExpansionRange(Begin).getBegin()) {
+BeginExpansions[SM.getFileID(Begin).getHashValue()] = Begin;
+  }
+  // Move up the stack of expansion locations for the end until we find the
+  // location in BeginExpansions with that has the same file id.
+  for (SourceLocation End = ExpansionRange.getEnd(); End.isValid();
+   End = End.isFileID() ? SourceLocation()
+: toTokenRange(SM.getImmediateExpansionRange(End),
+   SM, LangOpts)
+  .getEnd()) {
+auto It = BeginExpansions.find(SM.getFileID(End).getHashValue());
+if (It != BeginExpansions.end())
+  return {It->second, End};
+  }
+  llvm_unreachable(
+  "We should able to find a common ancestor in the expansion tree.");
+}
+// Returns the file range for a given Location as a Token Range
 // This is quite similar to getFileLoc in SourceManager as both use
 // getImmediateExpansionRange and getImmediateSpellingLoc (for macro IDs).
 // However:
 // - We want to maintain the full range information as we move from one file to
 //   the next. getFileLoc only uses the BeginLoc of getImmediateExpansionRange.
-// - We want to split '>>' tokens as the lexer parses the '>>' in template
-//   instantiations as a '>>' instead of a '>'.
+// - We want to split '>>' tokens as the lexer parses the '>>' in nested
+//   template instantiations as a '>>' instead of two '>'s.
 // There is also getExpansionRange but it simply calls
 // getImmediateExpansionRange on the begin and ends separately which is wrong.
 static SourceRange getTokenFileRange(SourceLocation Loc,
@@ -311,16 +349,19 @@
  const LangOptions &LangOpts) {
   SourceRange FileRange = Loc;
   while (!FileRange.getBegin().isFileID()) {
-assert(!FileRange.getEnd().isFileID() &&
-   "Both Begin and End should be MacroIDs.");
 if (SM.isMacroArgExpansion(FileRange.getBegin())) {
-  FileRange.setBegin(SM.getImmediateSpellingLoc(FileRange.getBegin()));
-  FileRange.setEnd(SM.getImmediateSpellingLoc(FileRange.getEnd()));
+  FileRange = unionTokenRange(
+  SM.getImmediateSpellingLoc(FileRange.getBegin()),
+  SM.getImmediateSpellingLoc(FileRange.getEnd()), SM, LangOpts);
+  

[PATCH] D65804: Fixed toHalfOpenFileRange assertion fail

[Shaurya Gupta via Phabricator via cfe-commits]

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

- Added new function that gets Expansion range with both ends in the

same file.

- Fixes the crash at https://github.com/clangd/clangd/issues/113


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65804

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -460,15 +460,22 @@
 #define FOO(X, Y) int Y = ++X
 #define BAR(X) X + 1
 #define ECHO(X) X
+
+#define BUZZ BAZZ(ADD)
+#define BAZZ(m) m(1)
+#define ADD(a) int f = a + 1;
 template
 class P {};
-void f() {
+
+int main() {
   $a[[P a]];
   $b[[int b = 1]];
   $c[[FOO(b, c)]]; 
   $d[[FOO(BAR(BAR(b)), d)]];
   // FIXME: We might want to select everything inside the outer ECHO.
   ECHO(ECHO($e[[int) ECHO(e]]));
+  // Shouldn't crash.
+  $f[[BUZZ]];
 }
   )cpp");
 
@@ -495,6 +502,7 @@
   CheckRange("c");
   CheckRange("d");
   CheckRange("e");
+  CheckRange("f");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -296,14 +296,52 @@
  E1 < E2 ? R2.getEnd() : R1.getEnd());
 }
 
-// Returns the tokenFileRange for a given Location as a Token Range
+// Check if two locations have the same file id.
+static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
+   const SourceManager &SM) {
+  return SM.getFileID(Loc1) == SM.getFileID(Loc2);
+}
+
+// Find an expansion range (not necessarily immediate) the ends of which are in
+// the same file id.
+static SourceRange
+getExpansionTokenRangeInSameFile(SourceLocation Loc, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+  SourceRange ExpansionRange =
+  toTokenRange(SM.getImmediateExpansionRange(Loc), SM, LangOpts);
+  // Fast path for most common cases.
+  if (inSameFile(ExpansionRange.getBegin(), ExpansionRange.getEnd(), SM))
+return ExpansionRange;
+  // Record the stack of expansion locations for the beginning, keyed by FileID.
+  llvm::DenseMap BeginExpansions;
+  for (SourceLocation Begin = ExpansionRange.getBegin(); Begin.isValid();
+   Begin = Begin.isFileID()
+   ? SourceLocation()
+   : SM.getImmediateExpansionRange(Begin).getBegin()) {
+BeginExpansions[SM.getFileID(Begin).getHashValue()] = Begin;
+  }
+  // Move up the stack of expansion locations for the end until we find the
+  // location in BeginExpansions with that has the same file id.
+  for (SourceLocation End = ExpansionRange.getEnd(); End.isValid();
+   End = End.isFileID() ? SourceLocation()
+: toTokenRange(SM.getImmediateExpansionRange(End),
+   SM, LangOpts)
+  .getEnd()) {
+auto It = BeginExpansions.find(SM.getFileID(End).getHashValue());
+if (It != BeginExpansions.end())
+  return {It->second, End};
+  }
+  llvm_unreachable(
+  "We should able to find a common ancestor in the expansion tree.");
+}
+// Returns the file range for a given Location as a Token Range
 // This is quite similar to getFileLoc in SourceManager as both use
 // getImmediateExpansionRange and getImmediateSpellingLoc (for macro IDs).
 // However:
 // - We want to maintain the full range information as we move from one file to
 //   the next. getFileLoc only uses the BeginLoc of getImmediateExpansionRange.
-// - We want to split '>>' tokens as the lexer parses the '>>' in template
-//   instantiations as a '>>' instead of a '>'.
+// - We want to split '>>' tokens as the lexer parses the '>>' in nested
+//   template instantiations as a '>>' instead of two '>'s.
 // There is also getExpansionRange but it simply calls
 // getImmediateExpansionRange on the begin and ends separately which is wrong.
 static SourceRange getTokenFileRange(SourceLocation Loc,
@@ -311,16 +349,19 @@
  const LangOptions &LangOpts) {
   SourceRange FileRange = Loc;
   while (!FileRange.getBegin().isFileID()) {
-assert(!FileRange.getEnd().isFileID() &&
-   "Both Begin and End should be MacroIDs.");
 if (SM.isMacroArgExpansion(FileRange.getBegin())) {
-  FileRange.setBegin(SM.getImmediateSpellingLoc(FileRange.getBegin()));
-  FileRange.setEnd(SM.getImmediateSpellingLoc(FileRange.getEnd()));
+  FileRange = unionTokenRange(
+  SM.getImmediateSpellingLoc(FileRange.getBegin()

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6581
+if (!Callee->getIdentifier()) {
+  auto OO = Callee->getOverloadedOperator();
+  return OO == OverloadedOperatorKind::OO_Subscript ||

gribozavr wrote:
> mgehre wrote:
> > xazax.hun wrote:
> > > If we want to relax the warnings to give more results we could extend the 
> > > checking of these overloaded operators for annotated types. But this 
> > > would imply that the user need to have the expected semantics for those 
> > > types and can only suppress false positives by removing some 
> > > gsl:::owner/poinnter annotations.
> > I see those options:
> > - Either gsl::Owner implies some specific form of those operators (and if 
> > that does not hold for a class, then one should not annotate it with 
> > gsl::Owner)
> > - or gsl::Owner only implies some specific behavior for the "gsl::Pointer 
> > constructed from gsl::Owner" case and everything else requires additional 
> > annotation
> > I expect that we will experiment a bit in the future to see what works well 
> > for real-world code.
> I understand the difficulty, but I don't think it is appropriate to 
> experiment by ourselves -- these attributes are defined in a spec, and if 
> something is not clear, the spec should be clarified.
This is exactly what is going to happen but I think it would be unfortunate to 
stall the progress until the new version of the spec materializes. The idea is 
to keep the implementations and the specs in sync, but as Herb has other 
projects too, it takes some time to channel the experience back into the spec. 
As the current version of the warnings found true positives in real world 
projects and we have yet to see any false positives I would prefer to move 
forward to maximize utility.  




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

https://reviews.llvm.org/D65127



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

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, looks simpler now, just a few more nits.




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:7
+interface TokenColorRule {
+  scope: string, textColor: string,
+}

nit: we need documentation for the fields.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:11
+// Gets a TextMate theme and all its included themes by its name.
+async function loadTheme(themeName: string): Promise {
+  const extension =

do we need `async` here? since we don't use any `await` in the function body.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:25
+
+  const extensionInfo = extension.packageJSON.contributes.themes.find(
+  (theme: any) => theme.id === themeName || theme.label === themeName);

nit: name it `themeData` or `themeInfo`?



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:31
+/**
+ * Recursively parse the TM grammar at fullPath. If there are multiple TM 
scopes
+ * of the same name in the include chain only the earliest entry of the scope 
is

nit: textmate grammar is another different thing, the file is the textmate 
**theme**.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:35
+ * @param fullPath The absolute path to the theme.
+ * @param scopeSet A set containing the name of the scopes that have already
+ * been set.

nit: maybe `SeenScopes`?



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:42
+scopeSet = new Set();
+  // FIXME: Add support for themes written as .thTheme.
+  if (path.extname(fullPath) === '.tmTheme')

nit: th => tm



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:45
+return [];
+  // If there is an error opening a file, the TM files that were correctly 
found
+  // and parsed further up the chain should be returned. Otherwise there will 
be

I think it would be more suitable to move this comment in the `catch` block 
below, we don't throw this error.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:94
+  }
+  resolve(data);
+});

nit: return resolve(data).



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc:3
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [

nit: any reason use `jsonc` not `json`?




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc:2
+{
+"include": "tmThemeInclude.tmTheme",
+"tokenColors": [

since we don't support `tmTheme` now, I'd prefer just dropping it from the test.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc:3
+// Some comment
+"include": "firstIncludedTheme.jsonc",
+"name": "TestTheme",

I think using one-level test is sufficient, how about the tests below?

- simpleTheme.json (a simple non-include theme)
- includeTheme.json (one-level include them)



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:1
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';

this comment doesn't seem to provide much information, I'd remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738



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

[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:307
+// the same file id.
+static SourceRange getExpansionRangeInSameFiles(SourceLocation Loc,
+const SourceManager &SM,

nit: `getExpansionTokenRangeInSameFile`?



Comment at: clang-tools-extra/clangd/SourceCode.cpp:315
+return ExpansionRange;
+  // Expand begin of Expansion range till the top and map with file id hash.
+  llvm::DenseMap BeginExpansions;

Having a little trouble parsing this comment...

"Record the stack of expansion locations for the beginning, keyed by FileID"?



Comment at: clang-tools-extra/clangd/SourceCode.cpp:316
+  // Expand begin of Expansion range till the top and map with file id hash.
+  llvm::DenseMap BeginExpansions;
+  SourceRange BeginExpansion = ExpansionRange.getBegin();

DenseMap

Why are the values SourceRange here and not just SourceLocation?



Comment at: clang-tools-extra/clangd/SourceCode.cpp:318
+  SourceRange BeginExpansion = ExpansionRange.getBegin();
+  while (1) {
+BeginExpansions[SM.getFileID(BeginExpansion.getBegin()).getHashValue()] =

Is there a way to write this loop that better captures the intent? while(1) 
with a break in the middle is hard for readers to understand at a glance.

I think it's just

```
for (SourceLocation Begin = ExpansionRange.getBegin();
   Begin.isValid();
   Begin = Begin.isFileId() ? SourceLocation() : 
SM.getImmediateExpansionRange(Begin).getBegin())
  BeginExpansions[SM.getFileID(Begin)] = Begin;
```



Comment at: clang-tools-extra/clangd/SourceCode.cpp:323
+  break;
+BeginExpansion = toTokenRange(
+SM.getImmediateExpansionRange(BeginExpansion.getBegin()), SM, 
LangOpts);

this call to toTokenRange seems unneccesary, that just adjusts End, but we only 
care about Start



Comment at: clang-tools-extra/clangd/SourceCode.cpp:328
+  // BeginExpansions.
+  SourceRange EndExpansion = ExpansionRange.getEnd();
+  while (1) {

again, I think by treating these as ranges rather than points you're making it 
harder than it needs to be. Traversing up the expansion tree isn't going to 
invert the order (though spelling might)



Comment at: clang-tools-extra/clangd/SourceCode.cpp:333
+if (It != BeginExpansions.end())
+  return unionTokenRange(It->second, EndExpansion, SM, LangOpts);
+if (EndExpansion.getEnd().isFileID())

isn't this always SourceRange(It->second.getBegin(), EndExpansion.getEnd())?



Comment at: clang-tools-extra/clangd/SourceCode.cpp:362
+  assert(inSameFile(FileRange.getBegin(), FileRange.getEnd(), SM) &&
+ "Both ends should be in same file.");
 } else {

nit: as with comments, avoid adding messages to the assertion unless it adds 
something to the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65754



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

r368029 - Improve MSVC visualizations so the parser shows where we are in the code

[Mike Spertus via cfe-commits]

Author: mps
Date: Tue Aug  6 06:29:35 2019
New Revision: 368029

URL: http://llvm.org/viewvc/llvm-project?rev=368029&view=rev
Log:
Improve MSVC visualizations so the parser shows where we are in the code

Also provide a visualizer for lambda introducers

Modified:
cfe/trunk/utils/ClangVisualizers/clang.natvis

Modified: cfe/trunk/utils/ClangVisualizers/clang.natvis
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/ClangVisualizers/clang.natvis?rev=368029&r1=368028&r2=368029&view=diff
==
--- cfe/trunk/utils/ClangVisualizers/clang.natvis (original)
+++ cfe/trunk/utils/ClangVisualizers/clang.natvis Tue Aug  6 06:29:35 2019
@@ -763,6 +763,51 @@ For later versions of Visual Studio, no
 {{Identifier 
({(clang::IdentifierInfo *)(PtrData),na})}}
 {(clang::tok::TokenKind)Kind,en}
   
+  
+{BufferPtr,nasb}
+  
+  
+{TheLexer._Mypair._Myval2,na}
+Expanding 
Macro: {TheTokenLexer._Mypair._Myval2,na}
+
+  
+  
+
+  [{(Token *)(CachedTokens.BeginX) + CachedLexPos,na}] 
{IncludeMacroStack._Mypair._Myval2._Mylast - 1,na}
+
+ 
{IncludeMacroStack._Mypair._Myval2._Mylast - 1,na}
+{CurLexer._Mypair._Myval2,na}
+Expanding 
Macro: {CurTokenLexer._Mypair._Myval2,na}
+
+
+  {this,view(cached)}
+
+CLK_LexAfterModuleImport
+  
+  
+[{Tok}] {PP,na}
+  
+  
+this
+*this
+{Id}
+&{Id}
+No visualizer for {Kind}
+  
+  
+
+=,
+&,
+
+{(LambdaCapture 
*)(Captures.BeginX),na}{this,view(capture1)na}
+
+,{(LambdaCapture 
*)(Captures.BeginX)+1,na}{this,view(capture2)na}
+
+,{(LambdaCapture 
*)(Captures.BeginX)+2,na}{this,view(capture3)na}
+
+,...
+
[{this,view(default)na}{this,view(capture0)na}]
+  
   
 
   , [{TypeRep}]
@@ -817,6 +862,7 @@ For later versions of Visual Studio, no
 {(DeclaratorDecl*)this,nand}
 
   (DeclaratorDecl*)this,nd
+  Init
   VarDeclBits
 
   


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

[PATCH] D64736: [clang-tidy] New bugprone-infinite-loop check for detecting obvious infinite loops

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: docs/clang-tidy/checks/bugprone-infinite-loop.rst:12
+the loop condition is not changed. This check detects such loops. A loop is
+considered as infinite if it does not have any loop exit statement (``break``,
+``continue``, ``goto``, ``return``, ``throw`` or a call to a function called as

s/as//



Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:156
+  }
+  int &ii = i;
+}

baloghadamsoftware wrote:
> gribozavr wrote:
> > Is all escaping that syntactically follows the loop actually irrelevant? 
> > For example:
> > 
> > ```
> > int i = 0;
> > int j = 0;
> > int *p = &j;
> > for (int k = 0; k < 5; k++) {
> >   *p = 100;
> >   if (k != 0) {
> > // This loop would be reported as infinite.
> > while (i < 10) {
> > }
> >   }
> >   p = &i;
> > }
> > ```
> You are right, but how frequent are such cases? I found no false positives at 
> all when checking some ope-source projects. Should we spend resources to 
> detect escaping in a nesting loop? I could not find a cheap way yet. I 
> suppose this can only be done when nesting two loops. (I am sure we can 
> ignore the cases where the outer loop is implemented using a `goto`.
I don't know, however, this is one of the few sources of false positives for 
this check. This check generally can't detect all infinite loops, it is only 
trying to detect obvious ones. Therefore, I don't think it is exchange a bit 
more precision for false positives.

My best advice is to drop the heuristic about variable escaping after the loop. 
Any escaping => silence the warning.

If you want to handle this case correctly, you have to use the CFG.



Comment at: test/clang-tidy/bugprone-infinite-loop.cpp:225
+  while (1)
+; //FIXME: We expect report in this case.
+}

baloghadamsoftware wrote:
> gribozavr wrote:
> > Why?
> > 
> > Intentional infinite loops *that perform side-effects in their body* are 
> > quite common -- for example, network servers, command-line interpreters etc.
> > 
> > Also, if the loop body calls an arbitrary function, we don't know if it 
> > will throw, or `exit()`, or kill the current thread or whatnot.
> We already handle loop exit statements including calls to `[[noreturn]]` 
> functions. Is that not enough?
User-defined functions are not always annotated with ``[[noreturn]]``, and 
sometimes can't be annotated, for example, because they call `exit()` 
conditionally:

```
while (true) {
  std::string command = readCommand();
  executeCommand(command);
}

void executeCommand(const std::string &command) {
  if (command == "exit") {
exit();
  }
  ...
}
```


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

https://reviews.llvm.org/D64736



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

[PATCH] D65445: [CrossTU] Handle case when no USR could be generated during Decl search.

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368020: [CrossTU] Handle case when no USR could be generated 
during Decl search. (authored by balazske, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65445?vs=213571&id=213584#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65445

Files:
  cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
  cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
  cfe/trunk/test/Analysis/Inputs/ctu-other.cpp
  cfe/trunk/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  cfe/trunk/test/Analysis/ctu-main.cpp
  cfe/trunk/test/Analysis/func-mapping-test.cpp
  cfe/trunk/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

Index: cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
===
--- cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
+++ cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
@@ -193,12 +193,13 @@
 
 CrossTranslationUnitContext::~CrossTranslationUnitContext() {}
 
-std::string CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
+llvm::Optional
+CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
   SmallString<128> DeclUSR;
   bool Ret = index::generateUSRForDecl(ND, DeclUSR);
-  (void)Ret;
-  assert(!Ret && "Unable to generate USR");
-  return DeclUSR.str();
+  if (Ret)
+return {};
+  return std::string(DeclUSR.str());
 }
 
 /// Recursively visits the decls of a DeclContext, and returns one with the
@@ -218,7 +219,8 @@
 const T *ResultDecl;
 if (!ND || !hasBodyOrInit(ND, ResultDecl))
   continue;
-if (getLookupName(ResultDecl) != LookupName)
+llvm::Optional ResultLookupName = getLookupName(ResultDecl);
+if (!ResultLookupName || *ResultLookupName != LookupName)
   continue;
 return ResultDecl;
   }
@@ -233,12 +235,12 @@
   assert(!hasBodyOrInit(D) &&
  "D has a body or init in current translation unit!");
   ++NumGetCTUCalled;
-  const std::string LookupName = getLookupName(D);
-  if (LookupName.empty())
+  const llvm::Optional LookupName = getLookupName(D);
+  if (!LookupName)
 return llvm::make_error(
 index_error_code::failed_to_generate_usr);
   llvm::Expected ASTUnitOrError =
-  loadExternalAST(LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
+  loadExternalAST(*LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
   if (!ASTUnitOrError)
 return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
@@ -294,7 +296,7 @@
   }
 
   TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
-  if (const T *ResultDecl = findDefInDeclContext(TU, LookupName))
+  if (const T *ResultDecl = findDefInDeclContext(TU, *LookupName))
 return importDefinition(ResultDecl, Unit);
   return llvm::make_error(index_error_code::failed_import);
 }
Index: cfe/trunk/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
===
--- cfe/trunk/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
+++ cfe/trunk/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
@@ -76,7 +76,12 @@
 
 void MapExtDefNamesConsumer::addIfInMain(const DeclaratorDecl *DD,
  SourceLocation defStart) {
-  std::string LookupName = CrossTranslationUnitContext::getLookupName(DD);
+  llvm::Optional LookupName =
+  CrossTranslationUnitContext::getLookupName(DD);
+  if (!LookupName)
+return;
+  assert(!LookupName->empty() && "Lookup name should be non-empty.");
+
   if (CurrentFileName.empty()) {
 CurrentFileName =
 SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
@@ -89,7 +94,7 @@
   case VisibleNoLinkage:
   case UniqueExternalLinkage:
 if (SM.isInMainFile(defStart))
-  Index[LookupName] = CurrentFileName;
+  Index[*LookupName] = CurrentFileName;
 break;
   default:
 break;
Index: cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
===
--- cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
+++ cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
@@ -17,6 +17,7 @@
 #include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Error.h"
@@ -159,7 +160,7 @@
ASTUnit *Unit);
 
   /// Get a name to identify a named decl.
-  static std::string getLookupName(const NamedDecl *ND);
+  static llvm::Optional getLookupName(const NamedDecl *ND);
 
   /// Emit diagnostics for the user for potential configuration errors.
   void emitCrossTUDiagnostics(const IndexError &IE);
Index: cfe/trunk/test/Analysis/Inputs/ctu-

r368020 - [CrossTU] Handle case when no USR could be generated during Decl search.

[Balazs Keri via cfe-commits]

Author: balazske
Date: Tue Aug  6 05:10:16 2019
New Revision: 368020

URL: http://llvm.org/viewvc/llvm-project?rev=368020&view=rev
Log:
[CrossTU] Handle case when no USR could be generated during Decl search.

Summary:
When searching for a declaration to be loaded the "lookup name" for every
other Decl is computed. If the USR can not be determined here should be
not an assert, instead skip this Decl.

Reviewers: martong

Reviewed By: martong

Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, cfe-commits

Tags: #clang

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

Modified:
cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
cfe/trunk/test/Analysis/Inputs/ctu-other.cpp
cfe/trunk/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
cfe/trunk/test/Analysis/ctu-main.cpp
cfe/trunk/test/Analysis/func-mapping-test.cpp
cfe/trunk/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

Modified: cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h?rev=368020&r1=368019&r2=368020&view=diff
==
--- cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h (original)
+++ cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h Tue Aug  6 05:10:16 
2019
@@ -17,6 +17,7 @@
 #include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Error.h"
@@ -159,7 +160,7 @@ public:
ASTUnit *Unit);
 
   /// Get a name to identify a named decl.
-  static std::string getLookupName(const NamedDecl *ND);
+  static llvm::Optional getLookupName(const NamedDecl *ND);
 
   /// Emit diagnostics for the user for potential configuration errors.
   void emitCrossTUDiagnostics(const IndexError &IE);

Modified: cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp?rev=368020&r1=368019&r2=368020&view=diff
==
--- cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp (original)
+++ cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp Tue Aug  6 05:10:16 2019
@@ -193,12 +193,13 @@ CrossTranslationUnitContext::CrossTransl
 
 CrossTranslationUnitContext::~CrossTranslationUnitContext() {}
 
-std::string CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
+llvm::Optional
+CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
   SmallString<128> DeclUSR;
   bool Ret = index::generateUSRForDecl(ND, DeclUSR);
-  (void)Ret;
-  assert(!Ret && "Unable to generate USR");
-  return DeclUSR.str();
+  if (Ret)
+return {};
+  return std::string(DeclUSR.str());
 }
 
 /// Recursively visits the decls of a DeclContext, and returns one with the
@@ -218,7 +219,8 @@ CrossTranslationUnitContext::findDefInDe
 const T *ResultDecl;
 if (!ND || !hasBodyOrInit(ND, ResultDecl))
   continue;
-if (getLookupName(ResultDecl) != LookupName)
+llvm::Optional ResultLookupName = getLookupName(ResultDecl);
+if (!ResultLookupName || *ResultLookupName != LookupName)
   continue;
 return ResultDecl;
   }
@@ -233,12 +235,12 @@ llvm::Expected CrossTranslati
   assert(!hasBodyOrInit(D) &&
  "D has a body or init in current translation unit!");
   ++NumGetCTUCalled;
-  const std::string LookupName = getLookupName(D);
-  if (LookupName.empty())
+  const llvm::Optional LookupName = getLookupName(D);
+  if (!LookupName)
 return llvm::make_error(
 index_error_code::failed_to_generate_usr);
   llvm::Expected ASTUnitOrError =
-  loadExternalAST(LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
+  loadExternalAST(*LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
   if (!ASTUnitOrError)
 return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
@@ -294,7 +296,7 @@ llvm::Expected CrossTranslati
   }
 
   TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
-  if (const T *ResultDecl = findDefInDeclContext(TU, LookupName))
+  if (const T *ResultDecl = findDefInDeclContext(TU, *LookupName))
 return importDefinition(ResultDecl, Unit);
   return llvm::make_error(index_error_code::failed_import);
 }

Modified: cfe/trunk/test/Analysis/Inputs/ctu-other.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/ctu-other.cpp?rev=368020&r1=368019&r2=368020&view=diff
==
--- cfe/trunk/test/Analysis/Inputs/ctu-other.cpp (original)
+++ cfe/trunk/test/Analysis/Inputs/ctu-other.cpp Tue Aug  6 05:10:16 2019
@@ -131,3 +131,17 @@ union U {
   const unsigned int b;
 };
 U extU =

[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Shaurya Gupta via Phabricator via cfe-commits]

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

Implemented function to get expansion range such that ends are both in the same 
file id.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65754

Files:
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

Index: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
===
--- clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
+++ clang-tools-extra/clangd/unittests/SourceCodeTests.cpp
@@ -460,15 +460,22 @@
 #define FOO(X, Y) int Y = ++X
 #define BAR(X) X + 1
 #define ECHO(X) X
+
+#define BUZZ BAZZ(ADD)
+#define BAZZ(m) m(1)
+#define ADD(a) int f = a + 1;
 template
 class P {};
-void f() {
+
+int main() {
   $a[[P a]];
   $b[[int b = 1]];
   $c[[FOO(b, c)]]; 
   $d[[FOO(BAR(BAR(b)), d)]];
   // FIXME: We might want to select everything inside the outer ECHO.
   ECHO(ECHO($e[[int) ECHO(e]]));
+  // Shouldn't crash.
+  $f[[BUZZ]];
 }
   )cpp");
 
@@ -495,6 +502,7 @@
   CheckRange("c");
   CheckRange("d");
   CheckRange("e");
+  CheckRange("f");
 }
 
 } // namespace
Index: clang-tools-extra/clangd/SourceCode.cpp
===
--- clang-tools-extra/clangd/SourceCode.cpp
+++ clang-tools-extra/clangd/SourceCode.cpp
@@ -296,14 +296,57 @@
  E1 < E2 ? R2.getEnd() : R1.getEnd());
 }
 
-// Returns the tokenFileRange for a given Location as a Token Range
+// Check if two locations have the same file id.
+static bool inSameFile(SourceLocation Loc1, SourceLocation Loc2,
+   const SourceManager &SM) {
+  return SM.getFileID(Loc1) == SM.getFileID(Loc2);
+}
+
+// Find an expansion range (not necessarily immediate) the ends of which are in
+// the same file id.
+static SourceRange getExpansionRangeInSameFiles(SourceLocation Loc,
+const SourceManager &SM,
+const LangOptions &LangOpts) {
+  SourceRange ExpansionRange =
+  toTokenRange(SM.getImmediateExpansionRange(Loc), SM, LangOpts);
+  // Fast path for most common cases.
+  if (inSameFile(ExpansionRange.getBegin(), ExpansionRange.getEnd(), SM))
+return ExpansionRange;
+  // Expand begin of Expansion range till the top and map with file id hash.
+  llvm::DenseMap BeginExpansions;
+  SourceRange BeginExpansion = ExpansionRange.getBegin();
+  while (1) {
+BeginExpansions[SM.getFileID(BeginExpansion.getBegin()).getHashValue()] =
+BeginExpansion;
+if (BeginExpansion.getBegin().isFileID())
+  break;
+BeginExpansion = toTokenRange(
+SM.getImmediateExpansionRange(BeginExpansion.getBegin()), SM, LangOpts);
+  }
+  // Expand the end of Expansion range till the top and find the first match in
+  // BeginExpansions.
+  SourceRange EndExpansion = ExpansionRange.getEnd();
+  while (1) {
+auto It = BeginExpansions.find(
+SM.getFileID(EndExpansion.getEnd()).getHashValue());
+if (It != BeginExpansions.end())
+  return unionTokenRange(It->second, EndExpansion, SM, LangOpts);
+if (EndExpansion.getEnd().isFileID())
+  break;
+EndExpansion = toTokenRange(
+SM.getImmediateExpansionRange(EndExpansion.getEnd()), SM, LangOpts);
+  }
+  llvm_unreachable(
+  "We should able to find a common ancestor in the expansion tree.");
+}
+// Returns the file range for a given Location as a Token Range
 // This is quite similar to getFileLoc in SourceManager as both use
 // getImmediateExpansionRange and getImmediateSpellingLoc (for macro IDs).
 // However:
 // - We want to maintain the full range information as we move from one file to
 //   the next. getFileLoc only uses the BeginLoc of getImmediateExpansionRange.
-// - We want to split '>>' tokens as the lexer parses the '>>' in template
-//   instantiations as a '>>' instead of a '>'.
+// - We want to split '>>' tokens as the lexer parses the '>>' in nested
+//   template instantiations as a '>>' instead of two '>'s.
 // There is also getExpansionRange but it simply calls
 // getImmediateExpansionRange on the begin and ends separately which is wrong.
 static SourceRange getTokenFileRange(SourceLocation Loc,
@@ -311,16 +354,20 @@
  const LangOptions &LangOpts) {
   SourceRange FileRange = Loc;
   while (!FileRange.getBegin().isFileID()) {
-assert(!FileRange.getEnd().isFileID() &&
-   "Both Begin and End should be MacroIDs.");
 if (SM.isMacroArgExpansion(FileRange.getBegin())) {
-  FileRange.setBegin(SM.getImmediateSpellingLoc(FileRange.getBegin()));
-  FileRange.setEnd(SM.getImmediateSpellingLoc(FileRange.getEnd()));
+  FileRange = unionTokenRange(
+  SM.getImmediateSpellin

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

[Aaron Ballman via Phabricator via cfe-commits]

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

This LGTM, but you should hold off a bit to commit -- @rsmith, do you have any 
concerns with this approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64838



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

[PATCH] D65120: More warnings regarding gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6572
+return false;
+  return llvm::StringSwitch(Callee->getName())
+  .Cases("begin", "rbegin", "cbegin", "crbegin", true)

xazax.hun wrote:
> gribozavr wrote:
> > Should we also check that `Callee->getParent` is an owner?
> > 
> > Otherwise the check would complain about `begin()` on, for example, a 
> > `std::string_view`.
> This is intentional. We only warn if the initialization chain can be tracked 
> back to a temporary (or local in some cases) owner. 
> So we warn for the following code:
> ```
> const char *trackThroughMultiplePointer() {
>   return std::basic_string_view(std::basic_string()).begin(); // 
> expected-warning {{returning address of local temporary object}}
> }
> ```
Makes sense, but then we should still check that `Callee->getParent` is an 
owner or a pointer.


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

https://reviews.llvm.org/D65120



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

[PATCH] D65796: [clangd] Compute scopes eagerly in IncludeFixer

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368019: [clangd] Compute scopes eagerly in IncludeFixer 
(authored by ibiryukov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65796?vs=213557&id=213582#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65796

Files:
  clang-tools-extra/trunk/clangd/IncludeFixer.cpp
  clang-tools-extra/trunk/clangd/IncludeFixer.h
  clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/trunk/clangd/IncludeFixer.h
===
--- clang-tools-extra/trunk/clangd/IncludeFixer.h
+++ clang-tools-extra/trunk/clangd/IncludeFixer.h
@@ -58,9 +58,7 @@
   struct UnresolvedName {
 std::string Name;   // E.g. "X" in foo::X.
 SourceLocation Loc; // Start location of the unresolved name.
-// Lazily get the possible scopes of the unresolved name. `Sema` must be
-// alive when this is called.
-std::function()> GetScopes;
+std::vector Scopes; // Namespace scopes we should search in.
   };
 
   /// Records the last unresolved name seen by Sema.
Index: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
@@ -797,6 +797,27 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {
+  Annotations Test(R"cpp(
+template  struct Templ {
+  template 
+  typename U::type operator=(const U &);
+};
+
+struct A {
+  Templ s;
+  A() { [[a]]; } // this caused crashes if we compute scopes lazily.
+};
+  )cpp");
+
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol({});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
+}
+
 TEST(DiagsInHeaders, DiagInsideHeader) {
   Annotations Main(R"cpp(
 #include [["a.h"]]
Index: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
===
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp
@@ -16,6 +16,7 @@
 #include "index/Symbol.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
@@ -34,6 +35,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
@@ -301,6 +303,24 @@
   return Result;
 }
 
+/// Returns all namespace scopes that the unqualified lookup would visit.
+std::vector
+collectAccessibleScopes(Sema &Sem, const DeclarationNameInfo &Typo, Scope *S,
+Sema::LookupNameKind LookupKind) {
+  std::vector Scopes;
+  VisitedContextCollector Collector;
+  Sem.LookupVisibleDecls(S, LookupKind, Collector,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+
+  Scopes.push_back("");
+  for (const auto *Ctx : Collector.takeVisitedContexts()) {
+if (isa(Ctx))
+  Scopes.push_back(printNamespaceScope(*Ctx));
+  }
+  return Scopes;
+}
+
 class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {
 public:
   UnresolvedNameRecorder(llvm::Optional &LastUnresolvedName)
@@ -321,48 +341,30 @@
 if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr))
   return clang::TypoCorrection();
 
-// This is not done lazily because `SS` can get out of scope and it's
-// relatively cheap.
 auto Extracted = extractUnresolvedNameCheaply(
 SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,
 static_cast(LookupKind) ==
 Sema::LookupNameKind::LookupNestedNameSpecifierName);
 if (!Extracted)
   return TypoCorrection();
-auto CheapUnresolved = std::move(*Extracted);
+
 UnresolvedName Unresolved;
-Unresolved.Name = CheapUnresolved.Name;
+Unresolved.Name = Extracted->Name;
 Unresolved.Loc = Typo.getBeginLoc();
-
-if (!CheapUnresolved.ResolvedScope && !S) // Give up if no scope available.
+if (!Extracted->ResolvedScope && !S) // Give up if no scope available.
   return TypoCorrection();
 
-auto *Sem = SemaPtr; // Avoid capturing `this`.
-Unresolved.GetScopes = [Sem, CheapUnresolved, S, LookupKind]() {
-  std::vector Scopes;
-  if (CheapUnresolved.ResolvedScope) {
-Scopes.push_back(

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7031
+  LLVM_FALLTHROUGH;
+case IndirectLocalPathEntry::DefaultInit:
   return Path[I].E->getSourceRange();

This change would be best committed separately (preferably with a test).



Comment at: clang/lib/Sema/SemaInit.cpp:7046
+  }
+  return GslPointerInits;
+}

I think you can go back to llvm::c_find now?



Comment at: clang/lib/Sema/SemaInit.cpp:7076
+//   int &p = *localOwner;
+//   someContainer.add(std::move(localOWner));
+//   return p;

"Owner"



Comment at: clang/lib/Sema/SemaInit.cpp:7077
+//   someContainer.add(std::move(localOWner));
+//   return p;
+if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) &&

Why is it a false positive? `std::move` left memory owned by `localOwner` in 
unspecified state.


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

https://reviews.llvm.org/D64256



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

[clang-tools-extra] r368019 - [clangd] Compute scopes eagerly in IncludeFixer

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Tue Aug  6 04:37:50 2019
New Revision: 368019

URL: http://llvm.org/viewvc/llvm-project?rev=368019&view=rev
Log:
[clangd] Compute scopes eagerly in IncludeFixer

Summary:
Computing lazily leads to crashes. In particular, computing scopes may
produce diagnostics (from inside template instantiations) and we
currently do it when processing another diagnostic, which leads to
crashes.

Moreover, we remember and access 'Scope*' when computing scopes. This
might lead to invalid memory access if the Scope is deleted by the time
we run the delayed computation. We did not actually construct an example
when this happens, though.

From the VCS and review history, it seems the optimization was
introduced in the initial version without a mention of any performance
benchmarks justifying the performance gains. This led me to a
conclusion that the optimization was premature, so removing it to avoid
crashes seems like the right trade-off at that point.

Reviewers: sammccall

Reviewed By: sammccall

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

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/IncludeFixer.cpp
clang-tools-extra/trunk/clangd/IncludeFixer.h
clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

Modified: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=368019&r1=368018&r2=368019&view=diff
==
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp (original)
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp Tue Aug  6 04:37:50 2019
@@ -16,6 +16,7 @@
 #include "index/Symbol.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
@@ -34,6 +35,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
@@ -301,6 +303,24 @@ llvm::Optional extr
   return Result;
 }
 
+/// Returns all namespace scopes that the unqualified lookup would visit.
+std::vector
+collectAccessibleScopes(Sema &Sem, const DeclarationNameInfo &Typo, Scope *S,
+Sema::LookupNameKind LookupKind) {
+  std::vector Scopes;
+  VisitedContextCollector Collector;
+  Sem.LookupVisibleDecls(S, LookupKind, Collector,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+
+  Scopes.push_back("");
+  for (const auto *Ctx : Collector.takeVisitedContexts()) {
+if (isa(Ctx))
+  Scopes.push_back(printNamespaceScope(*Ctx));
+  }
+  return Scopes;
+}
+
 class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {
 public:
   UnresolvedNameRecorder(llvm::Optional &LastUnresolvedName)
@@ -321,48 +341,30 @@ public:
 if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr))
   return clang::TypoCorrection();
 
-// This is not done lazily because `SS` can get out of scope and it's
-// relatively cheap.
 auto Extracted = extractUnresolvedNameCheaply(
 SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,
 static_cast(LookupKind) ==
 Sema::LookupNameKind::LookupNestedNameSpecifierName);
 if (!Extracted)
   return TypoCorrection();
-auto CheapUnresolved = std::move(*Extracted);
+
 UnresolvedName Unresolved;
-Unresolved.Name = CheapUnresolved.Name;
+Unresolved.Name = Extracted->Name;
 Unresolved.Loc = Typo.getBeginLoc();
-
-if (!CheapUnresolved.ResolvedScope && !S) // Give up if no scope available.
+if (!Extracted->ResolvedScope && !S) // Give up if no scope available.
   return TypoCorrection();
 
-auto *Sem = SemaPtr; // Avoid capturing `this`.
-Unresolved.GetScopes = [Sem, CheapUnresolved, S, LookupKind]() {
-  std::vector Scopes;
-  if (CheapUnresolved.ResolvedScope) {
-Scopes.push_back(*CheapUnresolved.ResolvedScope);
-  } else {
-assert(S);
-// No scope specifier is specified. Collect all accessible scopes in 
the
-// context.
-VisitedContextCollector Collector;
-Sem->LookupVisibleDecls(
-S, static_cast(LookupKind), Collector,
-/*IncludeGlobalScope=*/false,
-/*LoadExternal=*/false);
-
-Scopes.push_back("");
-for (const auto *Ctx : Collector.takeVisitedContexts())
-  if (isa(Ctx))
-Scopes.push_back(printNamespaceScope(*Ctx));
-  }
+if (Extracted->ResolvedScope)
+  Unresolved.Scopes.push_back(*Extracted->ResolvedScope);
+else // no qualifier or qualifier is unresolved.
+  Unresolved.Scopes = collectAccessibleScopes(
+

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[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/include/clang/Sema/Sema.h:6122
+  /// std::container::iterator. \param UnderlyingRecord The record named by ND.
+  void addDefaultGslPointerAttribute(NamedDecl *ND,
+ CXXRecordDecl *UnderlyingRecord);

s/addDefault/infer/ ?  (everywhere in the patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6581
+if (!Callee->getIdentifier()) {
+  auto OO = Callee->getOverloadedOperator();
+  return OO == OverloadedOperatorKind::OO_Subscript ||

mgehre wrote:
> xazax.hun wrote:
> > If we want to relax the warnings to give more results we could extend the 
> > checking of these overloaded operators for annotated types. But this would 
> > imply that the user need to have the expected semantics for those types and 
> > can only suppress false positives by removing some gsl:::owner/poinnter 
> > annotations.
> I see those options:
> - Either gsl::Owner implies some specific form of those operators (and if 
> that does not hold for a class, then one should not annotate it with 
> gsl::Owner)
> - or gsl::Owner only implies some specific behavior for the "gsl::Pointer 
> constructed from gsl::Owner" case and everything else requires additional 
> annotation
> I expect that we will experiment a bit in the future to see what works well 
> for real-world code.
I understand the difficulty, but I don't think it is appropriate to experiment 
by ourselves -- these attributes are defined in a spec, and if something is not 
clear, the spec should be clarified.


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

https://reviews.llvm.org/D65127



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

[PATCH] D65445: [CrossTU] Handle case when no USR could be generated during Decl search.

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

balazske updated this revision to Diff 213571.
balazske added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65445

Files:
  clang/include/clang/CrossTU/CrossTranslationUnit.h
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  clang/test/Analysis/ctu-main.cpp
  clang/test/Analysis/func-mapping-test.cpp
  clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

Index: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
===
--- clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
+++ clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
@@ -76,7 +76,12 @@
 
 void MapExtDefNamesConsumer::addIfInMain(const DeclaratorDecl *DD,
  SourceLocation defStart) {
-  std::string LookupName = CrossTranslationUnitContext::getLookupName(DD);
+  llvm::Optional LookupName =
+  CrossTranslationUnitContext::getLookupName(DD);
+  if (!LookupName)
+return;
+  assert(!LookupName->empty() && "Lookup name should be non-empty.");
+
   if (CurrentFileName.empty()) {
 CurrentFileName =
 SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
@@ -89,7 +94,7 @@
   case VisibleNoLinkage:
   case UniqueExternalLinkage:
 if (SM.isInMainFile(defStart))
-  Index[LookupName] = CurrentFileName;
+  Index[*LookupName] = CurrentFileName;
 break;
   default:
 break;
Index: clang/test/Analysis/func-mapping-test.cpp
===
--- clang/test/Analysis/func-mapping-test.cpp
+++ clang/test/Analysis/func-mapping-test.cpp
@@ -41,3 +41,10 @@
 };
 U u = {.a = 6};
 // CHECK-DAG: c:@u
+
+// No USR can be generated for this.
+// Check for no crash in this case.
+static union {
+  float uf;
+  const int ui;
+};
Index: clang/test/Analysis/ctu-main.cpp
===
--- clang/test/Analysis/ctu-main.cpp
+++ clang/test/Analysis/ctu-main.cpp
@@ -112,6 +112,19 @@
   clang_analyzer_eval(obj->fvcl(1) == 8);  // expected-warning{{FALSE}} expected-warning{{TRUE}}
 }
 
+class TestAnonUnionUSR {
+public:
+  inline float f(int value) {
+union {
+  float f;
+  int i;
+};
+i = value;
+return f;
+  }
+  static const int Test;
+};
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
@@ -144,4 +157,5 @@
   clang_analyzer_eval(extSubSCN.a == 1); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCC.a == 7); // TODO
   clang_analyzer_eval(extU.a == 4); // expected-warning{{TRUE}}
+  clang_analyzer_eval(TestAnonUnionUSR::Test == 5); // expected-warning{{TRUE}}
 }
Index: clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
===
--- clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
+++ clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
@@ -25,3 +25,4 @@
 c:@extSubSCN ctu-other.cpp.ast
 c:@extSCC ctu-other.cpp.ast
 c:@extU ctu-other.cpp.ast
+c:@S@TestAnonUnionUSR@Test ctu-other.cpp.ast
Index: clang/test/Analysis/Inputs/ctu-other.cpp
===
--- clang/test/Analysis/Inputs/ctu-other.cpp
+++ clang/test/Analysis/Inputs/ctu-other.cpp
@@ -131,3 +131,17 @@
   const unsigned int b;
 };
 U extU = {.a = 4};
+
+class TestAnonUnionUSR {
+public:
+  inline float f(int value) {
+union {
+  float f;
+  int i;
+};
+i = value;
+return f;
+  }
+  static const int Test;
+};
+const int TestAnonUnionUSR::Test = 5;
Index: clang/lib/CrossTU/CrossTranslationUnit.cpp
===
--- clang/lib/CrossTU/CrossTranslationUnit.cpp
+++ clang/lib/CrossTU/CrossTranslationUnit.cpp
@@ -193,12 +193,13 @@
 
 CrossTranslationUnitContext::~CrossTranslationUnitContext() {}
 
-std::string CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
+llvm::Optional
+CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
   SmallString<128> DeclUSR;
   bool Ret = index::generateUSRForDecl(ND, DeclUSR);
-  (void)Ret;
-  assert(!Ret && "Unable to generate USR");
-  return DeclUSR.str();
+  if (Ret)
+return {};
+  return std::string(DeclUSR.str());
 }
 
 /// Recursively visits the decls of a DeclContext, and returns one with the
@@ -218,7 +219,8 @@
 const T *ResultDecl;
 if (!ND || !hasBodyOrInit(ND, ResultDecl))
   continue;
-if (getLookupName(ResultDecl) != LookupName)
+llvm::Optional ResultLookupName = getLookupName(ResultDecl);
+if (!ResultLookupName || *ResultLookupName != LookupName)
   continue;
 return ResultDecl;
   }
@@ -233,12 +235,12 @@
   assert(!hasBody

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

Still looks good to me, other than some style nits.




Comment at: clang/lib/AST/ASTImporter.cpp:3829
 
+Error ASTNodeImporter::ImportDefaultArgOfParmVarDecl(const ParmVarDecl 
*FromParam, ParmVarDecl *ToParam) {
+  ToParam->setHasInheritedDefaultArg(FromParam->hasInheritedDefaultArg());

This line seems to be longer than 80 columns, forgot to run clang-format?



Comment at: clang/lib/AST/ASTImporter.cpp:6943
+Optional FromParam = Importer.getImportedFromDecl(ToParam);
+assert(FromParam && "Parameter value was not imported?");
+

This would be more appropriate I guess: "ParmVarDecl was not imported?"




Comment at: clang/test/Analysis/Inputs/ctu-other.cpp:135
+
+struct DefParmContext {
+  static const int I;

shafik wrote:
> martong wrote:
> > Perhaps we could write `Default` instead of `Def`.
> +1 to this and the name suggestion below.
Please do not forget to rename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577



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

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

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

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

Clarified comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
  clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -0,0 +1,17 @@
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';
+import * as path from 'path';
+
+import * as TM from '../src/semantic-highlighting';
+
+suite('TextMate Tests', () => {
+  test('Parses arrays of textmate themes.', async () => {
+const themePath = path.join(__dirname, '../../test/assets/testTheme.jsonc');
+const scopeColorRules = await TM.parseThemeFile(themePath);
+const getScopeRule = (scope: string) =>
+scopeColorRules.find((v) => v.scope === scope);
+assert.equal(scopeColorRules.length, 2);
+assert.deepEqual(getScopeRule('a'), {scope : 'a', textColor : '#fff'});
+assert.deepEqual(getScopeRule('b'), {scope : 'b', textColor : '#000'});
+  });
+});
\ No newline at end of file
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
@@ -0,0 +1,5 @@
+
+http://www.apple.com/DTDs/PropertyList-1.0.dtd";>
+
+
+
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
@@ -0,0 +1,16 @@
+{
+// Some comment
+"include": "firstIncludedTheme.jsonc",
+"name": "TestTheme",
+"type": "dark",
+"colors": {
+"dropdown.background": "#fff"
+},
+"tokenColors": [
+{
+"settings": {
+"foreground": "#fff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
@@ -0,0 +1,11 @@
+{
+"include": "tmThemeInclude.tmTheme",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#ff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
@@ -0,0 +1,18 @@
+{
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#fff"
+}
+},
+{
+"scope": ["a", "b"],
+"settings": {
+"foreground": "#000"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -0,0 +1,97 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+interface TokenColorRule {
+  scope: string, textColor: string,
+}
+
+// Gets a TextMate theme and all its included themes by its name.
+async function loadTheme(themeName: string): Promise {
+  const extension =
+  vscode.extensions.all.find((extension: vscode.Extension) => {
+const contribs = extension.packageJSON.contributes;
+if (!contribs || !contribs.themes)
+  return false;
+return contribs.themes.some((theme: any) => theme.id === themeName ||
+theme.label === themeName);
+  });
+
+  if (!extension) {
+return Promise.reject('Could not find a theme with name: ' + themeName);
+  }
+
+  const extensionInfo

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

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

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

Renamed file to semantic-highlighting.ts. Added test for parsing theme files. 
Inlined the parsing into the loading function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
  clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -0,0 +1,17 @@
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';
+import * as path from 'path';
+
+import * as TM from '../src/semantic-highlighting';
+
+suite('TextMate Tests', () => {
+  test('Parses arrays of textmate themes.', async () => {
+const themePath = path.join(__dirname, '../../test/assets/testTheme.jsonc');
+const scopeColorRules = await TM.parseThemeFile(themePath);
+const getScopeRule = (scope: string) =>
+scopeColorRules.find((v) => v.scope === scope);
+assert.equal(scopeColorRules.length, 2);
+assert.deepEqual(getScopeRule('a'), {scope : 'a', textColor : '#fff'});
+assert.deepEqual(getScopeRule('b'), {scope : 'b', textColor : '#000'});
+  });
+});
\ No newline at end of file
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
@@ -0,0 +1,5 @@
+
+http://www.apple.com/DTDs/PropertyList-1.0.dtd";>
+
+
+
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
@@ -0,0 +1,16 @@
+{
+// Some comment
+"include": "firstIncludedTheme.jsonc",
+"name": "TestTheme",
+"type": "dark",
+"colors": {
+"dropdown.background": "#fff"
+},
+"tokenColors": [
+{
+"settings": {
+"foreground": "#fff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
@@ -0,0 +1,11 @@
+{
+"include": "tmThemeInclude.tmTheme",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#ff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
@@ -0,0 +1,18 @@
+{
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#fff"
+}
+},
+{
+"scope": ["a", "b"],
+"settings": {
+"foreground": "#000"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -0,0 +1,94 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+interface TokenColorRule {
+  scope: string, textColor: string,
+}
+
+// Gets a TextMate theme by its name and all its included themes.
+async function loadTheme(themeName: string): Promise {
+  const extension =
+  vscode.extensions.all.find((extension: vscode.Extension) => {
+const contribs = extension.packageJSON.contributes;
+if (!contribs || !contribs.themes)
+  return false;
+return contribs.themes.some((theme: any) => theme.id === themeName ||
+theme.label === the

[PATCH] D65510: [clangd] Fix implicit template instatiations appearing as topLevelDecls.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/AST.h:86
 
+bool isImplicitTemplateInstantiation(const NamedDecl *D);
+bool isExplicitTemplateSpecialization(const NamedDecl *D);

Could you add a small comment with an example?
```
/// Indicates \p D is a template instantiation implicitly generated by the 
compiler, e.g.
/// template  struct vector {};
/// vector v; // 'vector' is an implicit instantiation
bool isImplicitTemplateInstantiation(const NamedDecl *D);

/// Indicates \p D is an explicit template specialization, e.g.
///   template  struct vector {};
///   template <> struct vector {}; // <-- explicit specialization
///
/// Note that explicit instantiations are NOT explicit specializations, albeit 
they look similar.
///   template struct vector; // <-- explicit instantiation, NOT an 
explicit specialization.
bool isExplicitTemplateSpecialization(const NamedDecl *D);
```





Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:110
+template
+void f(T) {}
+void s() {

Could you also check that:

1.  explicit specializations are present
```
template <>
void f(bool) {}
```
2. explicit instantiations are absent
```
template void f(bool);
```
3. partial specializations are present (they should not be affected by this 
change, but testing those here seems appropriate)
```
template 
struct vector {};

template 
struct vector {}; // partial specialization, should be present
```
4. template variables and classes are also handled:
```
template 
T foo = 10; // (!) requires C++17
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65510



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

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

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

balazske updated this revision to Diff 213563.
balazske added a comment.

Small but important fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  clang/test/Analysis/ctu-main.cpp

Index: clang/test/Analysis/ctu-main.cpp
===
--- clang/test/Analysis/ctu-main.cpp
+++ clang/test/Analysis/ctu-main.cpp
@@ -112,6 +112,8 @@
   clang_analyzer_eval(obj->fvcl(1) == 8);  // expected-warning{{FALSE}} expected-warning{{TRUE}}
 }
 
+extern int testDefParmIncompleteImport(int);
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
@@ -144,4 +146,6 @@
   clang_analyzer_eval(extSubSCN.a == 1); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCC.a == 7); // TODO
   clang_analyzer_eval(extU.a == 4); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(testDefParmIncompleteImport(9) == 9); // expected-warning{{TRUE}}
 }
Index: clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
===
--- clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
+++ clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
@@ -25,3 +25,4 @@
 c:@extSubSCN ctu-other.cpp.ast
 c:@extSCC ctu-other.cpp.ast
 c:@extU ctu-other.cpp.ast
+c:@F@testDefParmIncompleteImport#I# ctu-other.cpp.ast
Index: clang/test/Analysis/Inputs/ctu-other.cpp
===
--- clang/test/Analysis/Inputs/ctu-other.cpp
+++ clang/test/Analysis/Inputs/ctu-other.cpp
@@ -131,3 +131,22 @@
   const unsigned int b;
 };
 U extU = {.a = 4};
+
+struct DefParmContext {
+  static const int I;
+  int f();
+};
+
+int fDefParm(int I = DefParmContext::I) {
+  return I;
+}
+
+int testDefParmIncompleteImport(int I) {
+  return fDefParm(I);
+}
+
+const int DefParmContext::I = 0;
+
+int DefParmContext::f() {
+  return fDefParm();
+}
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -426,6 +426,8 @@
 
 Error ImportFunctionDeclBody(FunctionDecl *FromFD, FunctionDecl *ToFD);
 
+Error ImportDefaultArgOfParmVarDecl(const ParmVarDecl *FromParam, ParmVarDecl *ToParam);
+
 template 
 bool hasSameVisibilityContext(T *Found, T *From);
 
@@ -3824,6 +3826,27 @@
   return ToParm;
 }
 
+Error ASTNodeImporter::ImportDefaultArgOfParmVarDecl(const ParmVarDecl *FromParam, ParmVarDecl *ToParam) {
+  ToParam->setHasInheritedDefaultArg(FromParam->hasInheritedDefaultArg());
+  ToParam->setKNRPromoted(FromParam->isKNRPromoted());
+
+  if (FromParam->hasUninstantiatedDefaultArg()) {
+if (auto ToDefArgOrErr = import(FromParam->getUninstantiatedDefaultArg()))
+  ToParam->setUninstantiatedDefaultArg(*ToDefArgOrErr);
+else
+  return ToDefArgOrErr.takeError();
+  } else if (FromParam->hasUnparsedDefaultArg()) {
+ToParam->setUnparsedDefaultArg();
+  } else if (FromParam->hasDefaultArg()) {
+if (auto ToDefArgOrErr = import(FromParam->getDefaultArg()))
+  ToParam->setDefaultArg(*ToDefArgOrErr);
+else
+  return ToDefArgOrErr.takeError();
+  }
+
+  return Error::success();
+}
+
 ExpectedDecl ASTNodeImporter::VisitParmVarDecl(ParmVarDecl *D) {
   // Parameters are created in the translation unit's context, then moved
   // into the function declaration's context afterward.
@@ -3850,23 +3873,11 @@
   /*DefaultArg*/ nullptr))
 return ToParm;
 
-  // Set the default argument.
-  ToParm->setHasInheritedDefaultArg(D->hasInheritedDefaultArg());
-  ToParm->setKNRPromoted(D->isKNRPromoted());
-
-  if (D->hasUninstantiatedDefaultArg()) {
-if (auto ToDefArgOrErr = import(D->getUninstantiatedDefaultArg()))
-  ToParm->setUninstantiatedDefaultArg(*ToDefArgOrErr);
-else
-  return ToDefArgOrErr.takeError();
-  } else if (D->hasUnparsedDefaultArg()) {
-ToParm->setUnparsedDefaultArg();
-  } else if (D->hasDefaultArg()) {
-if (auto ToDefArgOrErr = import(D->getDefaultArg()))
-  ToParm->setDefaultArg(*ToDefArgOrErr);
-else
-  return ToDefArgOrErr.takeError();
-  }
+  // Set the default argument. It should be no problem if it was already done.
+  // Do not import the default expression before GetImportedOrCreateDecl call
+  // to avoid possible infinite import loop because circular dependency.
+  if (Error Err = ImportDefaultArgOfParmVarDecl(D, ToParm))
+return std::move(Err);
 
   if (D->isObjCMethodParameter()) {
 ToParm->setObjCMethodScopeInfo(D->getFunctionScopeIndex());
@@ -6920,6 +6931,21 @@
   if (!UsedContextOrErr)
 return UsedCo

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

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Add the missing newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64907

Files:
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/RecursiveASTVisitorTest.cpp

Index: clang/unittests/AST/RecursiveASTVisitorTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/RecursiveASTVisitorTest.cpp
@@ -0,0 +1,106 @@
+//===- unittest/AST/RecursiveASTVisitorTest.cpp ---===//
+//
+// 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 "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock-generated-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace clang;
+using ::testing::ElementsAre;
+
+namespace {
+class ProcessASTAction : public clang::ASTFrontendAction {
+public:
+  ProcessASTAction(llvm::unique_function Process)
+  : Process(std::move(Process)) {
+assert(this->Process);
+  }
+
+  std::unique_ptr CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) {
+class Consumer : public ASTConsumer {
+public:
+  Consumer(llvm::function_ref Process)
+  : Process(Process) {}
+
+  void HandleTranslationUnit(ASTContext &Ctx) override { Process(Ctx); }
+
+private:
+  llvm::function_ref Process;
+};
+
+return llvm::make_unique(Process);
+  }
+
+private:
+  llvm::unique_function Process;
+};
+
+enum class VisitEvent {
+  StartTraverseFunction,
+  EndTraverseFunction,
+  StartTraverseAttr,
+  EndTraverseAttr
+};
+
+class CollectInterestingEvents
+: public RecursiveASTVisitor {
+public:
+  bool TraverseFunctionDecl(FunctionDecl *D) {
+Events.push_back(VisitEvent::StartTraverseFunction);
+bool Ret = RecursiveASTVisitor::TraverseFunctionDecl(D);
+Events.push_back(VisitEvent::EndTraverseFunction);
+
+return Ret;
+  }
+
+  bool TraverseAttr(Attr *A) {
+Events.push_back(VisitEvent::StartTraverseAttr);
+bool Ret = RecursiveASTVisitor::TraverseAttr(A);
+Events.push_back(VisitEvent::EndTraverseAttr);
+
+return Ret;
+  }
+
+  std::vector takeEvents() && { return std::move(Events); }
+
+private:
+  std::vector Events;
+};
+
+std::vector collectEvents(llvm::StringRef Code) {
+  CollectInterestingEvents Visitor;
+  clang::tooling::runToolOnCode(
+  new ProcessASTAction(
+  [&](clang::ASTContext &Ctx) { Visitor.TraverseAST(Ctx); }),
+  Code);
+  return std::move(Visitor).takeEvents();
+}
+} // namespace
+
+TEST(RecursiveASTVisitorTest, AttributesInsideDecls) {
+  /// Check attributes are traversed inside TraverseFunctionDecl.
+  llvm::StringRef Code = R"cpp(
+__attribute__((annotate("something"))) int foo() { return 10; }
+  )cpp";
+
+  EXPECT_THAT(collectEvents(Code),
+  ElementsAre(VisitEvent::StartTraverseFunction,
+  VisitEvent::StartTraverseAttr,
+  VisitEvent::EndTraverseAttr,
+  VisitEvent::EndTraverseFunction));
+}
Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -26,6 +26,7 @@
   Language.cpp
   NamedDeclPrinterTest.cpp
   OMPStructuredBlockTest.cpp
+  RecursiveASTVisitorTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   StructuralEquivalenceTest.cpp
Index: clang/include/clang/AST/RecursiveASTVisitor.h
===
--- clang/include/clang/AST/RecursiveASTVisitor.h
+++ clang/include/clang/AST/RecursiveASTVisitor.h
@@ -722,12 +722,6 @@
 break;
 #include "clang/AST/DeclNodes.inc"
   }
-
-  // Visit any attributes attached to this declaration.
-  for (auto *I : D->attrs()) {
-if (!getDerived().TraverseAttr(I))
-  return false;
-  }
   return true;
 }
 
@@ -1407,6 +1401,11 @@
 { CODE; }  \
 if (ReturnValue && ShouldVisitChildren)\
   TRY_TO(TraverseDeclContextHelper(dyn_cast(D))); \
+if (ReturnValue) { \
+  /* Visit any attributes attached to this declaration. */  

[PATCH] D65796: [clangd] Compute scopes eagerly in IncludeFixer

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Make sure the test actually runs IncludeFixer.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65796

Files:
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/IncludeFixer.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -797,6 +797,27 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {
+  Annotations Test(R"cpp(
+template  struct Templ {
+  template 
+  typename U::type operator=(const U &);
+};
+
+struct A {
+  Templ s;
+  A() { [[a]]; } // this caused crashes if we compute scopes lazily.
+};
+  )cpp");
+
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol({});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
+}
+
 TEST(DiagsInHeaders, DiagInsideHeader) {
   Annotations Main(R"cpp(
 #include [["a.h"]]
Index: clang-tools-extra/clangd/IncludeFixer.h
===
--- clang-tools-extra/clangd/IncludeFixer.h
+++ clang-tools-extra/clangd/IncludeFixer.h
@@ -58,9 +58,7 @@
   struct UnresolvedName {
 std::string Name;   // E.g. "X" in foo::X.
 SourceLocation Loc; // Start location of the unresolved name.
-// Lazily get the possible scopes of the unresolved name. `Sema` must be
-// alive when this is called.
-std::function()> GetScopes;
+std::vector Scopes; // Namespace scopes we should search in.
   };
 
   /// Records the last unresolved name seen by Sema.
Index: clang-tools-extra/clangd/IncludeFixer.cpp
===
--- clang-tools-extra/clangd/IncludeFixer.cpp
+++ clang-tools-extra/clangd/IncludeFixer.cpp
@@ -16,6 +16,7 @@
 #include "index/Symbol.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
@@ -34,6 +35,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
@@ -301,6 +303,24 @@
   return Result;
 }
 
+/// Returns all namespace scopes that the unqualified lookup would visit.
+std::vector
+collectAccessibleScopes(Sema &Sem, const DeclarationNameInfo &Typo, Scope *S,
+Sema::LookupNameKind LookupKind) {
+  std::vector Scopes;
+  VisitedContextCollector Collector;
+  Sem.LookupVisibleDecls(S, LookupKind, Collector,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+
+  Scopes.push_back("");
+  for (const auto *Ctx : Collector.takeVisitedContexts()) {
+if (isa(Ctx))
+  Scopes.push_back(printNamespaceScope(*Ctx));
+  }
+  return Scopes;
+}
+
 class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {
 public:
   UnresolvedNameRecorder(llvm::Optional &LastUnresolvedName)
@@ -321,48 +341,30 @@
 if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr))
   return clang::TypoCorrection();
 
-// This is not done lazily because `SS` can get out of scope and it's
-// relatively cheap.
 auto Extracted = extractUnresolvedNameCheaply(
 SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,
 static_cast(LookupKind) ==
 Sema::LookupNameKind::LookupNestedNameSpecifierName);
 if (!Extracted)
   return TypoCorrection();
-auto CheapUnresolved = std::move(*Extracted);
+
 UnresolvedName Unresolved;
-Unresolved.Name = CheapUnresolved.Name;
+Unresolved.Name = Extracted->Name;
 Unresolved.Loc = Typo.getBeginLoc();
-
-if (!CheapUnresolved.ResolvedScope && !S) // Give up if no scope available.
+if (!Extracted->ResolvedScope && !S) // Give up if no scope available.
   return TypoCorrection();
 
-auto *Sem = SemaPtr; // Avoid capturing `this`.
-Unresolved.GetScopes = [Sem, CheapUnresolved, S, LookupKind]() {
-  std::vector Scopes;
-  if (CheapUnresolved.ResolvedScope) {
-Scopes.push_back(*CheapUnresolved.ResolvedScope);
-  } else {
-assert(S);
-// No scope specifier is specified. Collect all accessible scopes in the
-// context.
-VisitedContextCollector Collector;
-Sem->LookupVisibleDecls(
-S

[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.

Ping!


Repository:
  rG LLVM Github Monorepo

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] D65573: Add User docs for ASTImporter

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368009: Add User docs for ASTImporter (authored by martong, 
committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65573?vs=213332&id=213556#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65573

Files:
  cfe/trunk/docs/LibASTImporter.rst
  cfe/trunk/docs/index.rst

Index: cfe/trunk/docs/index.rst
===
--- cfe/trunk/docs/index.rst
+++ cfe/trunk/docs/index.rst
@@ -61,6 +61,7 @@
RAVFrontendAction
LibASTMatchersTutorial
LibASTMatchers
+   LibASTImporter
HowToSetupToolingForLLVM
JSONCompilationDatabase
RefactoringEngine
Index: cfe/trunk/docs/LibASTImporter.rst
===
--- cfe/trunk/docs/LibASTImporter.rst
+++ cfe/trunk/docs/LibASTImporter.rst
@@ -0,0 +1,613 @@
+===
+ASTImporter: Merging Clang ASTs
+===
+
+The ``ASTImporter`` class is part of Clang's core library, the AST library.
+It imports nodes of an ``ASTContext`` into another ``ASTContext``.
+
+In this document, we assume basic knowledge about the Clang AST.  See the :doc:`Introduction
+to the Clang AST ` if you want to learn more
+about how the AST is structured.
+Knowledge about :doc:`matching the Clang AST ` and the `reference for the matchers `_ are also useful.
+
+.. contents::
+   :local:
+
+Introduction
+
+
+``ASTContext`` holds long-lived AST nodes (such as types and decls) that can be referred to throughout the semantic analysis of a file.
+In some cases it is preferable to work with more than one ``ASTContext``.
+For example, we'd like to parse multiple different files inside the same Clang tool.
+It may be convenient if we could view the set of the resulting ASTs as if they were one AST resulting from the parsing of each file together.
+``ASTImporter`` provides the way to copy types or declarations from one ``ASTContext`` to another.
+We refer to the context from which we import as the **"from" context** or *source context*; and the context into which we import as the **"to" context** or *destination context*.
+
+Existing clients of the ``ASTImporter`` library are Cross Translation Unit (CTU) static analysis and the LLDB expression parser.
+CTU static analysis imports a definition of a function if its definition is found in another translation unit (TU).
+This way the analysis can breach out from the single TU limitation.
+LLDB's ``expr`` command parses a user-defined expression, creates an ``ASTContext`` for that and then imports the missing definitions from the AST what we got from the debug information (DWARF, etc).
+
+Algorithm of the import
+---
+
+Importing one AST node copies that node into the destination ``ASTContext``.
+Why do we have to copy the node?
+Isn't enough to insert the pointer to that node into the destination context?
+One reason is that the "from" context may outlive the "to" context.
+Also, the Clang AST consider nodes (or certain properties of nodes) equivalent if they have the same address!
+
+The import algorithm has to ensure that the structurally equivalent nodes in the different translation units are not getting duplicated in the merged AST.
+E.g. if we include the definition of the vector template (``#include ``) in two translation units, then their merged AST should have only one node which represents the template.
+Also, we have to discover *one definition rule* (ODR) violations.
+For instance, if there is a class definition with the same name in both translation units, but one of the definition contains a different number of fields.
+So, we look up existing definitions, and then we check the structural equivalency on those nodes.
+The following pseudo-code demonstrates the basics of the import mechanism:
+
+.. code-block:: cpp
+
+  // Pseudo-code(!) of import:
+  ErrorOrDecl Import(Decl *FromD) {
+Decl *ToDecl = nullptr;
+FoundDeclsList = Look up all Decls in the "to" Ctx with the same name of FromD;
+for (auto FoundDecl : FoundDeclsList) {
+  if (StructurallyEquivalentDecls(FoundDecl, FromD)) {
+ToDecl = FoundDecl;
+Mark FromD as imported;
+break;
+  } else {
+Report ODR violation;
+return error;
+  }
+}
+if (FoundDeclsList is empty) {
+  Import dependent declarations and types of ToDecl;
+  ToDecl = create a new AST node in "to" Ctx;
+  Mark FromD as imported;
+}
+return ToDecl;
+  }
+
+Two AST nodes are *structurally equivalent* if they are
+
+- builtin types and refer to the same type, e.g. ``int`` and ``int`` are structurally equivalent,
+- function types and all their parameters h

r368009 - Add User docs for ASTImporter

[Gabor Marton via cfe-commits]

Author: martong
Date: Tue Aug  6 02:52:21 2019
New Revision: 368009

URL: http://llvm.org/viewvc/llvm-project?rev=368009&view=rev
Log:
Add User docs for ASTImporter

Summary:
This document includes the description of the ASTImporter from the user/client
perspective.
A subsequent patch will describe the development internals.

Reviewers: a_sidorin, shafik, gamesh411, balazske, a.sidorin

Subscribers: rnkovacs, dkrupp, arphaman, Szelethus, cfe-commits

Tags: #clang

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

Added:
cfe/trunk/docs/LibASTImporter.rst
Modified:
cfe/trunk/docs/index.rst

Added: cfe/trunk/docs/LibASTImporter.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTImporter.rst?rev=368009&view=auto
==
--- cfe/trunk/docs/LibASTImporter.rst (added)
+++ cfe/trunk/docs/LibASTImporter.rst Tue Aug  6 02:52:21 2019
@@ -0,0 +1,613 @@
+===
+ASTImporter: Merging Clang ASTs
+===
+
+The ``ASTImporter`` class is part of Clang's core library, the AST library.
+It imports nodes of an ``ASTContext`` into another ``ASTContext``.
+
+In this document, we assume basic knowledge about the Clang AST.  See the 
:doc:`Introduction
+to the Clang AST ` if you want to learn more
+about how the AST is structured.
+Knowledge about :doc:`matching the Clang AST ` and the 
`reference for the matchers 
`_ are also useful.
+
+.. contents::
+   :local:
+
+Introduction
+
+
+``ASTContext`` holds long-lived AST nodes (such as types and decls) that can 
be referred to throughout the semantic analysis of a file.
+In some cases it is preferable to work with more than one ``ASTContext``.
+For example, we'd like to parse multiple different files inside the same Clang 
tool.
+It may be convenient if we could view the set of the resulting ASTs as if they 
were one AST resulting from the parsing of each file together.
+``ASTImporter`` provides the way to copy types or declarations from one 
``ASTContext`` to another.
+We refer to the context from which we import as the **"from" context** or 
*source context*; and the context into which we import as the **"to" context** 
or *destination context*.
+
+Existing clients of the ``ASTImporter`` library are Cross Translation Unit 
(CTU) static analysis and the LLDB expression parser.
+CTU static analysis imports a definition of a function if its definition is 
found in another translation unit (TU).
+This way the analysis can breach out from the single TU limitation.
+LLDB's ``expr`` command parses a user-defined expression, creates an 
``ASTContext`` for that and then imports the missing definitions from the AST 
what we got from the debug information (DWARF, etc).
+
+Algorithm of the import
+---
+
+Importing one AST node copies that node into the destination ``ASTContext``.
+Why do we have to copy the node?
+Isn't enough to insert the pointer to that node into the destination context?
+One reason is that the "from" context may outlive the "to" context.
+Also, the Clang AST consider nodes (or certain properties of nodes) equivalent 
if they have the same address!
+
+The import algorithm has to ensure that the structurally equivalent nodes in 
the different translation units are not getting duplicated in the merged AST.
+E.g. if we include the definition of the vector template (``#include 
``) in two translation units, then their merged AST should have only 
one node which represents the template.
+Also, we have to discover *one definition rule* (ODR) violations.
+For instance, if there is a class definition with the same name in both 
translation units, but one of the definition contains a different number of 
fields.
+So, we look up existing definitions, and then we check the structural 
equivalency on those nodes.
+The following pseudo-code demonstrates the basics of the import mechanism:
+
+.. code-block:: cpp
+
+  // Pseudo-code(!) of import:
+  ErrorOrDecl Import(Decl *FromD) {
+Decl *ToDecl = nullptr;
+FoundDeclsList = Look up all Decls in the "to" Ctx with the same name of 
FromD;
+for (auto FoundDecl : FoundDeclsList) {
+  if (StructurallyEquivalentDecls(FoundDecl, FromD)) {
+ToDecl = FoundDecl;
+Mark FromD as imported;
+break;
+  } else {
+Report ODR violation;
+return error;
+  }
+}
+if (FoundDeclsList is empty) {
+  Import dependent declarations and types of ToDecl;
+  ToDecl = create a new AST node in "to" Ctx;
+  Mark FromD as imported;
+}
+return ToDecl;
+  }
+
+Two AST nodes are *structurally equivalent* if they are
+
+- builtin types and refer to the same type, e.g. ``int`` and ``int`` are 
structurally equivalent,
+- function types and all their parameters have structurally equivalent types,
+- record types and all their fields in order of thei

[PATCH] D65796: [clangd] Compute scopes eagerly in IncludeFixer

[Ilya Biryukov via Phabricator via cfe-commits]

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

Computing lazily leads to crashes. In particular, computing scopes may
produce diagnostics (from inside template instantiations) and we
currently do it when processing another diagnostic, which leads to
crashes.

Moreover, we remember and access 'Scope*' when computing scopes. This
might lead to invalid memory access if the Scope is deleted by the time
we run the delayed computation. We did not actually construct an example
when this happens, though.

>From the VCS and review history, it seems the optimization was
introduced in the initial version without a mention of any performance
benchmarks justifying the performance gains. This led me to a
conclusion that the optimization was premature, so removing it to avoid
crashes seems like the right trade-off at that point.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65796

Files:
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/IncludeFixer.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -797,6 +797,24 @@
   "Add include \"x.h\" for symbol a::X");
 }
 
+TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {
+  Annotations Test(R"cpp(
+template  struct Templ {
+  template 
+  typename U::type operator=(const U &);
+};
+
+struct A {
+  Templ s;
+  A() { [[a]]; } // this caused crashes if we compute scopes lazily.
+};
+  )cpp");
+
+  auto TU = TestTU::withCode(Test.code());
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
+}
+
 TEST(DiagsInHeaders, DiagInsideHeader) {
   Annotations Main(R"cpp(
 #include [["a.h"]]
Index: clang-tools-extra/clangd/IncludeFixer.h
===
--- clang-tools-extra/clangd/IncludeFixer.h
+++ clang-tools-extra/clangd/IncludeFixer.h
@@ -58,9 +58,7 @@
   struct UnresolvedName {
 std::string Name;   // E.g. "X" in foo::X.
 SourceLocation Loc; // Start location of the unresolved name.
-// Lazily get the possible scopes of the unresolved name. `Sema` must be
-// alive when this is called.
-std::function()> GetScopes;
+std::vector Scopes; // Namespace scopes we should search in.
   };
 
   /// Records the last unresolved name seen by Sema.
Index: clang-tools-extra/clangd/IncludeFixer.cpp
===
--- clang-tools-extra/clangd/IncludeFixer.cpp
+++ clang-tools-extra/clangd/IncludeFixer.cpp
@@ -16,6 +16,7 @@
 #include "index/Symbol.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
@@ -34,6 +35,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
@@ -301,6 +303,24 @@
   return Result;
 }
 
+/// Returns all namespace scopes that the unqualified lookup would visit.
+std::vector
+collectAccessibleScopes(Sema &Sem, const DeclarationNameInfo &Typo, Scope *S,
+Sema::LookupNameKind LookupKind) {
+  std::vector Scopes;
+  VisitedContextCollector Collector;
+  Sem.LookupVisibleDecls(S, LookupKind, Collector,
+ /*IncludeGlobalScope=*/false,
+ /*LoadExternal=*/false);
+
+  Scopes.push_back("");
+  for (const auto *Ctx : Collector.takeVisitedContexts()) {
+if (isa(Ctx))
+  Scopes.push_back(printNamespaceScope(*Ctx));
+  }
+  return Scopes;
+}
+
 class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {
 public:
   UnresolvedNameRecorder(llvm::Optional &LastUnresolvedName)
@@ -321,48 +341,30 @@
 if (!isInsideMainFile(Typo.getLoc(), SemaPtr->SourceMgr))
   return clang::TypoCorrection();
 
-// This is not done lazily because `SS` can get out of scope and it's
-// relatively cheap.
 auto Extracted = extractUnresolvedNameCheaply(
 SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,
 static_cast(LookupKind) ==
 Sema::LookupNameKind::LookupNestedNameSpecifierName);
 if (!Extracted)
   return TypoCorrection();
-auto CheapUnresolved = std::move(*Extracted);
+
 UnresolvedName Unresolved;
-Unresolved.Name = CheapUnresolved.Name;
+Unresolved.Name = Extracted->Name;
 Unresolved.Loc = Typo.getBeginLoc();
-
-

[PATCH] D65577: [ASTImporter] Import default expression of param before creating the param.

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

balazske updated this revision to Diff 213551.
balazske added a comment.

A new kind of solution, the previous does not work always.
Still no test for this new problem case (circular dependency between a 
ParmVarDecl and function of it and a CXXDefaultArgExpr that references to the 
function probably by UsedContext).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65577

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp
  clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  clang/test/Analysis/ctu-main.cpp

Index: clang/test/Analysis/ctu-main.cpp
===
--- clang/test/Analysis/ctu-main.cpp
+++ clang/test/Analysis/ctu-main.cpp
@@ -112,6 +112,8 @@
   clang_analyzer_eval(obj->fvcl(1) == 8);  // expected-warning{{FALSE}} expected-warning{{TRUE}}
 }
 
+extern int testDefParmIncompleteImport(int);
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
@@ -144,4 +146,6 @@
   clang_analyzer_eval(extSubSCN.a == 1); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCC.a == 7); // TODO
   clang_analyzer_eval(extU.a == 4); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(testDefParmIncompleteImport(9) == 9); // expected-warning{{TRUE}}
 }
Index: clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
===
--- clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
+++ clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
@@ -25,3 +25,4 @@
 c:@extSubSCN ctu-other.cpp.ast
 c:@extSCC ctu-other.cpp.ast
 c:@extU ctu-other.cpp.ast
+c:@F@testDefParmIncompleteImport#I# ctu-other.cpp.ast
Index: clang/test/Analysis/Inputs/ctu-other.cpp
===
--- clang/test/Analysis/Inputs/ctu-other.cpp
+++ clang/test/Analysis/Inputs/ctu-other.cpp
@@ -131,3 +131,22 @@
   const unsigned int b;
 };
 U extU = {.a = 4};
+
+struct DefParmContext {
+  static const int I;
+  int f();
+};
+
+int fDefParm(int I = DefParmContext::I) {
+  return I;
+}
+
+int testDefParmIncompleteImport(int I) {
+  return fDefParm(I);
+}
+
+const int DefParmContext::I = 0;
+
+int DefParmContext::f() {
+  return fDefParm();
+}
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -426,6 +426,8 @@
 
 Error ImportFunctionDeclBody(FunctionDecl *FromFD, FunctionDecl *ToFD);
 
+Error ImportDefaultArgOfParmVarDecl(const ParmVarDecl *FromParam, ParmVarDecl *ToParam);
+
 template 
 bool hasSameVisibilityContext(T *Found, T *From);
 
@@ -3824,6 +3826,27 @@
   return ToParm;
 }
 
+Error ASTNodeImporter::ImportDefaultArgOfParmVarDecl(const ParmVarDecl *FromParam, ParmVarDecl *ToParam) {
+  ToParam->setHasInheritedDefaultArg(FromParam->hasInheritedDefaultArg());
+  ToParam->setKNRPromoted(FromParam->isKNRPromoted());
+
+  if (FromParam->hasUninstantiatedDefaultArg()) {
+if (auto ToDefArgOrErr = import(FromParam->getUninstantiatedDefaultArg()))
+  ToParam->setUninstantiatedDefaultArg(*ToDefArgOrErr);
+else
+  return ToDefArgOrErr.takeError();
+  } else if (FromParam->hasUnparsedDefaultArg()) {
+ToParam->setUnparsedDefaultArg();
+  } else if (FromParam->hasDefaultArg()) {
+if (auto ToDefArgOrErr = import(FromParam->getDefaultArg()))
+  ToParam->setDefaultArg(*ToDefArgOrErr);
+else
+  return ToDefArgOrErr.takeError();
+  }
+
+  return Error::success();
+}
+
 ExpectedDecl ASTNodeImporter::VisitParmVarDecl(ParmVarDecl *D) {
   // Parameters are created in the translation unit's context, then moved
   // into the function declaration's context afterward.
@@ -3850,23 +3873,11 @@
   /*DefaultArg*/ nullptr))
 return ToParm;
 
-  // Set the default argument.
-  ToParm->setHasInheritedDefaultArg(D->hasInheritedDefaultArg());
-  ToParm->setKNRPromoted(D->isKNRPromoted());
-
-  if (D->hasUninstantiatedDefaultArg()) {
-if (auto ToDefArgOrErr = import(D->getUninstantiatedDefaultArg()))
-  ToParm->setUninstantiatedDefaultArg(*ToDefArgOrErr);
-else
-  return ToDefArgOrErr.takeError();
-  } else if (D->hasUnparsedDefaultArg()) {
-ToParm->setUnparsedDefaultArg();
-  } else if (D->hasDefaultArg()) {
-if (auto ToDefArgOrErr = import(D->getDefaultArg()))
-  ToParm->setDefaultArg(*ToDefArgOrErr);
-else
-  return ToDefArgOrErr.takeError();
-  }
+  // Set the default argument. It should be no problem if it was already done.
+  // Do not import the default expression before GetImportedOrCreateDecl call
+  // to avoid possible infinite import loop because circular dependency.
+  if (Error Err = ImportDefaultArgOfPar

[PATCH] D65000: [ARM] Set default alignment to 64bits

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

dnsampaio added a comment.

I have tested this in our MacOS and linux environments. @thakis @thegameg 
@phosek, would it be possible for you to check if this works for you?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65000



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

[PATCH] D65000: [ARM] Set default alignment to 64bits

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

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

Fix test


Repository:
  rC Clang

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

https://reviews.llvm.org/D65000

Files:
  lib/Basic/Targets/ARM.cpp
  test/CodeGenCXX/ARM/exception-alignment.cpp
  test/SemaCXX/warn-overaligned-type-thrown.cpp


Index: test/SemaCXX/warn-overaligned-type-thrown.cpp
===
--- test/SemaCXX/warn-overaligned-type-thrown.cpp
+++ test/SemaCXX/warn-overaligned-type-thrown.cpp
@@ -2,11 +2,12 @@
 // RUN: %clang_cc1 -triple arm64-apple-ios10 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos10 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos4 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
+// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions  -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-ios12 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos12 -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos5 -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
-// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
+// RUN: %clang_cc1 -triple arm-linux-androideabi -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple aarch64-linux-gnueabi -verify -fsyntax-only 
-std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mipsel-linux-gnu -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mips64el-linux-gnu -verify -fsyntax-only -std=c++11 
-fcxx-exceptions -fexceptions %s
Index: test/CodeGenCXX/ARM/exception-alignment.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ARM/exception-alignment.cpp
@@ -0,0 +1,21 @@
+// Bug: https://bugs.llvm.org/show_bug.cgi?id=42668
+// REQUIRES: arm-registered-target
+
+// RUN: %clang_cc1 -triple armv8-arm-none-eabi -emit-llvm -target-cpu generic 
-Os -fcxx-exceptions -o - -x c++ %s | FileCheck --check-prefixes=CHECK,A8 %s
+// RUN: %clang_cc1 -triple armv8-unknown-linux-android -emit-llvm -target-cpu 
generic -Os -fcxx-exceptions -o - -x c++ %s | FileCheck 
--check-prefixes=CHECK,A16 %s
+
+// CHECK: [[E:%[A-z0-9]+]] = tail call i8* @__cxa_allocate_exception
+// CHECK-NEXT: [[BC:%[A-z0-9]+]] = bitcast i8* [[E]] to <2 x i64>*
+// A8-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 8
+// A16-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 16
+#include 
+
+int main(void) {
+  try {
+throw vld1q_u64(((const uint64_t[2]){1, 2}));
+  } catch (uint64x2_t exc) {
+return 0;
+  }
+  return 1;
+}
+
Index: lib/Basic/Targets/ARM.cpp
===
--- lib/Basic/Targets/ARM.cpp
+++ lib/Basic/Targets/ARM.cpp
@@ -309,8 +309,9 @@
   setAtomic();
 
   // Maximum alignment for ARM NEON data types should be 64-bits (AAPCS)
+  // as well the default alignment
   if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))
-MaxVectorAlign = 64;
+DefaultAlignForAttributeAligned = MaxVectorAlign = 64;
 
   // Do force alignment of members that follow zero length bitfields.  If
   // the alignment of the zero-length bitfield is greater than the member


Index: test/SemaCXX/warn-overaligned-type-thrown.cpp
===
--- test/SemaCXX/warn-overaligned-type-thrown.cpp
+++ test/SemaCXX/warn-overaligned-type-thrown.cpp
@@ -2,11 +2,12 @@
 // RUN: %clang_cc1 -triple arm64-apple-ios10 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos10 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos4 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
+// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions  -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-ios12 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos12 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos5 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
-// RU

[PATCH] D65000: [ARM] Set default alignment to 64bits

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

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

Hi, first thanks for those that looked into this and sorry for the delay.
We have investigated the errors and seems that the test was, first in the wrong 
folder, inside CodeGen where it should be in CodeGenCXX and we should use 
clang_cc1.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65000



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

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:6
+
+export namespace SemanticHighlighting {
+interface ScopeColorRule {

I think we should not use the namespace in typescript. The `namespace` in TS 
refers to [“Internal 
modules”](https://www.typescriptlang.org/docs/handbook/namespaces.html). 

Each TS file is a separate module, so semantically namespace your code, use 
separate files. For semantic highlighting feature, I think we put all into one 
file `semantic-highlighting.ts`. 



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:7
+export namespace SemanticHighlighting {
+interface ScopeColorRule {
+  scope: string, textColor: string,

TokenColorRule seems a bit clearer to me.



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:24
+ */
+export function themesToScopeColors(themeContents: any[]): ScopeColorRule[] {
+  const ruleMap: Map = new Map();

we are introducing 2~3 helper functions here, I think we could simplify it like 
below.

```
function loadTheme(themeName) : Promise {
   // iterate all extensions and find the file path of the theme
   // call parseThemeFile().
}

function parseThemeFile(themeFilePath) : Promise {
  // read the file and parse the file content (recusively)
}
```

And for the test, we could save a small theme file in the test directory, and 
pass the file path to the `parseThemeFile` function.




Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:75
+const contents = await readFileText(fullPath);
+// FIXME: Add support for themes written as .thTheme.
+const parsed = jsonc.parse(contents);

nit: we should filter the `.tmTheme` out in the code (just checking the 
extension of the fullpath).



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:93
+function readFileText(path: string): Promise {
+  return new Promise((res, rej) => {
+fs.readFile(path, 'utf8', (err, data) => {

nit: use the full name for the parameters, `resolve`, `reject`.



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:96
+  if (err) {
+rej(err);
+  }

if we have err, we will run both `rej` and `res`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738



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

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

[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/unittests/AST/RecursiveASTVisitorTest.cpp:107
+}
\ No newline at end of file


Please add a newline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64907



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

Re: r367906 - [DirectoryWatcher][linux] Fix build for older kernels

[Hans Wennborg via cfe-commits]

Merged to release_90 in r367995.

On Mon, Aug 5, 2019 at 8:43 PM Jan Korous via cfe-commits
 wrote:
>
> Author: jkorous
> Date: Mon Aug  5 11:44:07 2019
> New Revision: 367906
>
> URL: http://llvm.org/viewvc/llvm-project?rev=367906&view=rev
> Log:
> [DirectoryWatcher][linux] Fix build for older kernels
>
> Apparently kernel support for IN_EXCL_UNLINK in inotify_add_watch() doesn't 
> imply it's defined in sys/inotify.h.
>
> https://bugs.llvm.org/show_bug.cgi?id=42824
>
> Modified:
> cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
>
> Modified: cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp?rev=367906&r1=367905&r2=367906&view=diff
> ==
> --- cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp (original)
> +++ cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp Mon Aug  
> 5 11:44:07 2019
> @@ -24,7 +24,6 @@
>  #include 
>
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -336,7 +335,7 @@ std::unique_ptr clang:
>InotifyFD, Path.str().c_str(),
>IN_CREATE | IN_DELETE | IN_DELETE_SELF | IN_MODIFY |
>IN_MOVED_FROM | IN_MOVE_SELF | IN_MOVED_TO | IN_ONLYDIR | IN_IGNORED
> -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,36)
> +#ifdef IN_EXCL_UNLINK
>| IN_EXCL_UNLINK
>  #endif
>);
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r367675 - [OpenCL] Allow OpenCL C style vector initialization in C++

[Hans Wennborg via cfe-commits]

Merged both this and the follow-up to release_90 in r367987.

On Mon, Aug 5, 2019 at 1:16 PM Anastasia Stulova via cfe-commits
 wrote:
>
> Hi Yvan,
>
>
> Sorry for this, it should now be fixed  in r367823.
>
>
> Thanks,
>
> Anastasia
>
>
>
> 
> From: Yvan Roux 
> Sent: 02 August 2019 14:09
> To: Anastasia Stulova 
> Cc: cfe-commits 
> Subject: Re: r367675 - [OpenCL] Allow OpenCL C style vector initialization in 
> C++
>
> Hi Anastasia,
>
> This commit broke ARMv8 bots, logs are available here:
>
> http://lab.llvm.org:8011/builders/clang-cmake-armv8-quick/builds/14655/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avector_literals_valid.cl
>
> Thanks,
> Yvan
>
> On Fri, 2 Aug 2019 at 13:18, Anastasia Stulova via cfe-commits
>  wrote:
> >
> > Author: stulova
> > Date: Fri Aug  2 04:19:35 2019
> > New Revision: 367675
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=367675&view=rev
> > Log:
> > [OpenCL] Allow OpenCL C style vector initialization in C++
> >
> > Allow creating vector literals from other vectors.
> >
> >  float4 a = (float4)(1.0f, 2.0f, 3.0f, 4.0f);
> >  float4 v = (float4)(a.s23, a.s01);
> >
> > Differential revision: https://reviews.llvm.org/D65286
> >
> >
> > Removed:
> > cfe/trunk/test/CodeGenOpenCL/vector_literals_nested.cl
> > cfe/trunk/test/SemaOpenCL/vector_literals_const.cl
> > Modified:
> > cfe/trunk/lib/Sema/SemaInit.cpp
> > cfe/trunk/test/CodeGenOpenCL/vector_literals_valid.cl
> > cfe/trunk/test/SemaCXX/vector.cpp
> >
> > Modified: cfe/trunk/lib/Sema/SemaInit.cpp
> > URL: 
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=367675&r1=367674&r2=367675&view=diff
> > ==
> > --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
> > +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  2 04:19:35 2019
> > @@ -1289,7 +1289,16 @@ void InitListChecker::CheckSubElementTyp
> >  // FIXME: Better EqualLoc?
> >  InitializationKind Kind =
> >  InitializationKind::CreateCopy(expr->getBeginLoc(), 
> > SourceLocation());
> > -InitializationSequence Seq(SemaRef, Entity, Kind, expr,
> > +
> > +// Vector elements can be initialized from other vectors in which case
> > +// we need initialization entity with a type of a vector (and not a 
> > vector
> > +// element!) initializing multiple vector elements.
> > +auto TmpEntity =
> > +(ElemType->isExtVectorType() && 
> > !Entity.getType()->isExtVectorType())
> > +? InitializedEntity::InitializeTemporary(ElemType)
> > +: Entity;
> > +
> > +InitializationSequence Seq(SemaRef, TmpEntity, Kind, expr,
> > /*TopLevelOfInitList*/ true);
> >
> >  // C++14 [dcl.init.aggr]p13:
> > @@ -1300,8 +1309,7 @@ void InitListChecker::CheckSubElementTyp
> >  // assignment-expression.
> >  if (Seq || isa(expr)) {
> >if (!VerifyOnly) {
> > -ExprResult Result =
> > -  Seq.Perform(SemaRef, Entity, Kind, expr);
> > +ExprResult Result = Seq.Perform(SemaRef, TmpEntity, Kind, expr);
> >  if (Result.isInvalid())
> >hadError = true;
> >
> >
> > Removed: cfe/trunk/test/CodeGenOpenCL/vector_literals_nested.cl
> > URL: 
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenOpenCL/vector_literals_nested.cl?rev=367674&view=auto
> > ==
> > --- cfe/trunk/test/CodeGenOpenCL/vector_literals_nested.cl (original)
> > +++ cfe/trunk/test/CodeGenOpenCL/vector_literals_nested.cl (removed)
> > @@ -1,23 +0,0 @@
> > -// RUN: %clang_cc1 %s -emit-llvm -O3 -o - | FileCheck %s
> > -
> > -typedef int int2 __attribute((ext_vector_type(2)));
> > -typedef int int4 __attribute((ext_vector_type(4)));
> > -
> > -__constant const int4 itest1 = (int4)(1, 2, ((int2)(3, 4)));
> > -// CHECK: constant <4 x i32> 
> > -__constant const int4 itest2 = (int4)(1, 2, ((int2)(3)));
> > -// CHECK: constant <4 x i32> 
> > -
> > -typedef float float2 __attribute((ext_vector_type(2)));
> > -typedef float float4 __attribute((ext_vector_type(4)));
> > -
> > -void ftest1(float4 *p) {
> > -  *p = (float4)(1.1f, 1.2f, ((float2)(1.3f, 1.4f)));
> > -// CHECK: store <4 x float>  > 0x3FF34000, float 0x3FF4C000, float 0x3FF66000>
> > -}
> > -
> > -float4 ftest2(float4 *p) {
> > -   *p =  (float4)(1.1f, 1.2f, ((float2)(1.3f)));
> > -// CHECK: store <4 x float>  > 0x3FF34000, float 0x3FF4C000, float 0x3FF4C000>
> > -}
> > -
> >
> > Modified: cfe/trunk/test/CodeGenOpenCL/vector_literals_valid.cl
> > URL: 
> > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenOpenCL/vector_literals_valid.cl?rev=367675&r1=367674&r2=367675&view=diff
> > ==
> > --- cfe/trunk/test/CodeGenOpenCL/vector_literals_valid.cl (original)
> > +++ cfe/trunk/test/

[PATCH] D64696: Adds a warning when an inline Doxygen comment has no argument

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added a comment.

From the Swift language source (http://github.com/apple/swift):

include/swift/AST/Attr.h:/// A limited variant of \c @objc that's used for 
classes with generic ancestry.
include/swift/AST/Decl.h:  /// \c @usableFromInline attribute are treated as 
public. This is normally
lib/Sema/LookupVisibleDecls.cpp:/// If \c baseType is \c @dynamicMemberLookup, 
this looks up any keypath


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64696



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

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

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

jvikstrom added a comment.

In D65738#1614897 , @hokein wrote:

> Haven't looked at the patch in details.
>
> Looks like the patch is doing different things, and the test just covers a 
> small set of the code.
>
> 1. find and parse the theme definition files `json` ;
> 2. define related data structures to save the TM scopes and do the 
> transformation;
> 3. handle changes of the theme;
>
>   could we narrow it further? I think we could just implement 1) for this 
> patch.


Removed everything that wasn't loading a TM theme. Also added  an interface for 
a TM scope that we parse into (which I also added a test for).
The getFullNamedTheme function is not used (or exported) anywhere for now 
though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738



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

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

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

jvikstrom updated this revision to Diff 213528.
jvikstrom marked 2 inline comments as done.
jvikstrom added a comment.

Narrowed CL down to loading/parsing a text mate theme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
  clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts
@@ -0,0 +1,27 @@
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';
+
+import {SemanticHighlighting} from '../src/TextMate';
+
+suite('TextMate Tests', () => {
+  test('Parses arrays of textmate themes.', () => {
+const themes = [
+  {}, {
+tokenColors : [
+  {}, {scope : 'a'},
+  {scope : [ 'b', 'c', 'd' ], settings : {background : '#000'}},
+  {scope : 'a', settings : {foreground : '#000'}},
+  {scope : [ 'a', 'b', 'c' ], settings : {foreground : '#fff'}}
+]
+  },
+  {tokenColors : [ {scope : 'b', settings : {foreground : '#ff'}} ]}
+];
+const scopeColorRules = SemanticHighlighting.themesToScopeColors(themes);
+const getScopeRule = (scope: string) =>
+scopeColorRules.find((v) => v.scope === scope);
+assert.equal(scopeColorRules.length, 3);
+assert.deepEqual(getScopeRule('a'), {scope : 'a', textColor : '#fff'});
+assert.deepEqual(getScopeRule('b'), {scope : 'b', textColor : '#ff'});
+assert.deepEqual(getScopeRule('c'), {scope : 'c', textColor : '#fff'});
+  });
+});
\ No newline at end of file
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
@@ -0,0 +1,102 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+export namespace SemanticHighlighting {
+interface ScopeColorRule {
+  scope: string, textColor: string,
+}
+
+async function getNamedThemeScopeColors(themeName: string):
+Promise {
+  const contents = await getFullNamedTheme(themeName);
+  return themesToScopeColors(contents);
+}
+
+/**
+ * Convert an array of TextMate theme contents into the an array of all scopes'
+ * colors. If there are duplicate entries of a scope only the latest occurence
+ * of the scope is kept.
+ * @param themeContents An entry in this array is either the theme's (or an
+ * included theme's) TextMate definition.
+ */
+export function themesToScopeColors(themeContents: any[]): ScopeColorRule[] {
+  const ruleMap: Map = new Map();
+  themeContents.forEach((content) => {
+if (!content.tokenColors)
+  return;
+content.tokenColors.forEach((rule: any) => {
+  if (!rule.scope || !rule.settings || !rule.settings.foreground)
+return;
+  const textColor = rule.settings.foreground;
+  if (rule.scope instanceof Array) {
+rule.scope.forEach((s: string) => ruleMap.set(s, textColor));
+return;
+  }
+  ruleMap.set(rule.scope, textColor);
+});
+  });
+
+  return Array.from(ruleMap.entries())
+  .map(([ scope, textColor ]) => ({scope, textColor}));
+}
+
+// Gets a TextMate theme by its name and all its included themes.
+async function getFullNamedTheme(themeName: string): Promise {
+  const extension =
+  vscode.extensions.all.find((extension: vscode.Extension) => {
+const contribs = extension.packageJSON.contributes;
+if (!contribs || !contribs.themes)
+  return false;
+return contribs.themes.some((theme: any) => theme.id === themeName ||
+theme.label === themeName);
+  });
+
+  if (!extension) {
+return Promise.reject('Could not find a theme with name: ' + themeName);
+  }
+
+  const extensionInfo = extension.packageJSON.contributes.themes.find(
+  (theme: any) => theme.id === themeName || theme.label === themeName);
+  return recursiveGetTextMateGrammarPath(
+  path.join(extension.extensionPath, extensionInfo.path));
+}
+
+// TM grammars can include other TM grammars, this function recursively gets all
+// of them.
+async function recursiveGetTextMateGrammarPath(fullPath: string):
+Promise {
+  // If there is an error opening a file, the TM files that were correctly found
+  // and parsed further up the chain should be returned. Otherwise there will be
+  // no highlightings at all.
+  try {
+const contents = await readFileText(fullPath);
+// FIXME: Add support for themes written as .thTheme.
+const parsed = 

Re: r367979 - [clang][DirectoryWatcher] Adding llvm::Expected error handling to create.

[Puyan Lotfi via cfe-commits]

Thank you! I had forgot that cmake decides which cpp file to build with for 
DirectoryWatcher depending on the os you’re building on :-(

PL

Sent from ProtonMail Mobile

On Tue, Aug 6, 2019 at 12:14 AM, Shoaib Meenai  wrote:

> You missed a semicolon after an assert, which broke asserts Mac builds. I 
> fixed it in r367984.
>
> From: cfe-commits  on behalf of Puyan 
> Lotfi via cfe-commits 
> Reply-To: Puyan Lotfi 
> Date: Monday, August 5, 2019 at 10:11 PM
> To: "cfe-commits@lists.llvm.org" 
> Subject: r367979 - [clang][DirectoryWatcher] Adding llvm::Expected error 
> handling to create.
>
> Author: zer0
>
> Date: Mon Aug  5 22:12:23 2019
>
> New Revision: 367979
>
> URL: 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D367979-26view-3Drev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=GdHgTwHdRXipmG75D6eyJGuUUbCL0EMIl0YPYAY0y2U&e=
>
> Log:
>
> [clang][DirectoryWatcher] Adding llvm::Expected error handling to create.
>
> Prior to this patch Unix style errno error reporting from the inotify layer 
> was
>
> used by DirectoryWatcher::create to simply return a nullptr on error. This
>
> would generally be ok, except that in LLVM we have much more robust error
>
> reporting through the facilities of llvm::Expected.
>
> The other critical thing I stumbled across was that the unit tests for
>
> DirectoryWatcher were not failing abruptly when inotify_init() was reporting 
> an
>
> error, but would continue with the testing and eventually hit a deadlock in a
>
> pathological machine state (ie in the unit test, the return nullptr on 
> ::create
>
> was ignored).
>
> Generally this pathological state never happens on any build bot, so it is
>
> totally understandable that it was overlooked, but on a Linux desktop running
>
> a dubious desktop environment (which I will not name) there is a chance that
>
> said desktop environment could use up enough inotify instances to exceed the
>
> user's limit. These are the conditions that led me to hit the deadlock I am
>
> addressing in this patch with more robust error handling.
>
> With the new llvm::Expected error handling when your system runs out of 
> inotify
>
> instances for your user, the unit test will be forced to handle the error or
>
> crash and report the issue to the user instead of weirdly deadlocking on a
>
> condition variable wait.
>
> Differential Revision: 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D65704&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=vGtUV_-i1W7ejLSH8cWFnYlusAJMA8EK5kMTvuMmQB4&e=
>
> Modified:
>
> cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
>
> 
> cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
>
> cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
>
> cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
>
> cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp
>
> Modified: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
>
> URL: 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_include_clang_DirectoryWatcher_DirectoryWatcher.h-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=VCfJ3mXkHYvO_8Iw35iGLiTPxX-OaQIrX9fUY22ukbw&e=
>
> ==
>
> --- cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h (original)
>
> +++ cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h Mon Aug  5 
> 22:12:23 2019
>
> @@ -11,6 +11,7 @@
>
> #include "llvm/ADT/ArrayRef.h"
>
> #include "llvm/ADT/StringRef.h"
>
> +#include "llvm/Support/Error.h"
>
> #include 
>
> #include 
>
> #include 
>
> @@ -98,10 +99,11 @@ public:
>
>  : Kind(Kind), Filename(Filename) {}
>
>};
>
> -  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
>
> -  /// Returns nullptr if OS kernel API told us we can't start watching. In 
> such
>
> -  /// case it's unclear whether just retrying has any chance to succeeed.
>
> -  static std::unique_ptr
>
> +  /// Asserts if \param Path doesn't exist or isn't a directory.
>
> +  /// Returns llvm::Expected Error if OS kernel API told us we can't start
>
> +  /// watching. In such case it's unclear whether just retrying has any 
> chance
>
> +  /// to succeeed.
>
> +  static llvm::Expected>
>
>create(llvm::StringRef Path,
>
>   std::function Events,
>
>  bool IsInitial)>
>
> Modified: 
> cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
>
> URL: 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_DirectoryWatcher_default_DirectoryWatcher-2Dnot-2Dimplemented.cpp-3Frev-3D367979

Re: r367979 - [clang][DirectoryWatcher] Adding llvm::Expected error handling to create.

[Shoaib Meenai via cfe-commits]

You missed a semicolon after an assert, which broke asserts Mac builds. I fixed 
it in r367984.

From: cfe-commits  on behalf of Puyan Lotfi 
via cfe-commits 
Reply-To: Puyan Lotfi 
Date: Monday, August 5, 2019 at 10:11 PM
To: "cfe-commits@lists.llvm.org" 
Subject: r367979 - [clang][DirectoryWatcher] Adding llvm::Expected error 
handling to create.

Author: zer0
Date: Mon Aug  5 22:12:23 2019
New Revision: 367979

URL: 
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D367979-26view-3Drev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=GdHgTwHdRXipmG75D6eyJGuUUbCL0EMIl0YPYAY0y2U&e=
Log:
[clang][DirectoryWatcher] Adding llvm::Expected error handling to create.

Prior to this patch Unix style errno error reporting from the inotify layer was
used by DirectoryWatcher::create to simply return a nullptr on error. This
would generally be ok, except that in LLVM we have much more robust error
reporting through the facilities of llvm::Expected.

The other critical thing I stumbled across was that the unit tests for
DirectoryWatcher were not failing abruptly when inotify_init() was reporting an
error, but would continue with the testing and eventually hit a deadlock in a
pathological machine state (ie in the unit test, the return nullptr on ::create
was ignored).

Generally this pathological state never happens on any build bot, so it is
totally understandable that it was overlooked, but on a Linux desktop running
a dubious desktop environment (which I will not name) there is a chance that
said desktop environment could use up enough inotify instances to exceed the
user's limit. These are the conditions that led me to hit the deadlock I am
addressing in this patch with more robust error handling.

With the new llvm::Expected error handling when your system runs out of inotify
instances for your user, the unit test will be forced to handle the error or
crash and report the issue to the user instead of weirdly deadlocking on a
condition variable wait.

Differential Revision: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D65704&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=vGtUV_-i1W7ejLSH8cWFnYlusAJMA8EK5kMTvuMmQB4&e=


Modified:
cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

Modified: cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h
URL: 
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_include_clang_DirectoryWatcher_DirectoryWatcher.h-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=VCfJ3mXkHYvO_8Iw35iGLiTPxX-OaQIrX9fUY22ukbw&e=
==
--- cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h (original)
+++ cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h Mon Aug  5 
22:12:23 2019
@@ -11,6 +11,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
#include 
#include 
#include 
@@ -98,10 +99,11 @@ public:
 : Kind(Kind), Filename(Filename) {}
   };
-  /// Returns nullptr if \param Path doesn't exist or isn't a directory.
-  /// Returns nullptr if OS kernel API told us we can't start watching. In such
-  /// case it's unclear whether just retrying has any chance to succeeed.
-  static std::unique_ptr
+  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// Returns llvm::Expected Error if OS kernel API told us we can't start
+  /// watching. In such case it's unclear whether just retrying has any chance
+  /// to succeeed.
+  static llvm::Expected>
   create(llvm::StringRef Path,
  std::function Events,
 bool IsInitial)>

Modified: 
cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp
URL: 
https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_DirectoryWatcher_default_DirectoryWatcher-2Dnot-2Dimplemented.cpp-3Frev-3D367979-26r1-3D367978-26r2-3D367979-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=6k2w1Mw8wKFqzKnFgq4r1I7Zmx7B-N-JdVMvtHYpjJs&s=kOGSMivkTUI-mqfkDLYmSGfQM8CSoTUydr4qZdtcaAc&e=
==
--- cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp 
(original)
+++ cfe/trunk/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp 
Mon Aug  5 22:12:23 2019
@@ -11,9 +11,11 @@
using namespace llv

r367984 - [DirectoryWatcher] Fix asserts Mac builds

[Shoaib Meenai via cfe-commits]

Author: smeenai
Date: Tue Aug  6 00:13:53 2019
New Revision: 367984

URL: http://llvm.org/viewvc/llvm-project?rev=367984&view=rev
Log:
[DirectoryWatcher] Fix asserts Mac builds

Add a missing semicolon after an assert. Remove the period from the
assert message while I'm here, because we don't usually have those.

Modified:
cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp

Modified: cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp?rev=367984&r1=367983&r2=367984&view=diff
==
--- cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp (original)
+++ cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp Tue Aug  6 
00:13:53 2019
@@ -210,7 +210,7 @@ llvm::Expected Result =
   llvm::make_unique(EventStream, Receiver, Path);


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