[PATCH] D56563: [clang-tidy] add options documentation to readability-identifier-naming checker

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 181497.
MyDeveloperDay added a comment.

Fix review comments

s/the checker will check/the check will ensure/


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

https://reviews.llvm.org/D56563

Files:
  docs/clang-tidy/checks/readability-identifier-naming.rst

Index: docs/clang-tidy/checks/readability-identifier-naming.rst
===
--- docs/clang-tidy/checks/readability-identifier-naming.rst
+++ docs/clang-tidy/checks/readability-identifier-naming.rst
@@ -5,14 +5,1867 @@
 
 Checks for identifiers naming style mismatch.
 
-This check will try to enforce coding guidelines on the identifiers naming.
-It supports `lower_case`, `UPPER_CASE`, `camelBack` and `CamelCase` casing and
-tries to convert from one to another if a mismatch is detected.
+This check will try to enforce coding guidelines on the identifiers naming. It
+supports one the following casing types and tries to convert from one to
+another if a mismatch is detected
 
-It also supports a fixed prefix and suffix that will be prepended or
-appended to the identifiers, regardless of the casing.
+Casing types inclde:
+
+ - ``lower_case``,
+ - ``UPPER_CASE``,
+ - ``camelBack``,
+ - ``CamelCase``,
+ - ``camel_Snake_Back``,
+ - ``Camel_Snake_Case``,
+ - ``aNy_CasE``.
+
+It also supports a fixed prefix and suffix that will be prepended or appended
+to the identifiers, regardless of the casing.
 
 Many configuration options are available, in order to be able to create
-different rules for different kind of identifier. In general, the
-rules are falling back to a more generic rule if the specific case is not
-configured.
+different rules for different kinds of identifier. In general, the rules are
+falling back to a more generic rule if the specific case is not configured.
+
+Options
+---
+
+.. option:: AbstractClassCase
+
+When defined, the check will ensure abstract class names conform to the
+selected casing.
+
+.. option:: AbstractClassPrefix
+
+When defined, the check will ensure abstract class names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: AbstractClassSuffix
+
+When defined, the check will ensure abstract class names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - AbstractClassCase of ``lower_case``
+   - AbstractClassPrefix of ``pre_``
+   - AbstractClassSuffix of ``_post``
+
+Identifies and/or transforms abstract class names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class ABSTRACT_CLASS {
+public:
+  ABSTRACT_CLASS();
+};
+
+After:
+
+.. code-block:: c++
+
+class abstract_class {
+public:
+  pre_abstract_class_post();
+};
+
+.. option:: ClassCase
+
+When defined, the check will ensure class names conform to the
+selected casing.
+
+.. option:: ClassPrefix
+
+When defined, the check will ensure class names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassSuffix
+
+When defined, the check will ensure class names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - ClassCase of ``lower_case``
+   - ClassPrefix of ``pre_``
+   - ClassSuffix of ``_post``
+
+Identifies and/or transforms class names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  FOO();
+  ~FOO();
+};
+
+After:
+
+.. code-block:: c++
+
+class pre_foo_post {
+public:
+  pre_foo_post();
+  ~pre_foo_post();
+};
+
+.. option:: ClassConstantCase
+
+When defined, the check will ensure class constant names conform to the
+selected casing.
+
+.. option:: ClassConstantPrefix
+
+When defined, the check will ensure class constant names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassConstantSuffix
+
+When defined, the check will ensure class constant names will add the
+suffix with the given value (regardless of casing).
+
+For example using values of:
+
+   - ClassConstantCase of ``lower_case``
+   - ClassConstantPrefix of ``pre_``
+   - ClassConstantSuffix of ``_post``
+
+Identifies and/or transforms class constant names as follows:
+
+Before:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  static const int CLASS_CONSTANT;
+};
+
+After:
+
+.. code-block:: c++
+
+class FOO {
+public:
+  static const int pre_class_constant_post;
+};
+
+.. option:: ClassMemberCase
+
+When defined, the check will ensure class member names conform to the
+selected casing.
+
+.. option:: ClassMemberPrefix
+
+When defined, the check will ensure class member names will add the
+prefixed with the given value (regardless of casing).
+
+.. option:: ClassMemberSuffix
+
+When defined, the check will ensure class member names will add the
+suffix wi

[PATCH] D56066: [OpenCL] Address space for default class members

[John McCall via Phabricator via cfe-commits]

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

Great, LGTM.


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

https://reviews.llvm.org/D56066



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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

It's pretty unfortunate that all these fields have to be individually called 
out like this.  Can you move all these basic layout fields into a separate 
`struct` (which can be a secondary base class of `TargetInfo`) which can then 
just be normally copied?  Anything that needs special copy semantics, like the 
LLVM `DataLayout` (do you need to copy this?) doesn't need to go into that 
struct, just the basic POD things that determine fundamental type layouts and 
semantics.


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

https://reviews.llvm.org/D56318



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

[PATCH] D56429: fix python3 compability issue

[serge via Phabricator via cfe-commits]

serge-sans-paille added inline comments.



Comment at: bindings/python/clang/cindex.py:2998
+for i,(name,contents) in enumerate(unsaved_files):
+if hasattr(contents, "read"):
+contents = contents.read()

serge-sans-paille wrote:
> Why did you remove the FIXME comment?
@roxma LGTM except this FIXME removal.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56429



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

r351036 - [X86] Remove mask parameter from avx512 pmultishiftqb intrinsics. Use select in IR instead.

[Craig Topper via cfe-commits]

Author: ctopper
Date: Mon Jan 14 00:46:51 2019
New Revision: 351036

URL: http://llvm.org/viewvc/llvm-project?rev=351036&view=rev
Log:
[X86] Remove mask parameter from avx512 pmultishiftqb intrinsics. Use select in 
IR instead.

Fixes PR40259

Modified:
cfe/trunk/include/clang/Basic/BuiltinsX86.def
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/Headers/avx512vbmiintrin.h
cfe/trunk/lib/Headers/avx512vbmivlintrin.h
cfe/trunk/test/CodeGen/avx512vbmi-builtins.c
cfe/trunk/test/CodeGen/avx512vbmivl-builtin.c

Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/BuiltinsX86.def?rev=351036&r1=351035&r2=351036&view=diff
==
--- cfe/trunk/include/clang/Basic/BuiltinsX86.def (original)
+++ cfe/trunk/include/clang/Basic/BuiltinsX86.def Mon Jan 14 00:46:51 2019
@@ -1821,9 +1821,9 @@ TARGET_BUILTIN(__builtin_ia32_cvtsd2ss_r
 TARGET_BUILTIN(__builtin_ia32_cvtsi2ss32, "V4fV4fiIi", "ncV:128:", "avx512f")
 TARGET_BUILTIN(__builtin_ia32_cvtss2sd_round_mask, "V2dV2dV4fV2dUcIi", 
"ncV:128:", "avx512f")
 TARGET_BUILTIN(__builtin_ia32_cvtusi2ss32, "V4fV4fUiIi", "ncV:128:", "avx512f")
-TARGET_BUILTIN(__builtin_ia32_vpmultishiftqb512_mask, "V64cV64cV64cV64cULLi", 
"ncV:512:", "avx512vbmi")
-TARGET_BUILTIN(__builtin_ia32_vpmultishiftqb128_mask, "V16cV16cV16cV16cUs", 
"ncV:128:", "avx512vbmi,avx512vl")
-TARGET_BUILTIN(__builtin_ia32_vpmultishiftqb256_mask, "V32cV32cV32cV32cUi", 
"ncV:256:", "avx512vbmi,avx512vl")
+TARGET_BUILTIN(__builtin_ia32_vpmultishiftqb512, "V64cV64cV64c", "ncV:512:", 
"avx512vbmi")
+TARGET_BUILTIN(__builtin_ia32_vpmultishiftqb128, "V16cV16cV16c", "ncV:128:", 
"avx512vbmi,avx512vl")
+TARGET_BUILTIN(__builtin_ia32_vpmultishiftqb256, "V32cV32cV32c", "ncV:256:", 
"avx512vbmi,avx512vl")
 
 // generic select intrinsics
 TARGET_BUILTIN(__builtin_ia32_selectb_128, "V16cUsV16cV16c", "ncV:128:", 
"avx512bw,avx512vl")

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=351036&r1=351035&r2=351036&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Mon Jan 14 00:46:51 2019
@@ -11152,6 +11152,26 @@ Value *CodeGenFunction::EmitX86BuiltinEx
 return EmitX86MaskedCompareResult(*this, Fpclass, NumElts, MaskIn);
   }
 
+  case X86::BI__builtin_ia32_vpmultishiftqb128:
+  case X86::BI__builtin_ia32_vpmultishiftqb256:
+  case X86::BI__builtin_ia32_vpmultishiftqb512: {
+Intrinsic::ID ID;
+switch (BuiltinID) {
+default: llvm_unreachable("Unsupported intrinsic!");
+case X86::BI__builtin_ia32_vpmultishiftqb128:
+  ID = Intrinsic::x86_avx512_pmultishift_qb_128;
+  break;
+case X86::BI__builtin_ia32_vpmultishiftqb256:
+  ID = Intrinsic::x86_avx512_pmultishift_qb_256;
+  break;
+case X86::BI__builtin_ia32_vpmultishiftqb512:
+  ID = Intrinsic::x86_avx512_pmultishift_qb_512;
+  break;
+}
+
+return Builder.CreateCall(CGM.getIntrinsic(ID), Ops);
+  }
+
   case X86::BI__builtin_ia32_vpshufbitqmb128_mask:
   case X86::BI__builtin_ia32_vpshufbitqmb256_mask:
   case X86::BI__builtin_ia32_vpshufbitqmb512_mask: {
@@ -11173,8 +11193,8 @@ Value *CodeGenFunction::EmitX86BuiltinEx
   break;
 }
 
-Value *Fpclass = Builder.CreateCall(CGM.getIntrinsic(ID), Ops);
-return EmitX86MaskedCompareResult(*this, Fpclass, NumElts, MaskIn);
+Value *Shufbit = Builder.CreateCall(CGM.getIntrinsic(ID), Ops);
+return EmitX86MaskedCompareResult(*this, Shufbit, NumElts, MaskIn);
   }
 
   // packed comparison intrinsics

Modified: cfe/trunk/lib/Headers/avx512vbmiintrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/avx512vbmiintrin.h?rev=351036&r1=351035&r2=351036&view=diff
==
--- cfe/trunk/lib/Headers/avx512vbmiintrin.h (original)
+++ cfe/trunk/lib/Headers/avx512vbmiintrin.h Mon Jan 14 00:46:51 2019
@@ -91,30 +91,26 @@ _mm512_mask_permutexvar_epi8 (__m512i __
 }
 
 static __inline__ __m512i __DEFAULT_FN_ATTRS
-_mm512_mask_multishift_epi64_epi8 (__m512i __W, __mmask64 __M, __m512i __X, 
__m512i __Y)
+_mm512_multishift_epi64_epi8(__m512i __X, __m512i __Y)
 {
-  return (__m512i) __builtin_ia32_vpmultishiftqb512_mask ((__v64qi) __X,
-(__v64qi) __Y,
-(__v64qi) __W,
-(__mmask64) __M);
+  return (__m512i)__builtin_ia32_vpmultishiftqb512((__v64qi)__X, (__v64qi) 
__Y);
 }
 
 static __inline__ __m512i __DEFAULT_FN_ATTRS
-_mm512_maskz_multishift_epi64_epi8 (__mmask64 __M, __m512i __X, __m512i __Y)
+_mm512_mask_multishift_epi64_epi8(__m512i __W, __mmask64 __M, __m512i __X,
+  __m512i __Y)
 {
-  return (__m512i) __builtin_ia32_vpmultishiftqb512_mask ((__v64

[PATCH] D55782: Fix isInSystemMacro to handle pasted token

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 181498.
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

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

https://reviews.llvm.org/D55782

Files:
  include/clang/Basic/SourceManager.h
  test/Misc/no-warn-in-system-macro.c
  test/Misc/no-warn-in-system-macro.c.inc


Index: test/Misc/no-warn-in-system-macro.c.inc
===
--- /dev/null
+++ test/Misc/no-warn-in-system-macro.c.inc
@@ -0,0 +1,9 @@
+extern int __isnanf(float f);
+extern int __isnan(double f);
+extern int __isnanl(long double f);
+#define isnan(x) \
+   (sizeof (x) == sizeof (float)\
+   ? __isnanf (x)\
+   : sizeof (x) == sizeof (double)   \
+   ? __isnan (x) : __isnanl (x))
+
Index: test/Misc/no-warn-in-system-macro.c
===
--- /dev/null
+++ test/Misc/no-warn-in-system-macro.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -isystem %S -Wdouble-promotion -fsyntax-only %s  2>&1 | 
FileCheck -allow-empty %s
+// CHECK-NOT: warning:
+
+#include 
+
+int main(void)
+{
+   double foo = 1.0;
+
+   if (isnan(foo))
+   return 1;
+   return 0;
+}
Index: include/clang/Basic/SourceManager.h
===
--- include/clang/Basic/SourceManager.h
+++ include/clang/Basic/SourceManager.h
@@ -1453,7 +1453,11 @@
 
   /// Returns whether \p Loc is expanded from a macro in a system header.
   bool isInSystemMacro(SourceLocation loc) const {
-return loc.isMacroID() && isInSystemHeader(getSpellingLoc(loc));
+return loc.isMacroID() &&
+   (isInSystemHeader(getSpellingLoc(loc)) ||
+   // This happens when the macro is the result of a paste, in that 
case
+   // its spelling is the scratch memory, so we take the parent 
context.
+isInSystemHeader(getSpellingLoc(getImmediateMacroCallerLoc(loc;
   }
 
   /// The size of the SLocEntry that \p FID represents.


Index: test/Misc/no-warn-in-system-macro.c.inc
===
--- /dev/null
+++ test/Misc/no-warn-in-system-macro.c.inc
@@ -0,0 +1,9 @@
+extern int __isnanf(float f);
+extern int __isnan(double f);
+extern int __isnanl(long double f);
+#define isnan(x) \
+	(sizeof (x) == sizeof (float)\
+	? __isnanf (x)\
+	: sizeof (x) == sizeof (double)   \
+	? __isnan (x) : __isnanl (x))
+
Index: test/Misc/no-warn-in-system-macro.c
===
--- /dev/null
+++ test/Misc/no-warn-in-system-macro.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -isystem %S -Wdouble-promotion -fsyntax-only %s  2>&1 | FileCheck -allow-empty %s
+// CHECK-NOT: warning:
+
+#include 
+
+int main(void)
+{
+	double foo = 1.0;
+
+	if (isnan(foo))
+		return 1;
+	return 0;
+}
Index: include/clang/Basic/SourceManager.h
===
--- include/clang/Basic/SourceManager.h
+++ include/clang/Basic/SourceManager.h
@@ -1453,7 +1453,11 @@
 
   /// Returns whether \p Loc is expanded from a macro in a system header.
   bool isInSystemMacro(SourceLocation loc) const {
-return loc.isMacroID() && isInSystemHeader(getSpellingLoc(loc));
+return loc.isMacroID() &&
+   (isInSystemHeader(getSpellingLoc(loc)) ||
+	// This happens when the macro is the result of a paste, in that case
+	// its spelling is the scratch memory, so we take the parent context.
+isInSystemHeader(getSpellingLoc(getImmediateMacroCallerLoc(loc;
   }
 
   /// The size of the SLocEntry that \p FID represents.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D53153#1353256 , @scott.linder 
wrote:

> In D53153#1317977 , @rjmccall wrote:
>
> > I think `-fvisibility=hidden` isn't good enough because you want to infer 
> > hidden visibility even for external symbol references, and that's just not 
> > how global visibility works.  But after this discussion, I'm prepared to 
> > accept that (1) we should have some sort of single-image compiler mode that 
> > implies that all symbols are defined within the image and (2) you can make 
> > your toolchain imply that together with `-fvisibility=hidden` and then have 
> > specific symbols opt in to non-hidden visibility if they need to be 
> > accessible to whatever loader / runtime you have.
>
>
> It seems that explicit visibility attributes on external symbol references 
> (e.g. `__attribute__((visibility("hidden"))) extern int foo;`) are respected 
> in Clang, so I don't understand the rationale for global visibility controls 
> not applying to them as well. Can you describe why this is the case?


Well, one answer is that that's the behavior that GCC defined when they added 
global visibility controls, and it would be unreasonable for Clang to deviate 
in such a fundamental way.  However, we do deviate in some other ways in our 
interpretation of visibility attributes, so that's not a totally satisfactory 
answer.

The stronger answer is that GCC has a very good reason for this behavior.  
Global visibility controls apply to all code in the translation unit.  While 
programmers often mentally distinguish between "project" headers (which they 
control) and "library" headers (which they do not), that is not a difference 
that is known to the compiler.  Therefore, if global visibility controls 
applied to external symbols, the compiler would end up assuming that all 
external symbols not annotated as `default` are provided by the current image.  
That assumption would have been incompatible with essentially all existing 
headers for dynamic libraries on GCC's core targets, including system headers; 
in other words, it would have caused ubiquitous miscompiles.*  It would be 
completely unreasonable to expect all those headers to be changed just for the 
benefit of a single new compiler feature.  Therefore, global visibility 
controls do not apply to external symbols because doing so would make it 
impossible to actually enable global visibility controls.

- GCC's rule helps a great deal even in C++, which is generally more sensitive 
to global visibility: you can typically use an unannotated C++ header under 
`-fvisibility=hidden` and have no problems unless you start using its types as 
exceptions (and GCC tries to work around that, too) or comparing the addresses 
of inline functions or template instantiations.

John.


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

https://reviews.llvm.org/D53153



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

[PATCH] D56655: [clangd] Fix mac buildbot failure.

[Haojian Wu via Phabricator via cfe-commits]

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

Fix a use-after-free bug. The inner loop may add element to `Dependencies`
vector, which may cause memory reallocation of the vector.

Also fix an assertion, we have to take errors when llvm::Expected holds an
error.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56655

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -484,7 +484,7 @@
   // Goes over each dependency.
   for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+std::string CurDependencyPath = Dependencies[CurrentDependency].Path;
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one 
TU
 // to another.
@@ -504,11 +504,18 @@
 // These are the edges in the include graph for current dependency.
 for (const auto &I : *Shard->Sources) {
   auto U = URI::parse(I.getKey());
-  if (!U)
+  if (!U) {
+elog("Failed to parse URI {0}: {1}", I.getKey(), U.takeError());
 continue;
+  }
+
   auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
-  if (!AbsolutePath)
+  if (!AbsolutePath) {
+elog("Failed to resolve URI {0}: {1}", I.getKey(),
+ AbsolutePath.takeError());
 continue;
+  }
+
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
 Dependencies.emplace_back(*AbsolutePath, true);
@@ -527,6 +534,7 @@
   // Check if the source needs re-indexing.
   // Get the digest, skip it if file doesn't exist.
   auto Buf = FS->getBufferForFile(CurDependencyPath);
+
   if (!Buf) {
 elog("Couldn't get buffer for file: {0}: {1}", CurDependencyPath,
  Buf.getError().message());


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -484,7 +484,7 @@
   // Goes over each dependency.
   for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+std::string CurDependencyPath = Dependencies[CurrentDependency].Path;
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one TU
 // to another.
@@ -504,11 +504,18 @@
 // These are the edges in the include graph for current dependency.
 for (const auto &I : *Shard->Sources) {
   auto U = URI::parse(I.getKey());
-  if (!U)
+  if (!U) {
+elog("Failed to parse URI {0}: {1}", I.getKey(), U.takeError());
 continue;
+  }
+
   auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
-  if (!AbsolutePath)
+  if (!AbsolutePath) {
+elog("Failed to resolve URI {0}: {1}", I.getKey(),
+ AbsolutePath.takeError());
 continue;
+  }
+
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
 Dependencies.emplace_back(*AbsolutePath, true);
@@ -527,6 +534,7 @@
   // Check if the source needs re-indexing.
   // Get the digest, skip it if file doesn't exist.
   auto Buf = FS->getBufferForFile(CurDependencyPath);
+
   if (!Buf) {
 elog("Couldn't get buffer for file: {0}: {1}", CurDependencyPath,
  Buf.getError().message());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56655: [clangd] Fix mac buildbot failure.

[Haojian Wu via Phabricator via cfe-commits]

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

remove unrelated blank changes.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56655

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -484,7 +484,7 @@
   // Goes over each dependency.
   for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+std::string CurDependencyPath = Dependencies[CurrentDependency].Path;
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one 
TU
 // to another.
@@ -504,11 +504,16 @@
 // These are the edges in the include graph for current dependency.
 for (const auto &I : *Shard->Sources) {
   auto U = URI::parse(I.getKey());
-  if (!U)
+  if (!U) {
+elog("Failed to parse URI {0}: {1}", I.getKey(), U.takeError());
 continue;
+  }
   auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
-  if (!AbsolutePath)
+  if (!AbsolutePath) {
+elog("Failed to resolve URI {0}: {1}", I.getKey(),
+ AbsolutePath.takeError());
 continue;
+  }
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
 Dependencies.emplace_back(*AbsolutePath, true);


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -484,7 +484,7 @@
   // Goes over each dependency.
   for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+std::string CurDependencyPath = Dependencies[CurrentDependency].Path;
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one TU
 // to another.
@@ -504,11 +504,16 @@
 // These are the edges in the include graph for current dependency.
 for (const auto &I : *Shard->Sources) {
   auto U = URI::parse(I.getKey());
-  if (!U)
+  if (!U) {
+elog("Failed to parse URI {0}: {1}", I.getKey(), U.takeError());
 continue;
+  }
   auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
-  if (!AbsolutePath)
+  if (!AbsolutePath) {
+elog("Failed to resolve URI {0}: {1}", I.getKey(),
+ AbsolutePath.takeError());
 continue;
+  }
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
 Dependencies.emplace_back(*AbsolutePath, true);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47819: [compiler-rt] [test] Disable sunrpc tests when rpc/xdr.h is missing

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

Ping. I'd really like to merge this before the branch.


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

https://reviews.llvm.org/D47819



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

[PATCH] D56592: [clangd] Do not override contents of the shards without modification

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:309
   for (const auto &I : *Index.Sources) {
+// We already have the map from uris to absolutepaths in the cache,
+// therefore traverse Index.Sources rather than Files to get rid of 
absolute

If this the only reason we're traversing `Index.Sources`? If so, I suggest 
removing this comment and traversing `Files` instead.
This would make the code more straightforward and would definitely cost us only 
a negligible performance penalty.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56592



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

[PATCH] D56656: [clangd] Fix a reference invalidation

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.

Fix for the breakage in 
http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/52811/consoleFull#-42777206a1ca8a51-895e-46c6-af87-ce24fa4cd561


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56656

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -510,8 +510,12 @@
   if (!AbsolutePath)
 continue;
   // Add file as dependency if haven't seen before.
-  if (InQueue.try_emplace(*AbsolutePath).second)
+  if (InQueue.try_emplace(*AbsolutePath).second) {
 Dependencies.emplace_back(*AbsolutePath, true);
+// The insertion above might invalidate the reference, so update it as
+// well.
+CurDependencyPath = Dependencies[CurrentDependency].Path;
+  }
   // The node contains symbol information only for current file, the rest 
is
   // just edges.
   if (*AbsolutePath != CurDependencyPath)


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -510,8 +510,12 @@
   if (!AbsolutePath)
 continue;
   // Add file as dependency if haven't seen before.
-  if (InQueue.try_emplace(*AbsolutePath).second)
+  if (InQueue.try_emplace(*AbsolutePath).second) {
 Dependencies.emplace_back(*AbsolutePath, true);
+// The insertion above might invalidate the reference, so update it as
+// well.
+CurDependencyPath = Dependencies[CurrentDependency].Path;
+  }
   // The node contains symbol information only for current file, the rest is
   // just edges.
   if (*AbsolutePath != CurDependencyPath)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r351041 - [clangd] Index main-file symbols (bug 39761)

[Sam McCall via cfe-commits]

Author: sammccall
Date: Mon Jan 14 02:01:17 2019
New Revision: 351041

URL: http://llvm.org/viewvc/llvm-project?rev=351041&view=rev
Log:
[clangd] Index main-file symbols (bug 39761)

Patch by Nathan Ridge!

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

Modified:
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/Index.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=351041&r1=351040&r2=351041&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Index.h (original)
+++ clang-tools-extra/trunk/clangd/index/Index.h Mon Jan 14 02:01:17 2019
@@ -241,6 +241,8 @@ struct Symbol {
 Deprecated = 1 << 1,
 // Symbol is an implementation detail.
 ImplementationDetail = 1 << 2,
+// Symbol is visible to other files (not e.g. a static helper function).
+VisibleOutsideFile = 1 << 3,
   };
 
   SymbolFlag Flags = SymbolFlag::None;

Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=351041&r1=351040&r2=351041&view=diff
==
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Mon Jan 14 
02:01:17 2019
@@ -240,22 +240,20 @@ void SymbolCollector::initialize(ASTCont
 
 bool SymbolCollector::shouldCollectSymbol(const NamedDecl &ND,
   const ASTContext &ASTCtx,
-  const Options &Opts) {
+  const Options &Opts,
+  bool IsMainFileOnly) {
   if (ND.isImplicit())
 return false;
   // Skip anonymous declarations, e.g (anonymous enum/class/struct).
   if (ND.getDeclName().isEmpty())
 return false;
 
-  // FIXME: figure out a way to handle internal linkage symbols (e.g. static
-  // variables, function) defined in the .cc files. Also we skip the symbols
-  // in anonymous namespace as the qualifier names of these symbols are like
-  // `foobar`, which need a special handling.
-  // In real world projects, we have a relatively large set of header files
-  // that define static variables (like "static const int A = 1;"), we still
-  // want to collect these symbols, although they cause potential ODR
-  // violations.
-  if (ND.isInAnonymousNamespace())
+  // Skip main-file symbols if we are not collecting them.
+  if (IsMainFileOnly && !Opts.CollectMainFileSymbols)
+return false;
+
+  // Skip symbols in anonymous namespaces in header files.
+  if (!IsMainFileOnly && ND.isInAnonymousNamespace())
 return false;
 
   // We want most things but not "local" symbols such as symbols inside
@@ -285,10 +283,6 @@ bool SymbolCollector::shouldCollectSymbo
   explicitTemplateSpecialization(ND))
 return false;
 
-  const auto &SM = ASTCtx.getSourceManager();
-  // Skip decls in the main file.
-  if (SM.isInMainFile(SM.getExpansionLoc(ND.getBeginLoc(
-return false;
   // Avoid indexing internal symbols in protobuf generated headers.
   if (isPrivateProtoDecl(ND))
 return false;
@@ -335,9 +329,15 @@ bool SymbolCollector::handleDeclOccurenc
 
   if (IsOnlyRef && !CollectRef)
 return true;
-  if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
+
+  // ND is the canonical (i.e. first) declaration. If it's in the main file,
+  // then no public declaration was visible, so assume it's main-file only.
+  bool IsMainFileOnly = SM.isWrittenInMainFile(SM.getExpansionLoc(
+ND->getBeginLoc()));
+  if (!shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
 return true;
-  if (CollectRef && !isa(ND) &&
+  // Do not store references to main-file symbols.
+  if (CollectRef && !IsMainFileOnly && !isa(ND) &&
   (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
 DeclRefs[ND].emplace_back(SpellingLoc, Roles);
   // Don't continue indexing if this is a mere reference.
@@ -351,13 +351,13 @@ bool SymbolCollector::handleDeclOccurenc
   const NamedDecl &OriginalDecl = *cast(ASTNode.OrigD);
   const Symbol *BasicSymbol = Symbols.find(*ID);
   if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
-BasicSymbol = addDeclaration(*ND, std::move(*ID));
+BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
   else if (isPreferredDeclaration(OriginalDecl, Roles))
 // If OriginalDecl is preferred, replace the existing canonica

[PATCH] D56656: [clangd] Fix a reference invalidation

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:517
+// well.
+CurDependencyPath = Dependencies[CurrentDependency].Path;
+  }

We should avoid modifying the container we're iterating over instead.
I suggest we separate the queue and the return value into two separate vectors.

We would `pop()` from the queue and `push()` to the result vector, therefore 
making bugs like this impossible.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56656



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

[PATCH] D55185: [Clangd] Index main-file symbols (bug 39761)

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351041: [clangd] Index main-file symbols (bug 39761) 
(authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55185

Files:
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.h
  clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
@@ -62,6 +62,7 @@
   : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {
 // Make sure the test root directory is created.
 FSProvider.Files[testPath("unused")] = "";
+CDB.ExtraClangFlags = {"-xc++"};
   }
 
 protected:
@@ -141,10 +142,11 @@
 
 TEST_F(WorkspaceSymbolsTest, InMainFile) {
   addFile("foo.cpp", R"cpp(
-  int test() {
-  }
+  int test() {}
+  static test2() {}
   )cpp");
-  EXPECT_THAT(getSymbols("test"), IsEmpty());
+  EXPECT_THAT(getSymbols("test"), 
+  ElementsAre(QName("test"), QName("test2")));
 }
 
 TEST_F(WorkspaceSymbolsTest, Namespaces) {
@@ -184,7 +186,7 @@
   addFile("foo.cpp", R"cpp(
   #include "foo.h"
   )cpp");
-  EXPECT_THAT(getSymbols("test"), IsEmpty());
+  EXPECT_THAT(getSymbols("test"), ElementsAre(QName("test")));
 }
 
 TEST_F(WorkspaceSymbolsTest, MultiFile) {
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -87,6 +87,9 @@
 MATCHER(ImplementationDetail, "") {
   return arg.Flags & Symbol::ImplementationDetail;
 }
+MATCHER(VisibleOutsideFile, "") {
+  return static_cast(arg.Flags & Symbol::VisibleOutsideFile);
+}
 MATCHER(RefRange, "") {
   const Ref &Pos = testing::get<0>(arg);
   const Range &Range = testing::get<1>(arg);
@@ -113,9 +116,13 @@
   // build() must have been called.
   bool shouldCollect(llvm::StringRef Name, bool Qualified = true) {
 assert(AST.hasValue());
+const NamedDecl& ND = Qualified ? findDecl(*AST, Name) 
+: findUnqualifiedDecl(*AST, Name);
+ASTContext& Ctx = AST->getASTContext();
+const SourceManager& SM = Ctx.getSourceManager();
+bool MainFile = SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
 return SymbolCollector::shouldCollectSymbol(
-Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name),
-AST->getASTContext(), SymbolCollector::Options());
+ND, Ctx, SymbolCollector::Options(), MainFile);
   }
 
 protected:
@@ -131,18 +138,22 @@
 class X{};
 auto f() { int Local; } // auto ensures function body is parsed.
 struct { int x; } var;
-namespace { class InAnonymous {}; }
 }
   )",
-"class InMain {};");
+R"(
+class InMain {};
+namespace { class InAnonymous {}; }
+static void g();
+  )");
   auto AST = File.build();
   EXPECT_TRUE(shouldCollect("nx"));
   EXPECT_TRUE(shouldCollect("nx::X"));
   EXPECT_TRUE(shouldCollect("nx::f"));
+  EXPECT_TRUE(shouldCollect("InMain"));
+  EXPECT_TRUE(shouldCollect("InAnonymous", /*Qualified=*/false));
+  EXPECT_TRUE(shouldCollect("g"));
 
-  EXPECT_FALSE(shouldCollect("InMain"));
   EXPECT_FALSE(shouldCollect("Local", /*Qualified=*/false));
-  EXPECT_FALSE(shouldCollect("InAnonymous", /*Qualified=*/false));
 }
 
 TEST_F(ShouldCollectSymbolTest, NoPrivateProtoSymbol) {
@@ -347,6 +358,35 @@
AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
 }
 
+TEST_F(SymbolCollectorTest, FileLocal) {
+  const std::string Header = R"(
+class Foo {};
+namespace {
+  class Ignored {};
+}
+void bar();
+  )";
+  const std::string Main = R"(
+class ForwardDecl;
+void bar() {}
+static void a();
+class B {};
+namespace {
+  void c();
+}
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Foo"), VisibleOutsideFile()),
+  AllOf(QName("bar"), VisibleOutsideFile()),
+  AllOf(QName("a"), Not(VisibleOutsideFile())),
+  AllOf(QName("B"), Not(VisibleOutsideFile())),
+  AllOf(QName("c"), Not(VisibleOutsideFile())),
+  // FIXME: ForwardDecl likely *is* visible outside.
+  AllOf(QName(

[PATCH] D55185: [Clangd] Index main-file symbols (bug 39761)

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 181510.
sammccall added a comment.
Herald added a subscriber: cfe-commits.

add comment about subtleties around canonical declarations and main-file check
invert sense of Symbol flag (FileLocal -> VisibleOutsideFile) so Merge does the 
right thing
test: add case for forward declarations (I think we do the wrong thing, fix 
later)
test: replace Matcher(true)/Matcher(false) with Matcher() and Not(Matcher())


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55185

Files:
  clangd/index/Index.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/BackgroundIndexTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -87,6 +87,9 @@
 MATCHER(ImplementationDetail, "") {
   return arg.Flags & Symbol::ImplementationDetail;
 }
+MATCHER(VisibleOutsideFile, "") {
+  return static_cast(arg.Flags & Symbol::VisibleOutsideFile);
+}
 MATCHER(RefRange, "") {
   const Ref &Pos = testing::get<0>(arg);
   const Range &Range = testing::get<1>(arg);
@@ -113,9 +116,13 @@
   // build() must have been called.
   bool shouldCollect(llvm::StringRef Name, bool Qualified = true) {
 assert(AST.hasValue());
+const NamedDecl& ND = Qualified ? findDecl(*AST, Name) 
+: findUnqualifiedDecl(*AST, Name);
+ASTContext& Ctx = AST->getASTContext();
+const SourceManager& SM = Ctx.getSourceManager();
+bool MainFile = SM.isWrittenInMainFile(SM.getExpansionLoc(ND.getBeginLoc()));
 return SymbolCollector::shouldCollectSymbol(
-Qualified ? findDecl(*AST, Name) : findUnqualifiedDecl(*AST, Name),
-AST->getASTContext(), SymbolCollector::Options());
+ND, Ctx, SymbolCollector::Options(), MainFile);
   }
 
 protected:
@@ -131,18 +138,22 @@
 class X{};
 auto f() { int Local; } // auto ensures function body is parsed.
 struct { int x; } var;
-namespace { class InAnonymous {}; }
 }
   )",
-"class InMain {};");
+R"(
+class InMain {};
+namespace { class InAnonymous {}; }
+static void g();
+  )");
   auto AST = File.build();
   EXPECT_TRUE(shouldCollect("nx"));
   EXPECT_TRUE(shouldCollect("nx::X"));
   EXPECT_TRUE(shouldCollect("nx::f"));
+  EXPECT_TRUE(shouldCollect("InMain"));
+  EXPECT_TRUE(shouldCollect("InAnonymous", /*Qualified=*/false));
+  EXPECT_TRUE(shouldCollect("g"));
 
-  EXPECT_FALSE(shouldCollect("InMain"));
   EXPECT_FALSE(shouldCollect("Local", /*Qualified=*/false));
-  EXPECT_FALSE(shouldCollect("InAnonymous", /*Qualified=*/false));
 }
 
 TEST_F(ShouldCollectSymbolTest, NoPrivateProtoSymbol) {
@@ -347,6 +358,35 @@
AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
 }
 
+TEST_F(SymbolCollectorTest, FileLocal) {
+  const std::string Header = R"(
+class Foo {};
+namespace {
+  class Ignored {};
+}
+void bar();
+  )";
+  const std::string Main = R"(
+class ForwardDecl;
+void bar() {}
+static void a();
+class B {};
+namespace {
+  void c();
+}
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Foo"), VisibleOutsideFile()),
+  AllOf(QName("bar"), VisibleOutsideFile()),
+  AllOf(QName("a"), Not(VisibleOutsideFile())),
+  AllOf(QName("B"), Not(VisibleOutsideFile())),
+  AllOf(QName("c"), Not(VisibleOutsideFile())),
+  // FIXME: ForwardDecl likely *is* visible outside.
+  AllOf(QName("ForwardDecl"), Not(VisibleOutsideFile();
+}
+
 TEST_F(SymbolCollectorTest, Template) {
   Annotations Header(R"(
 // Template is indexed, specialization and instantiation is not.
@@ -417,7 +457,7 @@
 void $printdef[[print]]() {}
 
 // Declared/defined in main only.
-int Y;
+int $ydecl[[Y]];
   )cpp");
   runSymbolCollector(Header.code(), Main.code());
   EXPECT_THAT(Symbols,
@@ -429,7 +469,8 @@
   AllOf(QName("print"), DeclRange(Header.range("printdecl")),
 DefRange(Main.range("printdef"))),
   AllOf(QName("Z"), DeclRange(Header.range("zdecl"))),
-  AllOf(QName("foo"), DeclRange(Header.range("foodecl");
+  AllOf(QName("foo"), DeclRange(Header.range("foodecl"))),
+  AllOf(QName("Y"), DeclRange(Main.range("ydecl");
 }
 
 TEST_F(SymbolCollectorTest, Refs) {
@@ -508,17 +549,25 @@
 W* w2 = nullptr; // only one usage counts
 X x();
 class V;
-V* v = nullptr; // Used, but not eligible for indexing.
 class Y{}; // definition doesn't count as a reference

[PATCH] D56656: [clangd] Fix a reference invalidation

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 181512.
kadircet added a comment.

- Change traversal logic to use two different containers


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56656

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -476,29 +476,31 @@
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
   std::vector Dependencies;
+  std::queue ToVisit;
   std::string AbsolutePath = getAbsolutePath(Cmd).str();
   // Up until we load the shard related to a dependency it needs to be
   // re-indexed.
-  Dependencies.emplace_back(AbsolutePath, true);
+  ToVisit.emplace(AbsolutePath, true);
   InQueue.insert(AbsolutePath);
   // Goes over each dependency.
-  for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
-   CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+  while (!ToVisit.empty()) {
+Dependencies.push_back(std::move(ToVisit.front()));
+auto &CurDependency = Dependencies.back();
+ToVisit.pop();
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one 
TU
 // to another.
-if (!LoadedShards.try_emplace(CurDependencyPath).second) {
+if (!LoadedShards.try_emplace(CurDependency.Path).second) {
   // If the dependency needs to be re-indexed, first occurence would 
already
   // have detected that, so we don't need to issue it again.
-  Dependencies[CurrentDependency].NeedsReIndexing = false;
+  CurDependency.NeedsReIndexing = false;
   continue;
 }
 
-auto Shard = IndexStorage->loadShard(CurDependencyPath);
+auto Shard = IndexStorage->loadShard(CurDependency.Path);
 if (!Shard || !Shard->Sources) {
   // File will be returned as requiring re-indexing to caller.
-  vlog("Failed to load shard: {0}", CurDependencyPath);
+  vlog("Failed to load shard: {0}", CurDependency.Path);
   continue;
 }
 // These are the edges in the include graph for current dependency.
@@ -506,34 +508,34 @@
   auto U = URI::parse(I.getKey());
   if (!U)
 continue;
-  auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
+  auto AbsolutePath = URI::resolve(*U, CurDependency.Path);
   if (!AbsolutePath)
 continue;
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
-Dependencies.emplace_back(*AbsolutePath, true);
+ToVisit.emplace(*AbsolutePath, true);
   // The node contains symbol information only for current file, the rest 
is
   // just edges.
-  if (*AbsolutePath != CurDependencyPath)
+  if (*AbsolutePath != CurDependency.Path)
 continue;
 
   // We found source file info for current dependency.
   assert(I.getValue().Digest != FileDigest{{0}} && "Digest is empty?");
   ShardInfo SI;
-  SI.AbsolutePath = CurDependencyPath;
+  SI.AbsolutePath = CurDependency.Path;
   SI.Shard = std::move(Shard);
   SI.Digest = I.getValue().Digest;
   IntermediateSymbols.push_back(std::move(SI));
   // Check if the source needs re-indexing.
   // Get the digest, skip it if file doesn't exist.
-  auto Buf = FS->getBufferForFile(CurDependencyPath);
+  auto Buf = FS->getBufferForFile(CurDependency.Path);
   if (!Buf) {
-elog("Couldn't get buffer for file: {0}: {1}", CurDependencyPath,
+elog("Couldn't get buffer for file: {0}: {1}", CurDependency.Path,
  Buf.getError().message());
 continue;
   }
   // If digests match then dependency doesn't need re-indexing.
-  Dependencies[CurrentDependency].NeedsReIndexing =
+  CurDependency.NeedsReIndexing =
   digest(Buf->get()->getBuffer()) != I.getValue().Digest;
 }
   }


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -476,29 +476,31 @@
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
   std::vector Dependencies;
+  std::queue ToVisit;
   std::string AbsolutePath = getAbsolutePath(Cmd).str();
   // Up until we load the shard related to a dependency it needs to be
   // re-indexed.
-  Dependencies.emplace_back(AbsolutePath, true);
+  ToVisit.emplace(AbsolutePath, true);
   InQueue.insert(AbsolutePath);
   // Goes over each dependency.
-  for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
-   CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path

[PATCH] D55185: [Clangd] Index main-file symbols (bug 39761)

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The forward-declaration-in-main-file case:
In principle, if the definition is missing, I think we should treat these 
differently (not index them, or treat them as visible-outside file) because 
they're almost certainly declared "properly" elsewhere.
@ilya-biryukov convinced me offline that this isn't a big deal in practice as 
forward declarations in main-files (and not subsequently defining the symbol) 
aren't that useful.
So going to avoid patching that, as there might be unintended consequences.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55185



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

[libunwind] r351044 - [Sparc] Add Sparc V8 support

[Daniel Cederman via cfe-commits]

Author: dcederman
Date: Mon Jan 14 02:15:20 2019
New Revision: 351044

URL: http://llvm.org/viewvc/llvm-project?rev=351044&view=rev
Log:
[Sparc] Add Sparc V8 support

Summary:
Adds the register class implementation for Sparc.
Adds support for DW_CFA_GNU_window_save.
Adds save and restore context functionality.

Adds getArch() function to each Registers_ class to be able to separate
between DW_CFA_AARCH64_negate_ra_state and DW_CFA_GNU_window_save which
are both represented by the same constant.

On Sparc the return address is the address of the call instruction, so
an offset needs to be added when returning to skip the call instruction
and its delay slot. If the function returns a struct it is also necessary
to skip one extra instruction on Sparc V8.

Reviewers: jyknight, mclow.lists, mstorsjo, compnerd

Reviewed By: jyknight, compnerd

Subscribers: jgorbe, mgorny, christof, llvm-commits, fedor.sergeev, 
JDevlieghere, ldionne, libcxx-commits

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

Modified:
libunwind/trunk/include/__libunwind_config.h
libunwind/trunk/include/libunwind.h
libunwind/trunk/src/DwarfInstructions.hpp
libunwind/trunk/src/DwarfParser.hpp
libunwind/trunk/src/Registers.hpp
libunwind/trunk/src/UnwindCursor.hpp
libunwind/trunk/src/UnwindRegistersRestore.S
libunwind/trunk/src/UnwindRegistersSave.S
libunwind/trunk/src/assembly.h
libunwind/trunk/src/libunwind.cpp

Modified: libunwind/trunk/include/__libunwind_config.h
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/include/__libunwind_config.h?rev=351044&r1=351043&r2=351044&view=diff
==
--- libunwind/trunk/include/__libunwind_config.h (original)
+++ libunwind/trunk/include/__libunwind_config.h Mon Jan 14 02:15:20 2019
@@ -23,6 +23,7 @@
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER_ARM   287
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER_OR1K  32
 #define _LIBUNWIND_HIGHEST_DWARF_REGISTER_MIPS  65
+#define _LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC 31
 
 #if defined(_LIBUNWIND_IS_NATIVE_ONLY)
 # if defined(__i386__)
@@ -113,6 +114,11 @@
 #error "Unsupported MIPS ABI and/or environment"
 #  endif
 #  define _LIBUNWIND_HIGHEST_DWARF_REGISTER 
_LIBUNWIND_HIGHEST_DWARF_REGISTER_MIPS
+# elif defined(__sparc__)
+  #define _LIBUNWIND_TARGET_SPARC 1
+  #define _LIBUNWIND_HIGHEST_DWARF_REGISTER 
_LIBUNWIND_HIGHEST_DWARF_REGISTER_SPARC
+  #define _LIBUNWIND_CONTEXT_SIZE 16
+  #define _LIBUNWIND_CURSOR_SIZE 23
 # else
 #  error "Unsupported architecture."
 # endif
@@ -126,6 +132,7 @@
 # define _LIBUNWIND_TARGET_OR1K 1
 # define _LIBUNWIND_TARGET_MIPS_O32 1
 # define _LIBUNWIND_TARGET_MIPS_NEWABI 1
+# define _LIBUNWIND_TARGET_SPARC 1
 # define _LIBUNWIND_CONTEXT_SIZE 167
 # define _LIBUNWIND_CURSOR_SIZE 179
 # define _LIBUNWIND_HIGHEST_DWARF_REGISTER 287

Modified: libunwind/trunk/include/libunwind.h
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/include/libunwind.h?rev=351044&r1=351043&r2=351044&view=diff
==
--- libunwind/trunk/include/libunwind.h (original)
+++ libunwind/trunk/include/libunwind.h Mon Jan 14 02:15:20 2019
@@ -823,4 +823,40 @@ enum {
   UNW_MIPS_LO = 65,
 };
 
+// SPARC registers
+enum {
+  UNW_SPARC_G0 = 0,
+  UNW_SPARC_G1 = 1,
+  UNW_SPARC_G2 = 2,
+  UNW_SPARC_G3 = 3,
+  UNW_SPARC_G4 = 4,
+  UNW_SPARC_G5 = 5,
+  UNW_SPARC_G6 = 6,
+  UNW_SPARC_G7 = 7,
+  UNW_SPARC_O0 = 8,
+  UNW_SPARC_O1 = 9,
+  UNW_SPARC_O2 = 10,
+  UNW_SPARC_O3 = 11,
+  UNW_SPARC_O4 = 12,
+  UNW_SPARC_O5 = 13,
+  UNW_SPARC_O6 = 14,
+  UNW_SPARC_O7 = 15,
+  UNW_SPARC_L0 = 16,
+  UNW_SPARC_L1 = 17,
+  UNW_SPARC_L2 = 18,
+  UNW_SPARC_L3 = 19,
+  UNW_SPARC_L4 = 20,
+  UNW_SPARC_L5 = 21,
+  UNW_SPARC_L6 = 22,
+  UNW_SPARC_L7 = 23,
+  UNW_SPARC_I0 = 24,
+  UNW_SPARC_I1 = 25,
+  UNW_SPARC_I2 = 26,
+  UNW_SPARC_I3 = 27,
+  UNW_SPARC_I4 = 28,
+  UNW_SPARC_I5 = 29,
+  UNW_SPARC_I6 = 30,
+  UNW_SPARC_I7 = 31,
+};
+
 #endif

Modified: libunwind/trunk/src/DwarfInstructions.hpp
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/DwarfInstructions.hpp?rev=351044&r1=351043&r2=351044&view=diff
==
--- libunwind/trunk/src/DwarfInstructions.hpp (original)
+++ libunwind/trunk/src/DwarfInstructions.hpp Mon Jan 14 02:15:20 2019
@@ -159,7 +159,7 @@ int DwarfInstructions::stepWithDwa
&cieInfo) == NULL) {
 PrologInfo prolog;
 if (CFI_Parser::parseFDEInstructions(addressSpace, fdeInfo, cieInfo, pc,
-&prolog)) {
+R::getArch(), &prolog)) {
   // get pointer to cfa (architecture specific)
   pint_t cfa = getCFA(addressSpace, prolog, registers);
 
@@ -204,7 +204,8 @@ int DwarfInstructions::stepWithDwa
   // return address needs to be authenticated before

[PATCH] D56656: [clangd] Fix a reference invalidation

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:488
+Dependencies.push_back(std::move(ToVisit.front()));
+auto &CurDependency = Dependencies.back();
+ToVisit.pop();

This reference makes it just as easy to access the vector we'll be modifying 
inside the loop.
Can we avoid modifying the values inside the vector completely? I.e. ideally 
we'll have only `push_back` into the vector and no modifications of the 
internal references?



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56656



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

[PATCH] D56657: [clang-tidy] bugprone-string-constructor: Catch string from nullptr.

[Clement Courbet via Phabricator via cfe-commits]

courbet created this revision.
Herald added subscribers: cfe-commits, xazax.hun.

Context: https://twitter.com/willkirkby/status/1084219580799741953


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56657

Files:
  clang-tidy/bugprone/StringConstructorCheck.cpp
  test/clang-tidy/bugprone-string-constructor.cpp


Index: test/clang-tidy/bugprone-string-constructor.cpp
===
--- test/clang-tidy/bugprone-string-constructor.cpp
+++ test/clang-tidy/bugprone-string-constructor.cpp
@@ -9,6 +9,7 @@
 struct basic_string {
   basic_string();
   basic_string(const C*, unsigned int size);
+  basic_string(const C*, const A& allocator = A());
   basic_string(unsigned int size, C c);
 };
 typedef basic_string string;
@@ -45,6 +46,13 @@
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger then string 
literal size
   std::string q5(kText3,  0x100);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
+  std::string q6(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructing string from nullptr 
is undefined behaviour
+}
+
+std::string StringFromZero() {
+  return 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr 
is undefined behaviour
 }
 
 void Valid() {
@@ -53,4 +61,5 @@
   std::wstring wstr(4, L'x');
   std::string s1("test", 4);
   std::string s2("test", 3);
+  std::string s3("test");
 }
Index: clang-tidy/bugprone/StringConstructorCheck.cpp
===
--- clang-tidy/bugprone/StringConstructorCheck.cpp
+++ clang-tidy/bugprone/StringConstructorCheck.cpp
@@ -100,6 +100,17 @@
integerLiteral().bind("int"))
   .bind("constructor"),
   this);
+
+  // Check the literal string constructor with char pointer.
+  // [i.e. string (const char* s);]
+  Finder->addMatcher(
+  cxxConstructExpr(
+  hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+  //hasArgument(0, hasType(CharPtrType)),
+  hasArgument(0, expr().bind("from-ptr")),
+  hasArgument(1, unless(hasType(isInteger()
+  .bind("constructor"),
+  this);
 }
 
 void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
@@ -128,6 +139,14 @@
 if (Lit->getValue().ugt(Str->getLength())) {
   diag(Loc, "length is bigger then string literal size");
 }
+  } else if (const Expr* Ptr = Result.Nodes.getNodeAs("from-ptr")) {
+Expr::EvalResult ConstPtr;
+if (Ptr->EvaluateAsRValue(ConstPtr, Ctx) && (
+(ConstPtr.Val.isInt() && ConstPtr.Val.getInt().isNullValue()) ||
+(ConstPtr.Val.isLValue() && ConstPtr.Val.isNullPointer())
+  )) {
+  diag(Loc, "constructing string from nullptr is undefined behaviour");
+}
   }
 }
 


Index: test/clang-tidy/bugprone-string-constructor.cpp
===
--- test/clang-tidy/bugprone-string-constructor.cpp
+++ test/clang-tidy/bugprone-string-constructor.cpp
@@ -9,6 +9,7 @@
 struct basic_string {
   basic_string();
   basic_string(const C*, unsigned int size);
+  basic_string(const C*, const A& allocator = A());
   basic_string(unsigned int size, C c);
 };
 typedef basic_string string;
@@ -45,6 +46,13 @@
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger then string literal size
   std::string q5(kText3,  0x100);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
+  std::string q6(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructing string from nullptr is undefined behaviour
+}
+
+std::string StringFromZero() {
+  return 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr is undefined behaviour
 }
 
 void Valid() {
@@ -53,4 +61,5 @@
   std::wstring wstr(4, L'x');
   std::string s1("test", 4);
   std::string s2("test", 3);
+  std::string s3("test");
 }
Index: clang-tidy/bugprone/StringConstructorCheck.cpp
===
--- clang-tidy/bugprone/StringConstructorCheck.cpp
+++ clang-tidy/bugprone/StringConstructorCheck.cpp
@@ -100,6 +100,17 @@
integerLiteral().bind("int"))
   .bind("constructor"),
   this);
+
+  // Check the literal string constructor with char pointer.
+  // [i.e. string (const char* s);]
+  Finder->addMatcher(
+  cxxConstructExpr(
+  hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+  //hasArgument(0, hasType(CharPtrType)),
+  hasArgument(0, expr().bind("from-ptr")),
+  hasArgument(1, unless(hasType(isInteger()
+  .bind("constructor"),
+  this);
 }
 
 void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
@@ -128,6 +139,14 @@
 if (Lit->getValue().ugt(Str->getLength())) {
   diag(Loc, "length is bigger then st

[PATCH] D56656: [clangd] Fix a reference invalidation

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:488
+Dependencies.push_back(std::move(ToVisit.front()));
+auto &CurDependency = Dependencies.back();
+ToVisit.pop();

ilya-biryukov wrote:
> This reference makes it just as easy to access the vector we'll be modifying 
> inside the loop.
> Can we avoid modifying the values inside the vector completely? I.e. ideally 
> we'll have only `push_back` into the vector and no modifications of the 
> internal references?
> 
Ah, but we don't modify the vector anymore.
Could you leave a comment that `Dependencies` is not modified further in the 
loop body, so it's fine to have a reference inside it?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56656



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

[PATCH] D56656: [clangd] Fix a reference invalidation

[Ilya Biryukov via Phabricator via cfe-commits]

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

LGTM, but please add a comment the vector is not modified (it might be useful 
as a reminder in case we want to change the code later)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56656



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

r351047 - [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Mon Jan 14 02:31:42 2019
New Revision: 351047

URL: http://llvm.org/viewvc/llvm-project?rev=351047&view=rev
Log:
[AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

Summary:
This fixes ASTContext's parent map for nodes in such classes (e.g. operator()).
https://bugs.llvm.org/show_bug.cgi?id=39949

This also changes the observed shape of the AST for implicit RAVs.
- this includes AST MatchFinder: cxxRecordDecl() now matches lambda classes,
functionDecl() matches the call operator, and the parent chain is body -> call
operator -> lambda class -> lambdaexpr rather than body -> lambdaexpr.
- this appears not to matter for the ASTImporterLookupTable builder
- this doesn't matter for the other RAVs in-tree.

In order to do this, we remove the TraverseLambdaBody hook. The problem is it's
hard/weird to ensure this hook is called when traversing via the implicit class.
There were just two users of this hook in-tree, who use it to skip bodies.
I replaced these with explicitly traversing the captures only. Another approach
would be recording the bodies when the lambda is visited, and then recognizing
them later.
I'd be open to suggestion on how to preserve this hook, instead.

Reviewers: aaron.ballman, JonasToth

Subscribers: cfe-commits, rsmith, jdennett

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

Modified:
cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/unittests/AST/ASTContextParentMapTest.cpp
cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp

Modified: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=351047&r1=351046&r2=351047&view=diff
==
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h (original)
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h Mon Jan 14 02:31:42 2019
@@ -298,14 +298,6 @@ public:
   bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C,
  Expr *Init);
 
-  /// Recursively visit the body of a lambda expression.
-  ///
-  /// This provides a hook for visitors that need more context when visiting
-  /// \c LE->getBody().
-  ///
-  /// \returns false if the visitation was terminated early, true otherwise.
-  bool TraverseLambdaBody(LambdaExpr *LE, DataRecursionQueue *Queue = nullptr);
-
   /// Recursively visit the syntactic or semantic form of an
   /// initialization list.
   ///
@@ -936,13 +928,6 @@ RecursiveASTVisitor::TraverseLa
   return true;
 }
 
-template 
-bool RecursiveASTVisitor::TraverseLambdaBody(
-LambdaExpr *LE, DataRecursionQueue *Queue) {
-  TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(LE->getBody());
-  return true;
-}
-
 // - Type traversal -
 
 // This macro makes available a variable T, the passed-in type.
@@ -2404,6 +2389,7 @@ DEF_TRAVERSE_STMT(CXXTemporaryObjectExpr
 
 // Walk only the visible parts of lambda expressions.
 DEF_TRAVERSE_STMT(LambdaExpr, {
+  // Visit the capture list.
   for (unsigned I = 0, N = S->capture_size(); I != N; ++I) {
 const LambdaCapture *C = S->capture_begin() + I;
 if (C->isExplicit() || getDerived().shouldVisitImplicitCode()) {
@@ -2411,25 +2397,31 @@ DEF_TRAVERSE_STMT(LambdaExpr, {
 }
   }
 
-  TypeLoc TL = S->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
-  FunctionProtoTypeLoc Proto = TL.getAsAdjusted();
+  if (getDerived().shouldVisitImplicitCode()) {
+// The implicit model is simple: everything else is in the lambda class.
+TRY_TO(TraverseDecl(S->getLambdaClass()));
+  } else {
+// We need to poke around to find the bits that might be explicitly 
written.
+TypeLoc TL = S->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
+FunctionProtoTypeLoc Proto = TL.getAsAdjusted();
+
+if (S->hasExplicitParameters()) {
+  // Visit parameters.
+  for (unsigned I = 0, N = Proto.getNumParams(); I != N; ++I)
+TRY_TO(TraverseDecl(Proto.getParam(I)));
+}
+if (S->hasExplicitResultType())
+  TRY_TO(TraverseTypeLoc(Proto.getReturnLoc()));
 
-  if (S->hasExplicitParameters()) {
-// Visit parameters.
-for (unsigned I = 0, N = Proto.getNumParams(); I != N; ++I)
-  TRY_TO(TraverseDecl(Proto.getParam(I)));
-  }
-  if (S->hasExplicitResultType())
-TRY_TO(TraverseTypeLoc(Proto.getReturnLoc()));
+auto *T = Proto.getTypePtr();
+for (const auto &E : T->exceptions())
+  TRY_TO(TraverseType(E));
 
-  auto *T = Proto.getTypePtr();
-  for (const auto &E : T->exceptions())
-TRY_TO(TraverseType(E));
+if (Expr *NE = T->getNoexceptExpr())
+  TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(NE);
 
-  if (Expr *NE = T->getNoexceptExpr())
-TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(NE);
-
-  ReturnValue = TRAVERSE_STMT_BASE(LambdaBody, LambdaExpr, S, Queu

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351047: [AST] RecursiveASTVisitor visits lambda classes when 
implicit visitation is on. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56444?vs=180796&id=181518#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56444

Files:
  cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
  cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
  cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
  cfe/trunk/unittests/AST/ASTContextParentMapTest.cpp
  cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp

Index: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
===
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
@@ -298,14 +298,6 @@
   bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C,
  Expr *Init);
 
-  /// Recursively visit the body of a lambda expression.
-  ///
-  /// This provides a hook for visitors that need more context when visiting
-  /// \c LE->getBody().
-  ///
-  /// \returns false if the visitation was terminated early, true otherwise.
-  bool TraverseLambdaBody(LambdaExpr *LE, DataRecursionQueue *Queue = nullptr);
-
   /// Recursively visit the syntactic or semantic form of an
   /// initialization list.
   ///
@@ -936,13 +928,6 @@
   return true;
 }
 
-template 
-bool RecursiveASTVisitor::TraverseLambdaBody(
-LambdaExpr *LE, DataRecursionQueue *Queue) {
-  TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(LE->getBody());
-  return true;
-}
-
 // - Type traversal -
 
 // This macro makes available a variable T, the passed-in type.
@@ -2404,6 +2389,7 @@
 
 // Walk only the visible parts of lambda expressions.
 DEF_TRAVERSE_STMT(LambdaExpr, {
+  // Visit the capture list.
   for (unsigned I = 0, N = S->capture_size(); I != N; ++I) {
 const LambdaCapture *C = S->capture_begin() + I;
 if (C->isExplicit() || getDerived().shouldVisitImplicitCode()) {
@@ -2411,25 +2397,31 @@
 }
   }
 
-  TypeLoc TL = S->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
-  FunctionProtoTypeLoc Proto = TL.getAsAdjusted();
+  if (getDerived().shouldVisitImplicitCode()) {
+// The implicit model is simple: everything else is in the lambda class.
+TRY_TO(TraverseDecl(S->getLambdaClass()));
+  } else {
+// We need to poke around to find the bits that might be explicitly written.
+TypeLoc TL = S->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
+FunctionProtoTypeLoc Proto = TL.getAsAdjusted();
+
+if (S->hasExplicitParameters()) {
+  // Visit parameters.
+  for (unsigned I = 0, N = Proto.getNumParams(); I != N; ++I)
+TRY_TO(TraverseDecl(Proto.getParam(I)));
+}
+if (S->hasExplicitResultType())
+  TRY_TO(TraverseTypeLoc(Proto.getReturnLoc()));
 
-  if (S->hasExplicitParameters()) {
-// Visit parameters.
-for (unsigned I = 0, N = Proto.getNumParams(); I != N; ++I)
-  TRY_TO(TraverseDecl(Proto.getParam(I)));
-  }
-  if (S->hasExplicitResultType())
-TRY_TO(TraverseTypeLoc(Proto.getReturnLoc()));
+auto *T = Proto.getTypePtr();
+for (const auto &E : T->exceptions())
+  TRY_TO(TraverseType(E));
 
-  auto *T = Proto.getTypePtr();
-  for (const auto &E : T->exceptions())
-TRY_TO(TraverseType(E));
+if (Expr *NE = T->getNoexceptExpr())
+  TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(NE);
 
-  if (Expr *NE = T->getNoexceptExpr())
-TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(NE);
-
-  ReturnValue = TRAVERSE_STMT_BASE(LambdaBody, LambdaExpr, S, Queue);
+TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(S->getBody());
+  }
   ShouldVisitChildren = false;
 })
 
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1153,7 +1153,12 @@
 bool TraverseDecl(Decl *D) { return true; }
 
 // We analyze lambda bodies separately. Skip them here.
-bool TraverseLambdaBody(LambdaExpr *LE) { return true; }
+bool TraverseLambdaExpr(LambdaExpr *LE) {
+  // Traverse the captures, but not the body.
+  for (const auto &C : zip(LE->captures(), LE->capture_inits()))
+TraverseLambdaCapture(LE, &std::get<0>(C), std::get<1>(C));
+  return true;
+}
 
   private:
 
Index: cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenPGO.cpp
@@ -165,7 +165,12 @@
   // Blocks and lambdas are handled as separate functions, so we need not
   // traverse them in the parent context.
   bool TraverseBlockExpr(BlockExpr *BE) { return true; }
-  bool TraverseLambdaBody(Lam

[PATCH] D56657: [clang-tidy] bugprone-string-constructor: Catch string from nullptr.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 181519.
courbet added a comment.

Remove commented code.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56657

Files:
  clang-tidy/bugprone/StringConstructorCheck.cpp
  test/clang-tidy/bugprone-string-constructor.cpp


Index: test/clang-tidy/bugprone-string-constructor.cpp
===
--- test/clang-tidy/bugprone-string-constructor.cpp
+++ test/clang-tidy/bugprone-string-constructor.cpp
@@ -9,6 +9,7 @@
 struct basic_string {
   basic_string();
   basic_string(const C*, unsigned int size);
+  basic_string(const C*, const A& allocator = A());
   basic_string(unsigned int size, C c);
 };
 typedef basic_string string;
@@ -45,6 +46,13 @@
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger then string 
literal size
   std::string q5(kText3,  0x100);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
+  std::string q6(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructing string from nullptr 
is undefined behaviour
+}
+
+std::string StringFromZero() {
+  return 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr 
is undefined behaviour
 }
 
 void Valid() {
@@ -53,4 +61,5 @@
   std::wstring wstr(4, L'x');
   std::string s1("test", 4);
   std::string s2("test", 3);
+  std::string s3("test");
 }
Index: clang-tidy/bugprone/StringConstructorCheck.cpp
===
--- clang-tidy/bugprone/StringConstructorCheck.cpp
+++ clang-tidy/bugprone/StringConstructorCheck.cpp
@@ -100,6 +100,16 @@
integerLiteral().bind("int"))
   .bind("constructor"),
   this);
+
+  // Check the literal string constructor with char pointer.
+  // [i.e. string (const char* s);]
+  Finder->addMatcher(
+  cxxConstructExpr(
+  hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+  hasArgument(0, expr().bind("from-ptr")),
+  hasArgument(1, unless(hasType(isInteger()
+  .bind("constructor"),
+  this);
 }
 
 void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
@@ -128,6 +138,14 @@
 if (Lit->getValue().ugt(Str->getLength())) {
   diag(Loc, "length is bigger then string literal size");
 }
+  } else if (const Expr* Ptr = Result.Nodes.getNodeAs("from-ptr")) {
+Expr::EvalResult ConstPtr;
+if (Ptr->EvaluateAsRValue(ConstPtr, Ctx) && (
+(ConstPtr.Val.isInt() && ConstPtr.Val.getInt().isNullValue()) ||
+(ConstPtr.Val.isLValue() && ConstPtr.Val.isNullPointer())
+  )) {
+  diag(Loc, "constructing string from nullptr is undefined behaviour");
+}
   }
 }
 


Index: test/clang-tidy/bugprone-string-constructor.cpp
===
--- test/clang-tidy/bugprone-string-constructor.cpp
+++ test/clang-tidy/bugprone-string-constructor.cpp
@@ -9,6 +9,7 @@
 struct basic_string {
   basic_string();
   basic_string(const C*, unsigned int size);
+  basic_string(const C*, const A& allocator = A());
   basic_string(unsigned int size, C c);
 };
 typedef basic_string string;
@@ -45,6 +46,13 @@
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: length is bigger then string literal size
   std::string q5(kText3,  0x100);
   // CHECK-MESSAGES: [[@LINE-1]]:15: warning: suspicious large length parameter
+  std::string q6(nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: constructing string from nullptr is undefined behaviour
+}
+
+std::string StringFromZero() {
+  return 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: constructing string from nullptr is undefined behaviour
 }
 
 void Valid() {
@@ -53,4 +61,5 @@
   std::wstring wstr(4, L'x');
   std::string s1("test", 4);
   std::string s2("test", 3);
+  std::string s3("test");
 }
Index: clang-tidy/bugprone/StringConstructorCheck.cpp
===
--- clang-tidy/bugprone/StringConstructorCheck.cpp
+++ clang-tidy/bugprone/StringConstructorCheck.cpp
@@ -100,6 +100,16 @@
integerLiteral().bind("int"))
   .bind("constructor"),
   this);
+
+  // Check the literal string constructor with char pointer.
+  // [i.e. string (const char* s);]
+  Finder->addMatcher(
+  cxxConstructExpr(
+  hasDeclaration(cxxMethodDecl(hasName("basic_string"))),
+  hasArgument(0, expr().bind("from-ptr")),
+  hasArgument(1, unless(hasType(isInteger()
+  .bind("constructor"),
+  this);
 }
 
 void StringConstructorCheck::check(const MatchFinder::MatchResult &Result) {
@@ -128,6 +138,14 @@
 if (Lit->getValue().ugt(Str->getLength())) {
   diag(Loc, "length is bigger then string literal size");
 }
+  } else if (const Expr* Ptr = Result.Nodes.getNodeAs("from-p

[PATCH] D56552: [clang-tidy] update FunctionSizeCheck for D56444

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE351048: [clang-tidy] update FunctionSizeCheck for D56444 
(authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56552?vs=181082&id=181520#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56552

Files:
  clang-tidy/readability/FunctionSizeCheck.cpp


Index: clang-tidy/readability/FunctionSizeCheck.cpp
===
--- clang-tidy/readability/FunctionSizeCheck.cpp
+++ clang-tidy/readability/FunctionSizeCheck.cpp
@@ -145,7 +145,12 @@
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), 
this);
+  // Lambdas ignored - historically considered part of enclosing function.
+  // FIXME: include them instead? Top-level lambdas are currently never 
counted.
+  Finder->addMatcher(functionDecl(unless(isInstantiated()),
+  unless(cxxMethodDecl(ofClass(isLambda()
+ .bind("func"),
+ this);
 }
 
 void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {


Index: clang-tidy/readability/FunctionSizeCheck.cpp
===
--- clang-tidy/readability/FunctionSizeCheck.cpp
+++ clang-tidy/readability/FunctionSizeCheck.cpp
@@ -145,7 +145,12 @@
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), this);
+  // Lambdas ignored - historically considered part of enclosing function.
+  // FIXME: include them instead? Top-level lambdas are currently never counted.
+  Finder->addMatcher(functionDecl(unless(isInstantiated()),
+  unless(cxxMethodDecl(ofClass(isLambda()
+ .bind("func"),
+ this);
 }
 
 void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r351048 - [clang-tidy] update FunctionSizeCheck for D56444

[Sam McCall via cfe-commits]

Author: sammccall
Date: Mon Jan 14 02:40:41 2019
New Revision: 351048

URL: http://llvm.org/viewvc/llvm-project?rev=351048&view=rev
Log:
[clang-tidy] update FunctionSizeCheck for D56444

Reviewers: JonasToth, aaron.ballman

Subscribers: xazax.hun, cfe-commits

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

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

Modified: clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp?rev=351048&r1=351047&r2=351048&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp Mon 
Jan 14 02:40:41 2019
@@ -145,7 +145,12 @@ void FunctionSizeCheck::storeOptions(Cla
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), 
this);
+  // Lambdas ignored - historically considered part of enclosing function.
+  // FIXME: include them instead? Top-level lambdas are currently never 
counted.
+  Finder->addMatcher(functionDecl(unless(isInstantiated()),
+  unless(cxxMethodDecl(ofClass(isLambda()
+ .bind("func"),
+ this);
 }
 
 void FunctionSizeCheck::check(const MatchFinder::MatchResult &Result) {


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

[PATCH] D56532: [clang-tidy] Add the abseil-duration-conversion-cast check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added a comment.

Ping.


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

https://reviews.llvm.org/D56532



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

[PATCH] D55256: [clangd] Support clang-tidy configuration in clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Just a small comment wrt to a particular change in `ClangdLSPServer`. I haven't 
looked at the patch more closely, though.




Comment at: clangd/ClangdLSPServer.h:132
 
-  RealFileSystemProvider FSProvider;
   /// Options used for code completion

Could we instead call `getRealFS()` when we try to initialize a clang-tidy 
options provider in `main()` and avoid changing this?
To avoid adding extra non-real-fs "modes of operation" to `ClangdLSPServer`. 
Unless you see other uses for this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55256



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

[PATCH] D56644: [clang-tidy] readability-container-size-empty handle std::string length()

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could you please run it over LLVM or another significant codebase and check if 
there are miss-transformations (run with fix and then compile (+ tests 
optimally))?
You can use `clang-tidy/tool/run-clang-tidy.py` for a parallel runner.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56644



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

[clang-tools-extra] r351051 - [clangd] Unlink VFS working dir from OS working dir.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Mon Jan 14 03:06:48 2019
New Revision: 351051

URL: http://llvm.org/viewvc/llvm-project?rev=351051&view=rev
Log:
[clangd] Unlink VFS working dir from OS working dir.

A lot of our previous FS manipulation was thread-unsafe in practice with
the RealFS implementation.

This switches to a different RealFS mode where path-manipulation is used
to simulate multiple working dirs.
r351050 both added this mode and removed the cache. If we want to
move back to the old implementation we need to put the cache back.

Modified:
clang-tools-extra/trunk/clangd/FSProvider.cpp

Modified: clang-tools-extra/trunk/clangd/FSProvider.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FSProvider.cpp?rev=351051&r1=351050&r2=351051&view=diff
==
--- clang-tools-extra/trunk/clangd/FSProvider.cpp (original)
+++ clang-tools-extra/trunk/clangd/FSProvider.cpp Mon Jan 14 03:06:48 2019
@@ -75,9 +75,10 @@ clang::clangd::RealFileSystemProvider::g
 // FIXME: Try to use a similar approach in Sema instead of relying on
 //propagation of the 'isVolatile' flag through all layers.
 #ifdef _WIN32
-  return new VolatileFileSystem(llvm::vfs::getRealFileSystem());
+  return new VolatileFileSystem(
+  llvm::vfs::createPhysicalFileSystem().release());
 #else
-  return llvm::vfs::getRealFileSystem();
+  return llvm::vfs::createPhysicalFileSystem().release();
 #endif
 }
 } // namespace clangd


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

[PATCH] D56656: [clangd] Fix a reference invalidation

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 181524.
kadircet added a comment.

- Add a comment


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56656

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -476,29 +476,33 @@
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
   std::vector Dependencies;
+  std::queue ToVisit;
   std::string AbsolutePath = getAbsolutePath(Cmd).str();
   // Up until we load the shard related to a dependency it needs to be
   // re-indexed.
-  Dependencies.emplace_back(AbsolutePath, true);
+  ToVisit.emplace(AbsolutePath, true);
   InQueue.insert(AbsolutePath);
   // Goes over each dependency.
-  for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
-   CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+  while (!ToVisit.empty()) {
+Dependencies.push_back(std::move(ToVisit.front()));
+// Dependencies is not modified during the rest of the loop, so it is safe
+// to keep the reference.
+auto &CurDependency = Dependencies.back();
+ToVisit.pop();
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one 
TU
 // to another.
-if (!LoadedShards.try_emplace(CurDependencyPath).second) {
+if (!LoadedShards.try_emplace(CurDependency.Path).second) {
   // If the dependency needs to be re-indexed, first occurence would 
already
   // have detected that, so we don't need to issue it again.
-  Dependencies[CurrentDependency].NeedsReIndexing = false;
+  CurDependency.NeedsReIndexing = false;
   continue;
 }
 
-auto Shard = IndexStorage->loadShard(CurDependencyPath);
+auto Shard = IndexStorage->loadShard(CurDependency.Path);
 if (!Shard || !Shard->Sources) {
   // File will be returned as requiring re-indexing to caller.
-  vlog("Failed to load shard: {0}", CurDependencyPath);
+  vlog("Failed to load shard: {0}", CurDependency.Path);
   continue;
 }
 // These are the edges in the include graph for current dependency.
@@ -506,34 +510,34 @@
   auto U = URI::parse(I.getKey());
   if (!U)
 continue;
-  auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
+  auto AbsolutePath = URI::resolve(*U, CurDependency.Path);
   if (!AbsolutePath)
 continue;
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
-Dependencies.emplace_back(*AbsolutePath, true);
+ToVisit.emplace(*AbsolutePath, true);
   // The node contains symbol information only for current file, the rest 
is
   // just edges.
-  if (*AbsolutePath != CurDependencyPath)
+  if (*AbsolutePath != CurDependency.Path)
 continue;
 
   // We found source file info for current dependency.
   assert(I.getValue().Digest != FileDigest{{0}} && "Digest is empty?");
   ShardInfo SI;
-  SI.AbsolutePath = CurDependencyPath;
+  SI.AbsolutePath = CurDependency.Path;
   SI.Shard = std::move(Shard);
   SI.Digest = I.getValue().Digest;
   IntermediateSymbols.push_back(std::move(SI));
   // Check if the source needs re-indexing.
   // Get the digest, skip it if file doesn't exist.
-  auto Buf = FS->getBufferForFile(CurDependencyPath);
+  auto Buf = FS->getBufferForFile(CurDependency.Path);
   if (!Buf) {
-elog("Couldn't get buffer for file: {0}: {1}", CurDependencyPath,
+elog("Couldn't get buffer for file: {0}: {1}", CurDependency.Path,
  Buf.getError().message());
 continue;
   }
   // If digests match then dependency doesn't need re-indexing.
-  Dependencies[CurrentDependency].NeedsReIndexing =
+  CurDependency.NeedsReIndexing =
   digest(Buf->get()->getBuffer()) != I.getValue().Digest;
 }
   }


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -476,29 +476,33 @@
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
   std::vector Dependencies;
+  std::queue ToVisit;
   std::string AbsolutePath = getAbsolutePath(Cmd).str();
   // Up until we load the shard related to a dependency it needs to be
   // re-indexed.
-  Dependencies.emplace_back(AbsolutePath, true);
+  ToVisit.emplace(AbsolutePath, true);
   InQueue.insert(AbsolutePath);
   // Goes over each dependency.
-  for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
-   CurrentDependency++) {
-ll

[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/XRefs.cpp:578
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
+if (!Invalid) {

malaperle wrote:
> ilya-biryukov wrote:
> > Unfortunately we can't get the buffer here, because for the preamble macros 
> > the files might change on disk after we build the preamble and we can end 
> > up reading a different version of the file. Which can in turn lead to 
> > crashes as the offsets obtained from the source locations can point outside 
> > the buffer for the corresponding file.
> > 
> > This is really annoying and generally the solution is to try find a 
> > different way to obtain the same information, e.g. by looking at what 
> > `MacroInfo` has available.
> > However, I don't know of a good workaround for this. Happy to look into 
> > ways of providing something close to a macro definition from `MacroInfo` or 
> > other sources, can scout for this tomorrow.
> I tried to do something like MacroInfo::dump. One problem is that we don't 
> always have enough information about literals. For example:
> 
> ```
> #define MACRO 0
> int main() { return MACRO; }
> ```
> Becomes:
> ```
> #define MACRO numeric_constant
> ```
> 
> I poked around the Token code and I didn't find a way to retrieve the literal 
> without going through the buffer (Preprocessor::getSpelling for example).
> 
> What you mention is only a problem when you use the option of storing 
> preambles on disk, right? So something would need to modify the preamble 
> files on disk, which are stored in some temp folder. It doesn't sound like 
> that could happen frequently. There is also bounds checking in the code above 
> so it shouldn't crash, right? Even if the bounds are incorrect, it will be no 
> different than the Range we return to the client for document/definition, 
> i.e. the client can use the range on a newly modified header and get it 
> wrong. I also tested renaming the pch file and then editing the source file 
> and it results in an invalid AST. So either way modifying the pch would be 
> bad news not for just macro hover.
The problem shows up when the header files are modified, not the preamble files 
(otherwise we'll be fine, in clangd we do assume the .pch files we produce are 
not externally modified).
Which is pretty frequent, and we've seen real crashes coming from fetching 
documentation from the preambles.

I'll try to investigate how Preprocessor accesses the tokens of macro 
definitions from preabmle. There are two possible outcomes I expect:
1. we find a way to access the spelling of the tokens in the same way PP does 
and it's safe,
2. we realize preprocessor might crash with the preabmle because it accesses 
the spelling of the tokens.

I hope to find (1), but (2) is also very realistic, unfortunately :-(


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250



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

[clang-tools-extra] r351052 - [clangd] Fix a reference invalidation

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Mon Jan 14 03:24:07 2019
New Revision: 351052

URL: http://llvm.org/viewvc/llvm-project?rev=351052&view=rev
Log:
[clangd] Fix a reference invalidation

Summary: Fix for the breakage in 
http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/52811/consoleFull#-42777206a1ca8a51-895e-46c6-af87-ce24fa4cd561

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

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

Modified:
clang-tools-extra/trunk/clangd/index/Background.cpp

Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=351052&r1=351051&r2=351052&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Mon Jan 14 03:24:07 2019
@@ -476,29 +476,33 @@ BackgroundIndex::loadShard(const tooling
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
   std::vector Dependencies;
+  std::queue ToVisit;
   std::string AbsolutePath = getAbsolutePath(Cmd).str();
   // Up until we load the shard related to a dependency it needs to be
   // re-indexed.
-  Dependencies.emplace_back(AbsolutePath, true);
+  ToVisit.emplace(AbsolutePath, true);
   InQueue.insert(AbsolutePath);
   // Goes over each dependency.
-  for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
-   CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+  while (!ToVisit.empty()) {
+Dependencies.push_back(std::move(ToVisit.front()));
+// Dependencies is not modified during the rest of the loop, so it is safe
+// to keep the reference.
+auto &CurDependency = Dependencies.back();
+ToVisit.pop();
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one 
TU
 // to another.
-if (!LoadedShards.try_emplace(CurDependencyPath).second) {
+if (!LoadedShards.try_emplace(CurDependency.Path).second) {
   // If the dependency needs to be re-indexed, first occurence would 
already
   // have detected that, so we don't need to issue it again.
-  Dependencies[CurrentDependency].NeedsReIndexing = false;
+  CurDependency.NeedsReIndexing = false;
   continue;
 }
 
-auto Shard = IndexStorage->loadShard(CurDependencyPath);
+auto Shard = IndexStorage->loadShard(CurDependency.Path);
 if (!Shard || !Shard->Sources) {
   // File will be returned as requiring re-indexing to caller.
-  vlog("Failed to load shard: {0}", CurDependencyPath);
+  vlog("Failed to load shard: {0}", CurDependency.Path);
   continue;
 }
 // These are the edges in the include graph for current dependency.
@@ -506,34 +510,34 @@ BackgroundIndex::loadShard(const tooling
   auto U = URI::parse(I.getKey());
   if (!U)
 continue;
-  auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
+  auto AbsolutePath = URI::resolve(*U, CurDependency.Path);
   if (!AbsolutePath)
 continue;
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
-Dependencies.emplace_back(*AbsolutePath, true);
+ToVisit.emplace(*AbsolutePath, true);
   // The node contains symbol information only for current file, the rest 
is
   // just edges.
-  if (*AbsolutePath != CurDependencyPath)
+  if (*AbsolutePath != CurDependency.Path)
 continue;
 
   // We found source file info for current dependency.
   assert(I.getValue().Digest != FileDigest{{0}} && "Digest is empty?");
   ShardInfo SI;
-  SI.AbsolutePath = CurDependencyPath;
+  SI.AbsolutePath = CurDependency.Path;
   SI.Shard = std::move(Shard);
   SI.Digest = I.getValue().Digest;
   IntermediateSymbols.push_back(std::move(SI));
   // Check if the source needs re-indexing.
   // Get the digest, skip it if file doesn't exist.
-  auto Buf = FS->getBufferForFile(CurDependencyPath);
+  auto Buf = FS->getBufferForFile(CurDependency.Path);
   if (!Buf) {
-elog("Couldn't get buffer for file: {0}: {1}", CurDependencyPath,
+elog("Couldn't get buffer for file: {0}: {1}", CurDependency.Path,
  Buf.getError().message());
 continue;
   }
   // If digests match then dependency doesn't need re-indexing.
-  Dependencies[CurrentDependency].NeedsReIndexing =
+  CurDependency.NeedsReIndexing =
   digest(Buf->get()->getBuffer()) != I.getValue().Digest;
 }
   }


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

[PATCH] D56656: [clangd] Fix a reference invalidation

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE351052: [clangd] Fix a reference invalidation (authored by 
kadircet, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56656?vs=181524&id=181525#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56656

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -476,29 +476,33 @@
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
   std::vector Dependencies;
+  std::queue ToVisit;
   std::string AbsolutePath = getAbsolutePath(Cmd).str();
   // Up until we load the shard related to a dependency it needs to be
   // re-indexed.
-  Dependencies.emplace_back(AbsolutePath, true);
+  ToVisit.emplace(AbsolutePath, true);
   InQueue.insert(AbsolutePath);
   // Goes over each dependency.
-  for (size_t CurrentDependency = 0; CurrentDependency < Dependencies.size();
-   CurrentDependency++) {
-llvm::StringRef CurDependencyPath = Dependencies[CurrentDependency].Path;
+  while (!ToVisit.empty()) {
+Dependencies.push_back(std::move(ToVisit.front()));
+// Dependencies is not modified during the rest of the loop, so it is safe
+// to keep the reference.
+auto &CurDependency = Dependencies.back();
+ToVisit.pop();
 // If we have already seen this shard before(either loaded or failed) don't
 // re-try again. Since the information in the shard won't change from one 
TU
 // to another.
-if (!LoadedShards.try_emplace(CurDependencyPath).second) {
+if (!LoadedShards.try_emplace(CurDependency.Path).second) {
   // If the dependency needs to be re-indexed, first occurence would 
already
   // have detected that, so we don't need to issue it again.
-  Dependencies[CurrentDependency].NeedsReIndexing = false;
+  CurDependency.NeedsReIndexing = false;
   continue;
 }
 
-auto Shard = IndexStorage->loadShard(CurDependencyPath);
+auto Shard = IndexStorage->loadShard(CurDependency.Path);
 if (!Shard || !Shard->Sources) {
   // File will be returned as requiring re-indexing to caller.
-  vlog("Failed to load shard: {0}", CurDependencyPath);
+  vlog("Failed to load shard: {0}", CurDependency.Path);
   continue;
 }
 // These are the edges in the include graph for current dependency.
@@ -506,34 +510,34 @@
   auto U = URI::parse(I.getKey());
   if (!U)
 continue;
-  auto AbsolutePath = URI::resolve(*U, CurDependencyPath);
+  auto AbsolutePath = URI::resolve(*U, CurDependency.Path);
   if (!AbsolutePath)
 continue;
   // Add file as dependency if haven't seen before.
   if (InQueue.try_emplace(*AbsolutePath).second)
-Dependencies.emplace_back(*AbsolutePath, true);
+ToVisit.emplace(*AbsolutePath, true);
   // The node contains symbol information only for current file, the rest 
is
   // just edges.
-  if (*AbsolutePath != CurDependencyPath)
+  if (*AbsolutePath != CurDependency.Path)
 continue;
 
   // We found source file info for current dependency.
   assert(I.getValue().Digest != FileDigest{{0}} && "Digest is empty?");
   ShardInfo SI;
-  SI.AbsolutePath = CurDependencyPath;
+  SI.AbsolutePath = CurDependency.Path;
   SI.Shard = std::move(Shard);
   SI.Digest = I.getValue().Digest;
   IntermediateSymbols.push_back(std::move(SI));
   // Check if the source needs re-indexing.
   // Get the digest, skip it if file doesn't exist.
-  auto Buf = FS->getBufferForFile(CurDependencyPath);
+  auto Buf = FS->getBufferForFile(CurDependency.Path);
   if (!Buf) {
-elog("Couldn't get buffer for file: {0}: {1}", CurDependencyPath,
+elog("Couldn't get buffer for file: {0}: {1}", CurDependency.Path,
  Buf.getError().message());
 continue;
   }
   // If digests match then dependency doesn't need re-indexing.
-  Dependencies[CurrentDependency].NeedsReIndexing =
+  CurDependency.NeedsReIndexing =
   digest(Buf->get()->getBuffer()) != I.getValue().Digest;
 }
   }


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -476,29 +476,33 @@
   // Dependencies of this TU, paired with the information about whether they
   // need to be re-indexed or not.
   std::vector Dependencies;
+  std::queue ToVisit;
   std::string AbsolutePath = getAbsolutePath(Cmd).str();
   // Up until we load the shard related to a dependency it needs to be
   // re-indexed.
-  Dependencies.emplace_back(AbsolutePath, true);
+  ToVisit.emplace(AbsolutePath, true);
   InQueu

[PATCH] D56597: [clangd] Add Limit parameter for xref.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/XRefs.cpp:724
   }
-
+  if (!Limit)
+Limit = std::numeric_limits::max();

nit: move this to the top, since it's just adjusting input params?



Comment at: clangd/XRefs.cpp:730
   for (const auto &Ref : MainFileRefs) {
+if (Results.size() == Limit)
+  break;

I don't think we need to break up the flow here to early exit.

I'd suggest reducing the number of places we deal with limits, we can get away 
with 3 instead of 5:
```
... collect results as before ...
if (Index && Limit && Results.size() < Limit) {
  ... query index ...
  Req.Limit = Limit;
  ... append results as usual ...
}
if (Limit && Results.size() > Limit)
  Results.resize(limit)
```



Comment at: clangd/index/MemIndex.cpp:72
   trace::Span Tracer("MemIndex refs");
+  uint32_t Limit =
+  Req.Limit ? *Req.Limit : std::numeric_limits::max();

Req.Limit.getValueOr(...)



Comment at: clangd/index/MemIndex.cpp:72
   trace::Span Tracer("MemIndex refs");
+  uint32_t Limit =
+  Req.Limit ? *Req.Limit : std::numeric_limits::max();

sammccall wrote:
> Req.Limit.getValueOr(...)
if you're going to mutate this, maybe call it "remaining" or so?



Comment at: clangd/index/Merge.cpp:98
   // Ultimately we should explicit check which index has the file instead.
+  uint32_t DynamicCount = 0;
+  uint32_t Limit =

why two separate count variables?



Comment at: clangd/index/Merge.cpp:98
   // Ultimately we should explicit check which index has the file instead.
+  uint32_t DynamicCount = 0;
+  uint32_t Limit =

sammccall wrote:
> why two separate count variables?
you've inserted the new code between existing code and the comment that 
describes it.



Comment at: unittests/clangd/DexTests.cpp:673
 
-  std::vector Files;
-  RefsRequest Req;
-  Req.IDs.insert(Foo.ID);
-  Req.Filter = RefKind::Declaration | RefKind::Definition;
-  Dex(std::vector{Foo, Bar}, Refs).refs(Req, [&](const Ref &R) {
-Files.push_back(R.Location.FileURI);
-  });
-
-  EXPECT_THAT(Files, ElementsAre("foo.h"));
+  {
+std::vector Files;

If you want these to be the same test, there should be some connection between 
them.
A natural one is that it's the same query with a different limit - you could 
extend the original test to have multiple results.



Comment at: unittests/clangd/DexTests.cpp:686
+Req.IDs.insert(Foo.ID);
+Req.Limit = 1;
+size_t RefCount = 0;

As far as I can tell, this limit makes no difference, there's only one result 
anyway.



Comment at: unittests/clangd/DexTests.cpp:691
+});
+EXPECT_THAT(*Req.Limit, RefCount);
+  }

In tests, we know what the number is, and it's clearer to just spell it out: 
`EXPECT_EQ(1, RefCount)`



Comment at: unittests/clangd/DexTests.cpp:691
+});
+EXPECT_THAT(*Req.Limit, RefCount);
+  }

sammccall wrote:
> In tests, we know what the number is, and it's clearer to just spell it out: 
> `EXPECT_EQ(1, RefCount)`
generally prefer to use EXPECT_THAT only with matchers. EXPECT_EQ gives clearer 
error messages, reads more naturally, and is more common.

Better still would be to assert the actual outcomes, e.g. 
ElementsAre(anyOf("foo1.h", "foo2.h"))



Comment at: unittests/clangd/IndexTests.cpp:306
+  {
+Request.Limit = 1;
+size_t RefsCount = 0;

why new scope here?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56597



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

r351053 - [OpenCL] Set generic addr space of 'this' in special class members.

[Anastasia Stulova via cfe-commits]

Author: stulova
Date: Mon Jan 14 03:44:22 2019
New Revision: 351053

URL: http://llvm.org/viewvc/llvm-project?rev=351053&view=rev
Log:
[OpenCL] Set generic addr space of 'this' in special class members.

Set address spaces of 'this' param correctly for implicit special
class members.

This also changes initialization conversion sequence to separate
address space conversion from other qualifiers in case of binding
reference to a temporary. In this case address space conversion  
should happen after the binding (unlike for other quals). This is
needed to materialize it correctly in the alloca address space.

Initial patch by Mikael Nilssoni!

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


Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/AST/Expr.cpp
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Sema/SemaInit.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/CodeGenOpenCLCXX/addrspace-of-this.cl
cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=351053&r1=351052&r2=351053&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Jan 14 03:44:22 2019
@@ -307,6 +307,10 @@ class Sema {
   }
   bool shouldLinkPossiblyHiddenDecl(LookupResult &Old, const NamedDecl *New);
 
+  void setupImplicitSpecialMemberType(CXXMethodDecl *SpecialMem,
+  QualType ResultTy,
+  ArrayRef Args);
+
 public:
   typedef OpaquePtr DeclGroupPtrTy;
   typedef OpaquePtr TemplateTy;

Modified: cfe/trunk/lib/AST/Expr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Expr.cpp?rev=351053&r1=351052&r2=351053&view=diff
==
--- cfe/trunk/lib/AST/Expr.cpp (original)
+++ cfe/trunk/lib/AST/Expr.cpp Mon Jan 14 03:44:22 2019
@@ -1677,10 +1677,10 @@ bool CastExpr::CastConsistency() const {
 auto Ty = getType();
 auto SETy = getSubExpr()->getType();
 assert(getValueKindForType(Ty) == Expr::getValueKindForType(SETy));
-if (!isGLValue())
+if (isRValue()) {
   Ty = Ty->getPointeeType();
-if (!isGLValue())
   SETy = SETy->getPointeeType();
+}
 assert(!Ty.isNull() && !SETy.isNull() &&
Ty.getAddressSpace() != SETy.getAddressSpace());
 goto CheckNoBasePath;

Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=351053&r1=351052&r2=351053&view=diff
==
--- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCall.cpp Mon Jan 14 03:44:22 2019
@@ -74,7 +74,7 @@ static CanQualType GetThisType(ASTContex
const CXXMethodDecl *MD) {
   QualType RecTy = Context.getTagDeclType(RD)->getCanonicalTypeInternal();
   if (MD)
-RecTy = Context.getAddrSpaceQualType(RecTy, 
MD->getType().getAddressSpace());
+RecTy = Context.getAddrSpaceQualType(RecTy, 
MD->getTypeQualifiers().getAddressSpace());
   return Context.getPointerType(CanQualType::CreateUnsafe(RecTy));
 }
 

Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=351053&r1=351052&r2=351053&view=diff
==
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Mon Jan 14 03:44:22 2019
@@ -3192,12 +3192,7 @@ bool Sema::MergeFunctionDecl(FunctionDec
   if (RequiresAdjustment) {
 const FunctionType *AdjustedType = New->getType()->getAs();
 AdjustedType = Context.adjustFunctionType(AdjustedType, NewTypeInfo);
-
-QualType AdjustedQT = QualType(AdjustedType, 0);
-LangAS AS = Old->getType().getAddressSpace();
-AdjustedQT = Context.getAddrSpaceQualType(AdjustedQT, AS);
-
-New->setType(AdjustedQT);
+New->setType(QualType(AdjustedType, 0));
 NewQType = Context.getCanonicalType(New->getType());
 NewType = cast(NewQType);
   }

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=351053&r1=351052&r2=351053&view=diff
==
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Jan 14 03:44:22 2019
@@ -6551,8 +6551,11 @@ void Sema::CheckExplicitlyDefaultedSpeci
   if (CSM == CXXCopyAssignment || CSM == CXXMoveAssignment) {
 // Check for return type matching.
 ReturnType = Type->getReturnType();
-QualType ExpectedReturnType =
-Context.getLValueRefe

[PATCH] D56066: [OpenCL] Address space for default class members

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351053: [OpenCL] Set generic addr space of 'this' 
in special class members. (authored by stulova, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56066?vs=181256&id=181526#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56066

Files:
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/CodeGen/CGCall.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaDeclCXX.cpp
  cfe/trunk/lib/Sema/SemaInit.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/CodeGenOpenCLCXX/addrspace-of-this.cl
  cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl

Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -307,6 +307,10 @@
   }
   bool shouldLinkPossiblyHiddenDecl(LookupResult &Old, const NamedDecl *New);
 
+  void setupImplicitSpecialMemberType(CXXMethodDecl *SpecialMem,
+  QualType ResultTy,
+  ArrayRef Args);
+
 public:
   typedef OpaquePtr DeclGroupPtrTy;
   typedef OpaquePtr TemplateTy;
Index: cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
===
--- cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
+++ cfe/trunk/test/SemaOpenCLCXX/address-space-templates.cl
@@ -4,9 +4,7 @@
 struct S {
   T a;// expected-error{{field may not be qualified with an address space}}
   T f1(); // expected-error{{function type may not be qualified with an address space}}
-  // FIXME: Should only get the error message once.
-  void f2(T); // expected-error{{parameter may not be qualified with an address space}} expected-error{{parameter may not be qualified with an address space}}
-
+  void f2(T); // expected-error{{parameter may not be qualified with an address space}}
 };
 
 template 
Index: cfe/trunk/test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- cfe/trunk/test/CodeGenOpenCLCXX/addrspace-of-this.cl
+++ cfe/trunk/test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -9,18 +9,21 @@
 public:
   int v;
   C() { v = 2; }
-  // FIXME: Does not work yet.
-  // C(C &&c) { v = c.v; }
+  C(C &&c) { v = c.v; }
   C(const C &c) { v = c.v; }
   C &operator=(const C &c) {
 v = c.v;
 return *this;
   }
-  // FIXME: Does not work yet.
-  //C &operator=(C&& c) & {
-  //  v = c.v;
-  //  return *this;
-  //}
+  C &operator=(C &&c) & {
+v = c.v;
+return *this;
+  }
+
+  C operator+(const C& c) {
+v += c.v;
+return *this;
+  }
 
   int get() { return v; }
 
@@ -41,15 +44,13 @@
   C c1(c);
   C c2;
   c2 = c1;
-  // FIXME: Does not work yet.
-  // C c3 = c1 + c2;
-  // C c4(foo());
-  // C c5 = foo();
-
+  C c3 = c1 + c2;
+  C c4(foo());
+  C c5 = foo();
 }
 
 // CHECK-LABEL: @__cxx_global_var_init()
-// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
 
 // Test that the address space is __generic for the constructor
 // CHECK-LABEL: @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %this)
@@ -57,7 +58,7 @@
 // CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
 // CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
 // CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
-// CHECK:   call void @_ZNU3AS41CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   call void @_ZNU3AS41CC2Ev(%class.C addrspace(4)* %this1)
 // CHECK:   ret void
 
 // CHECK-LABEL: @_Z12test__globalv()
@@ -69,17 +70,36 @@
 // CHECK: %call1 = call i32 @_ZNU3AS41C7outsideEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
 
 // Test the address space of 'this' when invoking copy-constructor.
-// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
-// CHECK: call void @_ZNU3AS41CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+// CHECK: [[C1GEN:%[0-9]+]] = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK: call void @_ZNU3AS41CC1ERU3AS4KS_(%class.C addrspace(4)* [[C1GEN]], %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
 
 // Test the address space of 'this' when invoking a constructor.
-// CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
-// CHECK:   call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %1) #4
+// CHECK

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Marina Kalashina via Phabricator via cfe-commits]

MarinaKalashina updated this revision to Diff 181528.
MarinaKalashina marked an inline comment as done.
MarinaKalashina added a comment.

files renamed: //contribution.rst// to //Contributing.rst//, // 
integrations.rst// to //Integrations.rst//


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

https://reviews.llvm.org/D54945

Files:
  docs/clang-tidy/Contributing.rst
  docs/clang-tidy/Integrations.rst
  docs/clang-tidy/index.rst

Index: docs/clang-tidy/Integrations.rst
===
--- /dev/null
+++ docs/clang-tidy/Integrations.rst
@@ -0,0 +1,117 @@
+==
+Clang-tidy IDE/Editor Integrations
+==
+
+.. _Clangd: https://clang.llvm.org/extra/clangd.html
+
+Apart from being a standalone tool, :program:`clang-tidy` is integrated into
+various IDEs, code analyzers, and editors. Besides, it is currently being
+integrated into Clangd_. The following table shows the most
+well-known :program:`clang-tidy` integrations in detail.
+
++--++-+--+-+--+
+|  |Feature   |
++==++=+==+=+==+
+|  **Tool**| On-the-fly inspection  | Check list configuration (GUI)  | Options to checks (GUI)  | Configuration via ``.clang-tidy`` files | Custom clang-tidy binary |
++--++-+--+-+--+
+|A.L.E. for Vim| \+\|   \-\   |   \-\| \-\ |   \+\|
++--++-+--+-+--+
+|Clang Power Tools for Visual Studio   | \-\|   \+\   |   \-\| \+\ |   \-\|
++--++-+--+-+--+
+|Clangd| \+\|   \-\   |   \-\| \-\ |   \-\|
++--++-+--+-+--+
+|CLion IDE | \+\|   \+\   |   \+\| \+\ |   \+\|
++--++-+--+-+--+
+|CodeChecker   | \-\|   \-\   |   \-\| \-\ |   \+\|
++--++-+--+-+--+
+|CPPCheck  | \-\|   \-\   |   \-\| \-\ |   \-\|
++--++-+--+-+--+
+|CPPDepend | \-\|   \-\   |   \-\| \-\ |   \-\|
++--++-+--+-+--+
+|Flycheck for Emacs| \+\|   \-\   |   \-\| \+\ |   \+\   

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Marina Kalashina via Phabricator via cfe-commits]

MarinaKalashina added a comment.

@Eugene.Zelenko

> I would suggest to rename contribution to Contributing (see LLVM 
> documentation) and integrations to Integrations.

Sure, done.

> Will be also good idea to fix D55523  script 
> warnings.

The script warns about double spaces and line width in the table, for example:

  warning: line 39 contains double spaces
  warning: line 39 is in excess of 80 characters (196)

These spaces and width are intended since they 'draw' the table layout. Do you 
think we could ignore such warnings in this particular case?


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

https://reviews.llvm.org/D54945



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

[PATCH] D56658: [MSP430] Add msp430 toochain

[Kristina Bessonova via Phabricator via cfe-commits]

krisb created this revision.
Herald added subscribers: cfe-commits, arichardson, mgorny, emaste.
Herald added a reviewer: espindola.

This is an initial implementation for msp430 toolchain including

- -mmcu option support
- -mhwmult options support
- -integrated-as by default
- msp430-elf-as as a linker

Reviewiers: asl


Repository:
  rC Clang

https://reviews.llvm.org/D56658

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Basic/MSP430Target.def
  include/clang/Driver/Options.td
  include/clang/module.modulemap
  lib/Driver/CMakeLists.txt
  lib/Driver/Driver.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MSP430.cpp
  lib/Driver/ToolChains/MSP430.h
  test/Driver/Inputs/basic_msp430_tree/bin/msp430-elf-ld
  test/Driver/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430/crtbegin.o
  test/Driver/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430/crtend.o
  test/Driver/Inputs/basic_msp430_tree/msp430-elf/lib/430/crt0.o
  test/Driver/Inputs/basic_msp430_tree/msp430-elf/lib/430/crtn.o
  test/Driver/msp430-hwmult.c
  test/Driver/msp430-mmcu.c
  test/Driver/msp430-toolchain.c
  test/Driver/no-integrated-as.c

Index: test/Driver/no-integrated-as.c
===
--- test/Driver/no-integrated-as.c
+++ test/Driver/no-integrated-as.c
@@ -9,11 +9,6 @@
 // IAS-NOT: -no-integrated-as
 
 // RUN: %clang -target i386 -### -c %s 2>&1 | FileCheck %s -check-prefix DEFAULT
+// RUN: %clang -target msp430 -### -c %s 2>&1 | FileCheck %s -check-prefix DEFAULT
 
 // DEFAULT-NOT: -no-integrated-as
-
-// RUN: %clang -target msp430 -### -c %s 2>&1 \
-// RUN: | FileCheck %s -check-prefix NO-IAS-DEFAULT
-
-// NO-IAS-DEFAULT: -no-integrated-as
-
Index: test/Driver/msp430-toolchain.c
===
--- /dev/null
+++ test/Driver/msp430-toolchain.c
@@ -0,0 +1,78 @@
+// A basic clang -cc1 command-line, and simple environment check.
+
+// RUN: %clang %s -### -no-canonical-prefixes -target msp430 2>&1 \
+// RUN:   | FileCheck -check-prefix=CC1 %s
+// CC1: clang{{.*}} "-cc1" "-triple" "msp430"
+
+// RUN: %clang %s -### -no-canonical-prefixes -target msp430 \
+// RUN:   --gcc-toolchain=%S/Inputs/basic_msp430_tree 2>&1 \
+// RUN:   | FileCheck -check-prefix=MSP430 %s
+
+// MSP430: "{{.*}}Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../bin/msp430-elf-ld"
+// MSP430: "-L{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430"
+// MSP430: "-L{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../msp430-elf/lib/430"
+// MSP430: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../msp430-elf/lib/430/crt0.o"
+// MSP430: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430/crtbegin.o"
+// MSP430: "--start-group" "-lmul_none" "-lgcc" "-lc" "-lcrt" "-lnosys" "--end-group"
+// MSP430: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430/crtend.o"
+// MSP430: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../msp430-elf/lib/430/crtn.o"
+
+// RUN: %clang %s -### -no-canonical-prefixes -target msp430 -nodefaultlibs \
+// RUN:   --gcc-toolchain=%S/Inputs/basic_msp430_tree 2>&1 \
+// RUN:   | FileCheck -check-prefix=MSP430-NO-DFT-LIB %s
+
+// MSP430-NO-DFT-LIB: "{{.*}}Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../bin/msp430-elf-ld"
+// MSP430-NO-DFT-LIB: "-L{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430"
+// MSP430-NO-DFT-LIB: "-L{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../msp430-elf/lib/430"
+// MSP430-NO-DFT-LIB: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../msp430-elf/lib/430/crt0.o"
+// MSP430-NO-DFT-LIB: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430/crtbegin.o"
+// MSP430-NO-DFT-LIB: "--start-group" "-lmul_none" "-lgcc" "--end-group"
+// MSP430-NO-DFT-LIB: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430/crtend.o"
+// MSP430-NO-DFT-LIB: "{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../msp430-elf/lib/430/crtn.o"
+
+// RUN: %clang %s -### -no-canonical-prefixes -target msp430 -nostartfiles \
+// RUN:   --gcc-toolchain=%S/Inputs/basic_msp430_tree 2>&1 \
+// RUN:   | FileCheck -check-prefix=MSP430-NO-START %s
+
+// MSP430-NO-START: "{{.*}}Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../bin/msp430-elf-ld"
+// MSP430-NO-START: "-L{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/430"
+// MSP430-NO-START: "-L{{.*}}/Inputs/basic_msp430_tree/lib/gcc/msp430-elf/7.3.1/../../../../msp430-elf/lib/430"
+// MSP430-NO-START: "--start-group" "-lmul_none" "-lgcc" "-lc" "-lcrt" "-lnosys" "--end-group"
+
+// RUN: %clang %s -### -no-canonical-prefixes -target msp430 -nostdlib \
+// RUN:   --gcc-toolchain=%S/Inputs/basic_msp430_tree 2>&1 \
+// RUN:   | FileCheck -check-prefix=MSP430-NO-STD-LIB %s
+
+// MSP430-NO-STD-LIB: "{{.*}}Inputs/basic_msp430_tree/lib/gcc/ms

[PATCH] D56488: clang-cl: Align help texts for /O1 and O2

[Hans Wennborg via Phabricator via cfe-commits]

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

lgtm, thanks


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

https://reviews.llvm.org/D56488



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

[PATCH] D56489: clang-cl: Fix help text for /O: '/O2y-' means '/O2 /Oy-', not '/O2 /y-'

[Hans Wennborg via Phabricator via cfe-commits]

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

thanks


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

https://reviews.llvm.org/D56489



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

[PATCH] D56488: clang-cl: Align help texts for /O1 and O2

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351061: clang-cl: Align help texts for /O1 and O2 (authored 
by nico, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56488?vs=180832&id=181533#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56488

Files:
  cfe/trunk/include/clang/Driver/CLCompatOptions.td


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for size  (same as /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
@@ -143,7 +143,7 @@
 def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>,
   HelpText<"Optimize for speed">;
 def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>,
-  HelpText<"Deprecated (equivalent to /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
+  HelpText<"Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
 def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>,
   HelpText<"Enable frame pointer omission (x86 only)">;
 def : CLFlag<"Oy-">, Alias<_SLASH_O>, AliasArgs<["y-"]>,


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for size  (same as /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
@@ -143,7 +143,7 @@
 def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>,
   HelpText<"Optimize for speed">;
 def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>,
-  HelpText<"Deprecated (equivalent to /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
+  HelpText<"Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
 def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>,
   HelpText<"Enable frame pointer omission (x86 only)">;
 def : CLFlag<"Oy-">, Alias<_SLASH_O>, AliasArgs<["y-"]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r351061 - clang-cl: Align help texts for /O1 and O2

[Nico Weber via cfe-commits]

Author: nico
Date: Mon Jan 14 04:41:13 2019
New Revision: 351061

URL: http://llvm.org/viewvc/llvm-project?rev=351061&view=rev
Log:
clang-cl: Align help texts for /O1 and O2

Makes it a bit easier to see what exactly the difference is.

Also use "same as" instead of "equivalent to", because that's faster to read.

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

Modified:
cfe/trunk/include/clang/Driver/CLCompatOptions.td

Modified: cfe/trunk/include/clang/Driver/CLCompatOptions.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CLCompatOptions.td?rev=351061&r1=351060&r2=351061&view=diff
==
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td (original)
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td Mon Jan 14 04:41:13 2019
@@ -121,9 +121,9 @@ def _SLASH_O : CLJoined<"O">,
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for size  (same as /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
+  HelpText<"Optimize for speed (same as /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
@@ -143,7 +143,7 @@ def : CLFlag<"Os">, Alias<_SLASH_O>, Ali
 def : CLFlag<"Ot">, Alias<_SLASH_O>, AliasArgs<["t"]>,
   HelpText<"Optimize for speed">;
 def : CLFlag<"Ox">, Alias<_SLASH_O>, AliasArgs<["x"]>,
-  HelpText<"Deprecated (equivalent to /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
+  HelpText<"Deprecated (same as /Og /Oi /Ot /Oy /Ob2); use /O2 instead">;
 def : CLFlag<"Oy">, Alias<_SLASH_O>, AliasArgs<["y"]>,
   HelpText<"Enable frame pointer omission (x86 only)">;
 def : CLFlag<"Oy-">, Alias<_SLASH_O>, AliasArgs<["y-"]>,


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

[PATCH] D56489: clang-cl: Fix help text for /O: '/O2y-' means '/O2 /Oy-', not '/O2 /y-'

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351062: clang-cl: Fix help text for /O: 
'/O2y-' means '/O2 /Oy-', not '/O2 /y-' (authored 
by nico, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56489?vs=180833&id=181534#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56489

Files:
  cfe/trunk/include/clang/Driver/CLCompatOptions.td


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -116,7 +116,7 @@
 // The _SLASH_O option handles all the /O flags, but we also provide separate
 // aliased options to provide separate help messages.
 def _SLASH_O : CLJoined<"O">,
-  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' is the same as '/O2 
/y-'">,
+  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' for '/O2 /Oy-'">,
   MetaVarName<"">;
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -116,7 +116,7 @@
 // The _SLASH_O option handles all the /O flags, but we also provide separate
 // aliased options to provide separate help messages.
 def _SLASH_O : CLJoined<"O">,
-  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' is the same as '/O2 /y-'">,
+  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' for '/O2 /Oy-'">,
   MetaVarName<"">;
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r351062 - clang-cl: Fix help text for /O: '/O2y-' means '/O2 /Oy-', not '/O2 /y-'

[Nico Weber via cfe-commits]

Author: nico
Date: Mon Jan 14 04:42:35 2019
New Revision: 351062

URL: http://llvm.org/viewvc/llvm-project?rev=351062&view=rev
Log:
clang-cl: Fix help text for /O: '/O2y-' means '/O2 /Oy-', not '/O2 /y-'

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

Modified:
cfe/trunk/include/clang/Driver/CLCompatOptions.td

Modified: cfe/trunk/include/clang/Driver/CLCompatOptions.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CLCompatOptions.td?rev=351062&r1=351061&r2=351062&view=diff
==
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td (original)
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td Mon Jan 14 04:42:35 2019
@@ -116,7 +116,7 @@ def _SLASH_J : CLFlag<"J">, HelpText<"Ma
 // The _SLASH_O option handles all the /O flags, but we also provide separate
 // aliased options to provide separate help messages.
 def _SLASH_O : CLJoined<"O">,
-  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' is the same as '/O2 
/y-'">,
+  HelpText<"Set multiple /O flags at once; e.g. '/O2y-' for '/O2 /Oy-'">,
   MetaVarName<"">;
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;


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

[PATCH] D56592: [clangd] Do not override contents of the shards without modification

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 181540.
kadircet marked an inline comment as done.
kadircet added a comment.

- Change the logic to detect updated files to take new files without any 
symbols into account.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56592

Files:
  clangd/index/Background.cpp
  clangd/index/Background.h
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -351,11 +351,82 @@
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
-  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardSource, nullptr);
   EXPECT_THAT(*ShardSource->Symbols,
   Contains(AllOf(Named("f_b"), Declared(), Defined(;
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageEmptyFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/B.h")] = R"cpp(
+  #include "A.h"
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"B.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check that A.cc, A.h and B.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_THAT(Storage.keys(),
+  UnorderedElementsAre(testPath("root/A.cc"), testPath("root/A.h"),
+   testPath("root/B.h")));
+  auto ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_TRUE(ShardHeader->Symbols->empty());
+
+  // Check that A.cc, A.h and B.h has been loaded.
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+
+  // Update B.h to contain some symbols.
+  FS.Files[testPath("root/B.h")] = R"cpp(
+  #include "A.h"
+  void new_func();
+  )cpp";
+  // Check that B.h has been stored with new contents.
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+  ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols,
+  Contains(AllOf(Named("new_func"), Declared(), Not(Defined();
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -91,10 +91,12 @@
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
 private:
-  /// Given index results from a TU, only update files in \p FilesToUpdate.
-  /// Also stores new index information on IndexStorage.
+  /// Given index results from a TU, only update symbols coming from files in \p
+  /// FilesToUpdate and skips empty files if they are up-to-date. Also stores
+  /// new index information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
   const llvm::StringMap &FilesToUpdate,
+  const llvm::StringMap &DigestsSnapshot,
   BackgroundIndexStorage *IndexStorage);
 
   // configuration
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -264,9 +264,12 @@
   QueueCV.notify_all();
 }
 
-/// Given index results from a TU, only update files in \p FilesToUpdate.
+/// Given index results from a TU, only update symbols coming from files in \p
+/// FilesToUpdate and skips empty files if they are up-to-date. Also stores new
+/// index information on Ind

[PATCH] D56661: [clang-tidy] Fix incorrect array name generation in cppcoreguidelines-pro-bounds-constant-array-index

[Dmitry Venikov via Phabricator via cfe-commits]

Quolyk created this revision.
Quolyk added reviewers: aaron.ballman, alexfh, JonasToth, omtcyfz.
Herald added subscribers: cfe-commits, arphaman, kbarton, xazax.hun, nemanjai.

This patch fixes incorrect array name generation for a 
cppcoreguidelines-pro-bounds-constant-array-index warning.
Motivation: https://bugs.llvm.org/show_bug.cgi?id=38510.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D56661

Files:
  
test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp
  test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp


Index: test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
===
--- test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
+++ test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
@@ -47,12 +47,12 @@
 }
 
 void g() {
-  int a[10];
+  int list[10];
   for (int i = 0; i < 10; ++i) {
-a[i] = i;
+list[i] = i;
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript 
when the index is not an integer constant expression; use gsl::at() instead
-// CHECK-FIXES: gsl::at(a, i) = i;
-gsl::at(a, i) = i; // OK, gsl::at() instead of []
+// CHECK-FIXES: gsl::at(list, i) = i;
+gsl::at(list, i) = i; // OK, gsl::at() instead of []
   }
 
   a[-1] = 3; // flagged by clang-diagnostic-array-bounds
Index: 
test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp
===
--- 
test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp
+++ 
test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index-gslheader.cpp
@@ -24,48 +24,48 @@
   return base + 3;
 }
 
-void f(std::array a, int pos) {
-  a [ pos / 2 /*comment*/] = 1;
+void f(std::array list, int pos) {
+  list [ pos / 2 /*comment*/] = 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use array subscript when 
the index is not an integer constant expression; use gsl::at() instead 
[cppcoreguidelines-pro-bounds-constant-array-index]
-  // CHECK-FIXES: gsl::at(a,  pos / 2 /*comment*/) = 1;
-  int j = a[pos - 1];
+  // CHECK-FIXES: gsl::at(list,  pos / 2 /*comment*/) = 1;
+  int j = list[pos - 1];
   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not use array subscript when 
the index is not an integer constant expression; use gsl::at() instead
-  // CHECK-FIXES: int j = gsl::at(a, pos - 1);
+  // CHECK-FIXES: int j = gsl::at(list, pos - 1);
 
-  a.at(pos-1) = 2; // OK, at() instead of []
-  gsl::at(a, pos-1) = 2; // OK, gsl::at() instead of []
+  list.at(pos-1) = 2; // OK, at() instead of []
+  gsl::at(list, pos-1) = 2; // OK, gsl::at() instead of []
 
-  a[-1] = 3;
+  list[-1] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index -1 is 
negative [cppcoreguidelines-pro-bounds-constant-array-index]
-  a[10] = 4;
+  list[10] = 4;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past 
the end of the array (which contains 10 elements) 
[cppcoreguidelines-pro-bounds-constant-array-index]
 
-  a[const_index(7)] = 3;
+  list[const_index(7)] = 3;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: std::array<> index 10 is past 
the end of the array (which contains 10 elements)
 
-  a[0] = 3; // OK, constant index and inside bounds
-  a[1] = 3; // OK, constant index and inside bounds
-  a[9] = 3; // OK, constant index and inside bounds
-  a[const_index(6)] = 3; // OK, constant index and inside bounds
+  list[0] = 3; // OK, constant index and inside bounds
+  list[1] = 3; // OK, constant index and inside bounds
+  list[9] = 3; // OK, constant index and inside bounds
+  list[const_index(6)] = 3; // OK, constant index and inside bounds
 }
 
 void g() {
   int a[10];
   for (int i = 0; i < 10; ++i) {
-a[i] = i;
+list[i] = i;
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use array subscript 
when the index is not an integer constant expression; use gsl::at() instead
-// CHECK-FIXES: gsl::at(a, i) = i;
-gsl::at(a, i) = i; // OK, gsl::at() instead of []
+// CHECK-FIXES: gsl::at(list, i) = i;
+gsl::at(list, i) = i; // OK, gsl::at() instead of []
   }
 
-  a[-1] = 3; // flagged by clang-diagnostic-array-bounds
-  a[10] = 4; // flagged by clang-diagnostic-array-bounds
-  a[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
-
-  a[0] = 3; // OK, constant index and inside bounds
-  a[1] = 3; // OK, constant index and inside bounds
-  a[9] = 3; // OK, constant index and inside bounds
-  a[const_index(6)] = 3; // OK, constant index and inside bounds
+  list[-1] = 3; // flagged by clang-diagnostic-array-bounds
+  list[10] = 4; // flagged by clang-diagnostic-array-bounds
+  list[const_index(7)] = 3; // flagged by clang-diagnostic-array-bounds
+
+  list[0] = 3; // OK, constant index and inside bounds
+  list[1] = 3; // OK, constant index and inside bounds
+  list[9] = 3; // OK, 

[PATCH] D56661: [clang-tidy] Fix incorrect array name generation in cppcoreguidelines-pro-bounds-constant-array-index

[Dmitry Venikov via Phabricator via cfe-commits]

Quolyk added a comment.

For now I just updated tests. The problem is in `BaseRange` definition, as it 
holds `EndLoc` and `BeginLoc` pointing to the beginning of ArrayExpression base 
https://github.com/llvm-mirror/clang-tools-extra/blob/e0441f6939da38f26bea9c1d75bb33024daa4e40/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp#L78.
 I'm investigating this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56661



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

[PATCH] D56592: [clangd] Fix updated file detection logic in indexing

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/index/Background.cpp:309
   for (const auto &I : *Index.Sources) {
+// We already have the map from uris to absolutepaths in the cache,
+// therefore traverse Index.Sources rather than Files to get rid of 
absolute

ilya-biryukov wrote:
> If this the only reason we're traversing `Index.Sources`? If so, I suggest 
> removing this comment and traversing `Files` instead.
> This would make the code more straightforward and would definitely cost us 
> only a negligible performance penalty.
Well actually, in addition to that I was traversing source files rather than 
updated files to make sure we write down shards even for sources without any 
symbols or refs in them. Because current logic in `FileFilter` didn't take 
files without any symbols into account.

So this is actually rather about detecting if a file is already up-to-date or 
not, changing that logic to take new files without any symbols into account.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56592



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

[PATCH] D56655: [clangd] Fix mac buildbot failure.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

https://reviews.llvm.org/D56656 has landed to fix that issue


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56655



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

Re: r351012 - Implement TemplateArgument dumping in terms of Visitor

[Aaron Ballman via cfe-commits]

On Sat, Jan 12, 2019 at 11:39 AM Stephen Kelly via cfe-commits
 wrote:
>
> Author: steveire
> Date: Sat Jan 12 08:35:37 2019
> New Revision: 351012
>
> URL: http://llvm.org/viewvc/llvm-project?rev=351012&view=rev
> Log:
> Implement TemplateArgument dumping in terms of Visitor
>
> Summary: Split the output streaming from the traversal to other AST nodes.

Thanks for this! However, it looks like you missed some of the nits
raised in the review -- comments below.

>
> Reviewers: aaron.ballman
>
> Subscribers: cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D55491
>
> Added:
> cfe/trunk/include/clang/AST/TemplateArgumentVisitor.h
> Modified:
> cfe/trunk/include/clang/AST/TextNodeDumper.h
> cfe/trunk/lib/AST/ASTDumper.cpp
> cfe/trunk/lib/AST/TextNodeDumper.cpp
>
> Added: cfe/trunk/include/clang/AST/TemplateArgumentVisitor.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/TemplateArgumentVisitor.h?rev=351012&view=auto
> ==
> --- cfe/trunk/include/clang/AST/TemplateArgumentVisitor.h (added)
> +++ cfe/trunk/include/clang/AST/TemplateArgumentVisitor.h Sat Jan 12 08:35:37 
> 2019
> @@ -0,0 +1,99 @@
> +//===- TemplateArgumentVisitor.h - Visitor for TArg subclasses --*- C++ 
> -*-===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===--===//
> +//
> +//  This file defines the TemplateArgumentVisitor interface.
> +//
> +//===--===//
> +
> +#ifndef LLVM_CLANG_AST_TEMPLATEARGUMENTVISITOR_H
> +#define LLVM_CLANG_AST_TEMPLATEARGUMENTVISITOR_H
> +
> +#include "clang/AST/TemplateBase.h"
> +
> +namespace clang {
> +
> +namespace templateargumentvisitor {
> +
> +/// A simple visitor class that helps create template argument visitors.
> +template  class Ref, typename ImplClass,
> +  typename RetTy = void, typename... ParamTys>
> +class Base {
> +public:
> +#define REF(CLASS) typename Ref::type
> +#define DISPATCH(NAME)   
>   \
> +  case TemplateArgument::NAME:   
>   \
> +return static_cast(this)->Visit##NAME##TemplateArgument(
>   \
> +TA, std::forward(P)...)
> +
> +  RetTy Visit(REF(TemplateArgument) TA, ParamTys... P) {
> +switch (TA.getKind()) {
> +  DISPATCH(Null);
> +  DISPATCH(Type);
> +  DISPATCH(Declaration);
> +  DISPATCH(NullPtr);
> +  DISPATCH(Integral);
> +  DISPATCH(Template);
> +  DISPATCH(TemplateExpansion);
> +  DISPATCH(Expression);
> +  DISPATCH(Pack);
> +}
> +llvm_unreachable("TemplateArgument is not covered in switch!");
> +  }
> +
> +  // If the implementation chooses not to implement a certain visit
> +  // method, fall back to the parent.
> +
> +#define VISIT_METHOD(CATEGORY)   
>   \
> +  RetTy Visit##CATEGORY##TemplateArgument(REF(TemplateArgument) TA,  
>   \
> +  ParamTys... P) {   
>   \
> +return VisitTemplateArgument(TA, std::forward(P)...);  
>   \
> +  }
> +
> +  VISIT_METHOD(Null);
> +  VISIT_METHOD(Type);
> +  VISIT_METHOD(Declaration);
> +  VISIT_METHOD(NullPtr);
> +  VISIT_METHOD(Integral);
> +  VISIT_METHOD(Template);
> +  VISIT_METHOD(TemplateExpansion);
> +  VISIT_METHOD(Expression);
> +  VISIT_METHOD(Pack);
> +
> +  RetTy VisitTemplateArgument(REF(TemplateArgument), ParamTys...) {
> +return RetTy();
> +  }
> +
> +#undef REF
> +#undef DISPATCH
> +#undef VISIT_METHOD
> +};
> +
> +} // namespace templateargumentvisitor
> +
> +/// A simple visitor class that helps create template argument visitors.
> +///
> +/// This class does not preserve constness of TemplateArgument references 
> (see
> +/// also ConstTemplateArgumentVisitor).
> +template 
> +class TemplateArgumentVisitor
> +: public templateargumentvisitor::Base ImplClass,
> +   RetTy, ParamTys...> {};
> +
> +/// A simple visitor class that helps create template argument visitors.
> +///
> +/// This class preserves constness of TemplateArgument references (see also
> +/// TemplateArgumentVisitor).
> +template 
> +class ConstTemplateArgumentVisitor
> +: public templateargumentvisitor::Base +   RetTy, ParamTys...> {};
> +
> +} // namespace clang
> +
> +#endif // LLVM_CLANG_AST_TEMPLATEARGUMENTVISITOR_H
>
> Modified: cfe/trunk/include/clang/AST/TextNodeDumper.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/TextNodeDumper.h?rev=351012&r1=351011&r2=351012&view=diff
> 

[PATCH] D56267: [clangd] Interfaces for writing code actions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

High level comments:

- this is an important concept and in hindsight it's amazing we didn't have it 
yet!
- I don't get a clear sense of the boundaries between "code action", 
"refactoring", and "action".  There are also "prepared actions" and "instant 
actions".  I think some combination of fewer concepts, clearer names, and more 
consistent usage would help a lot.
- it's pretty heavy on registries/factories/preparedX, maybe we can find a way 
to simplify
- many (most?) actions are going to do some "walk up AST looking for X", the 
interface could help make that easy




Comment at: clangd/ClangdLSPServer.cpp:346
+  {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
+   ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION}},
  }},

Seems a bit more direct to give each one its own command name? Maybe under a 
namespace like refactor/swapIfBranches



Comment at: clangd/ClangdLSPServer.cpp:662
+std::vector Actions = std::move(FixIts);
+auto toCodeAction = [&](PreparedAction &&A) -> CodeAction {
+  CodeAction CA;

This seems like it could just be a helper function... what does it capture?
I think it might belong next to PreparedActions just like we have `toLSPDiags` 
in `Diag.h` - it's not ideal dependency-wise, but inlining everything into 
ClangdLSPServer seems dubious too. Happy to discuss offline...



Comment at: clangd/ClangdLSPServer.cpp:668
+// This is an instant action, so we reply with a command to 
directly
+// apply the edit that the action has already produced.
+auto R = cantFail(std::move(A).computeEdits());

This optimization seems inessential, and complicates the model a bit. Can we 
leave it out for now?



Comment at: clangd/ClangdServer.h:213
+
+  /// Apply the previously precomputed code action. This will always fully
+  /// execute the action.

Unclear what the second sentence means, I'd suggest dropping it. Most obvious 
(to me) interpretation is that this can't fail, which isn't true.
nit: "Previously precomputed" is redundant :-)




Comment at: clangd/CodeActions.h:16
+
+#include "refactor/ActionProvider.h"
+

why is ActionProvider in refactor/ but CodeActions/ is here?



Comment at: clangd/CodeActions.h:23
+/// running the actions.
+class CodeActions {
+public:

I'm not sure this indirection is necessary, would a 
StringMap> suffice?

Since it's not actually complicated and there's just one caller, I think 
knowing what `prepareAll` is doing at the callsite would be an improvement.



Comment at: clangd/refactor/ActionProvider.h:67
+  // FIXME: provide a way to get sources and ASTs for other files.
+  // FIXME: cache some commonly required information (e.g. AST nodes under
+  //cursor) to avoid redundant AST visit in every action.

I think we should consider (not necessarily adopt) designing the interface 
around walking up the AST node chain, rather than bolting it on later.



Comment at: clangd/refactor/ActionProvider.h:71
+
+using ActionID = size_t;
+/// Result of executing a first stage of the action. If computing the resulting

I'd prefer a string here, this will make the protocol easier to understand for 
human readers.



Comment at: clangd/refactor/ActionProvider.h:78
+/// so it's not safe to store the PreparedAction for long spans of time.
+class PreparedAction {
+public:

This is a concrete class that holds a type-erased unique_function with the 
actual logic. Similarly, the factories are just wrappers of unique_function.
This method of abstraction is harder (for me at least) to follow than 
expressing the commonality of the important objects (the refactorings) as 
inheritance. And more concretely, stack traces are going to be terrible :-)

The factory/prepared duality makes it hard to represent a refactoring as a 
single class, but let's have a think about this part.



Comment at: clangd/refactor/ActionProvider.h:87
+  /// CodeActions::prepare().
+  ActionID id() const { return ID; }
+  /// A title of the action for display in the UI.

Why is this dynamic and not a constructor param?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56267



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

[PATCH] D56639: NFC: Move Type Visit implementation to TextNodeDumper

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

LGTM aside from some minor nits.




Comment at: lib/AST/ASTDumper.cpp:434
+NodeDumper.Visit(T);
 if (!T) {
   return;

Elide braces.



Comment at: lib/AST/TextNodeDumper.cpp:15
 #include "clang/AST/TextNodeDumper.h"
 
+#include "clang/AST/LocInfoType.h"

Spurious whitespace.



Comment at: lib/AST/TextNodeDumper.cpp:142
+  }
+  if (llvm::isa(T)) {
+{

Drop the `llvm::`



Comment at: lib/AST/TextNodeDumper.cpp:163-166
+  if (T->isDependentType())
+OS << " dependent";
+  else if (T->isInstantiationDependentType())
+OS << " instantiation_dependent";

Can you add a newline before the `if` statement and after the `else if` body? 
Something to set it apart that only one of these two branches is output.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56639



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

[PATCH] D56650: [lld] [ELF] Support inferring target triple from filename

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

I'm happy with this approach since the triple can be set on the commandline.
If this gets merged I will update CHERI lld to use the triple instead of the 
new emulation that we added.




Comment at: ELF/Driver.cpp:757
+  if (!TargetOpt.empty()) {
+// TODO: do we want to verify it and fail on unsupported?
+Config->TargetTriple = llvm::Triple(TargetOpt);

I think failing on an unknown triple would be good since it can avoid user 
error where the wrong defaults are chosen just because of a small typo.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56650



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

[PATCH] D56640: NFC: Canonicalize handling of TypeLocInfo

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D56640



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

[PATCH] D56641: NFC: Move dumping of QualType node to TextNodeDumper

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D56641



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

[PATCH] D56643: NFC: Move Decl node handling to TextNodeDumper

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

LGTM aside from some minor nits.




Comment at: lib/AST/ASTDumper.cpp:518
+NodeDumper.Visit(D);
 if (!D) {
   return;

Elide braces.



Comment at: lib/AST/TextNodeDumper.cpp:239
+OS << " in " << M->getFullModuleName();
+  if (auto *ND = dyn_cast(D))
+for (Module *M : D->getASTContext().getModulesWithMergedDefinition(

`const auto *` (which helps to clarify the `const_cast<>` below.)



Comment at: lib/AST/TextNodeDumper.cpp:243
+  AddChild([=] { OS << "also in " << M->getFullModuleName(); });
+  if (const NamedDecl *ND = dyn_cast(D))
+if (ND->isHidden())

`const auto *`



Comment at: lib/AST/TextNodeDumper.cpp:248-251
+  if (D->isUsed())
+OS << " used";
+  else if (D->isThisDeclarationReferenced())
+OS << " referenced";

Surround these with newlines for visual clarity.



Comment at: lib/AST/TextNodeDumper.cpp:254
+OS << " invalid";
+  if (const FunctionDecl *FD = dyn_cast(D))
+if (FD->isConstexpr())

`const auto *`


Repository:
  rC Clang

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

https://reviews.llvm.org/D56643



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

[PATCH] D56597: [clangd] Add Limit parameter for xref.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 181543.
hokein marked 14 inline comments as done.
hokein added a comment.

Address comments.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56597

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/Merge.cpp
  clangd/index/dex/Dex.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1219,7 +1219,7 @@
 std::vector> ExpectedLocations;
 for (const auto &R : T.ranges())
   ExpectedLocations.push_back(RangeIs(R));
-EXPECT_THAT(findReferences(AST, T.point()),
+EXPECT_THAT(findReferences(AST, T.point(), 0),
 ElementsAreArray(ExpectedLocations))
 << Test;
   }
@@ -1266,7 +1266,7 @@
 std::vector> ExpectedLocations;
 for (const auto &R : T.ranges())
   ExpectedLocations.push_back(RangeIs(R));
-EXPECT_THAT(findReferences(AST, T.point()),
+EXPECT_THAT(findReferences(AST, T.point(), 0),
 ElementsAreArray(ExpectedLocations))
 << Test;
   }
@@ -1281,7 +1281,7 @@
   auto AST = TU.build();
 
   // References in main file are returned without index.
-  EXPECT_THAT(findReferences(AST, Main.point(), /*Index=*/nullptr),
+  EXPECT_THAT(findReferences(AST, Main.point(), 0, /*Index=*/nullptr),
   ElementsAre(RangeIs(Main.range(;
   Annotations IndexedMain(R"cpp(
 int main() { [[f^oo]](); }
@@ -1292,13 +1292,17 @@
   IndexedTU.Code = IndexedMain.code();
   IndexedTU.Filename = "Indexed.cpp";
   IndexedTU.HeaderCode = Header;
-  EXPECT_THAT(findReferences(AST, Main.point(), IndexedTU.index().get()),
+  EXPECT_THAT(findReferences(AST, Main.point(), 0, IndexedTU.index().get()),
   ElementsAre(RangeIs(Main.range()), RangeIs(IndexedMain.range(;
 
+  EXPECT_EQ(1u, findReferences(AST, Main.point(), /*Limit*/ 1,
+   IndexedTU.index().get())
+.size());
+
   // If the main file is in the index, we don't return duplicates.
   // (even if the references are in a different location)
   TU.Code = ("\n\n" + Main.code()).str();
-  EXPECT_THAT(findReferences(AST, Main.point(), TU.index().get()),
+  EXPECT_THAT(findReferences(AST, Main.point(), 0, TU.index().get()),
   ElementsAre(RangeIs(Main.range(;
 }
 
@@ -1328,7 +1332,7 @@
 Annotations File(T.AnnotatedCode);
 RecordingIndex Rec;
 auto AST = TestTU::withCode(File.code()).build();
-findReferences(AST, File.point(), &Rec);
+findReferences(AST, File.point(), 0, &Rec);
 if (T.WantQuery)
   EXPECT_NE(Rec.RefIDs, None) << T.AnnotatedCode;
 else
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -19,6 +19,7 @@
 
 using testing::_;
 using testing::AllOf;
+using testing::AnyOf;
 using testing::ElementsAre;
 using testing::Pair;
 using testing::Pointee;
@@ -290,16 +291,27 @@
 
   RefsRequest Request;
   Request.IDs = {Foo.ID};
-  RefSlab::Builder Results;
-  Merge.refs(Request, [&](const Ref &O) { Results.insert(Foo.ID, O); });
-
-  EXPECT_THAT(
-  std::move(Results).build(),
-  ElementsAre(Pair(
-  _, UnorderedElementsAre(AllOf(RefRange(Test1Code.range("Foo")),
-FileURI("unittest:///test.cc")),
-  AllOf(RefRange(Test2Code.range("Foo")),
-FileURI("unittest:///test2.cc"));
+  {
+RefSlab::Builder Results;
+Merge.refs(Request, [&](const Ref &O) { Results.insert(Foo.ID, O); });
+
+EXPECT_THAT(
+std::move(Results).build(),
+ElementsAre(Pair(
+_, UnorderedElementsAre(AllOf(RefRange(Test1Code.range("Foo")),
+  FileURI("unittest:///test.cc")),
+AllOf(RefRange(Test2Code.range("Foo")),
+  FileURI("unittest:///test2.cc"));
+  }
+  {
+Request.Limit = 1;
+RefSlab::Builder Results;
+Merge.refs(Request, [&](const Ref &O) { Results.insert(Foo.ID, O); });
+EXPECT_THAT(std::move(Results).build(),
+ElementsAre(Pair(
+_, ElementsAre(AnyOf(FileURI("unittest:///test.cc"),
+ FileURI("unittest:///test2.cc"));
+  }
 }
 
 MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
Index: unittests/clangd/DexTests.cpp
===
--- unittests/clangd/DexTests.c

[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/XRefs.cpp:578
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
+if (!Invalid) {

ilya-biryukov wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > Unfortunately we can't get the buffer here, because for the preamble 
> > > macros the files might change on disk after we build the preamble and we 
> > > can end up reading a different version of the file. Which can in turn 
> > > lead to crashes as the offsets obtained from the source locations can 
> > > point outside the buffer for the corresponding file.
> > > 
> > > This is really annoying and generally the solution is to try find a 
> > > different way to obtain the same information, e.g. by looking at what 
> > > `MacroInfo` has available.
> > > However, I don't know of a good workaround for this. Happy to look into 
> > > ways of providing something close to a macro definition from `MacroInfo` 
> > > or other sources, can scout for this tomorrow.
> > I tried to do something like MacroInfo::dump. One problem is that we don't 
> > always have enough information about literals. For example:
> > 
> > ```
> > #define MACRO 0
> > int main() { return MACRO; }
> > ```
> > Becomes:
> > ```
> > #define MACRO numeric_constant
> > ```
> > 
> > I poked around the Token code and I didn't find a way to retrieve the 
> > literal without going through the buffer (Preprocessor::getSpelling for 
> > example).
> > 
> > What you mention is only a problem when you use the option of storing 
> > preambles on disk, right? So something would need to modify the preamble 
> > files on disk, which are stored in some temp folder. It doesn't sound like 
> > that could happen frequently. There is also bounds checking in the code 
> > above so it shouldn't crash, right? Even if the bounds are incorrect, it 
> > will be no different than the Range we return to the client for 
> > document/definition, i.e. the client can use the range on a newly modified 
> > header and get it wrong. I also tested renaming the pch file and then 
> > editing the source file and it results in an invalid AST. So either way 
> > modifying the pch would be bad news not for just macro hover.
> The problem shows up when the header files are modified, not the preamble 
> files (otherwise we'll be fine, in clangd we do assume the .pch files we 
> produce are not externally modified).
> Which is pretty frequent, and we've seen real crashes coming from fetching 
> documentation from the preambles.
> 
> I'll try to investigate how Preprocessor accesses the tokens of macro 
> definitions from preabmle. There are two possible outcomes I expect:
> 1. we find a way to access the spelling of the tokens in the same way PP does 
> and it's safe,
> 2. we realize preprocessor might crash with the preabmle because it accesses 
> the spelling of the tokens.
> 
> I hope to find (1), but (2) is also very realistic, unfortunately :-(
I think the preprocessor does the same thing, therefore the compiler can 
potentially access the header files to get spelling of the tokens coming from 
headers.
This means that clangd can potentially crash if the headers are changed 
concurrently with the operations that use preabmles (code completion, being the 
most common one).

In that case, I agree that it's fine to use the buffers for headers here and we 
should fix this separately, probably by snapshotting the contents of accessed 
headers.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250



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

[PATCH] D56597: [clangd] Add Limit parameter for xref.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: unittests/clangd/DexTests.cpp:673
 
-  std::vector Files;
-  RefsRequest Req;
-  Req.IDs.insert(Foo.ID);
-  Req.Filter = RefKind::Declaration | RefKind::Definition;
-  Dex(std::vector{Foo, Bar}, Refs).refs(Req, [&](const Ref &R) {
-Files.push_back(R.Location.FileURI);
-  });
-
-  EXPECT_THAT(Files, ElementsAre("foo.h"));
+  {
+std::vector Files;

sammccall wrote:
> If you want these to be the same test, there should be some connection 
> between them.
> A natural one is that it's the same query with a different limit - you could 
> extend the original test to have multiple results.
Done, made the query request the same in this test except the limit.



Comment at: unittests/clangd/DexTests.cpp:686
+Req.IDs.insert(Foo.ID);
+Req.Limit = 1;
+size_t RefCount = 0;

sammccall wrote:
> As far as I can tell, this limit makes no difference, there's only one result 
> anyway.
Actually, the filter of the request was different than the case above, we would 
get 2 refs. Used the same request to avoid confusion.



Comment at: unittests/clangd/IndexTests.cpp:306
+  {
+Request.Limit = 1;
+size_t RefsCount = 0;

sammccall wrote:
> why new scope here?
To align with the case above. 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56597



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

[PATCH] D56655: [clangd] Fix mac buildbot failure.

[Haojian Wu via Phabricator via cfe-commits]

hokein abandoned this revision.
hokein added a comment.

In D56655#1356000 , @kadircet wrote:

> https://reviews.llvm.org/D56656 has landed to fix that issue


Thanks! I have verified that the test failure is gone after your patch.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56655



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

[PATCH] D56592: [clangd] Fix updated file detection logic in indexing

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 181544.
kadircet added a comment.

- Change FileFilter and update logic to do not care about if there are any 
symbols coming from a file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56592

Files:
  clangd/index/Background.cpp
  clangd/index/Background.h
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -351,11 +351,82 @@
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
-  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardSource, nullptr);
   EXPECT_THAT(*ShardSource->Symbols,
   Contains(AllOf(Named("f_b"), Declared(), Defined(;
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageEmptyFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/B.h")] = R"cpp(
+  #include "A.h"
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"B.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check that A.cc, A.h and B.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_THAT(Storage.keys(),
+  UnorderedElementsAre(testPath("root/A.cc"), testPath("root/A.h"),
+   testPath("root/B.h")));
+  auto ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_TRUE(ShardHeader->Symbols->empty());
+
+  // Check that A.cc, A.h and B.h has been loaded.
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+
+  // Update B.h to contain some symbols.
+  FS.Files[testPath("root/B.h")] = R"cpp(
+  #include "A.h"
+  void new_func();
+  )cpp";
+  // Check that B.h has been stored with new contents.
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+  ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols,
+  Contains(AllOf(Named("new_func"), Declared(), Not(Defined();
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -91,10 +91,11 @@
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
 private:
-  /// Given index results from a TU, only update files in \p FilesToUpdate.
-  /// Also stores new index information on IndexStorage.
+  /// Given index results from a TU, only update symbols coming from files with
+  /// different digests than \p DigestsSnapshot. Also stores new index
+  /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
-  const llvm::StringMap &FilesToUpdate,
+  const llvm::StringMap &DigestsSnapshot,
   BackgroundIndexStorage *IndexStorage);
 
   // configuration
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -94,9 +94,8 @@
 // \p FileDigests contains file digests for the current indexed files, and all
 // changed files will be added to \p FilesToUpdate.
 decltype(SymbolCollector::Options::FileFilter)
-createFileFilter(const llvm::StringMap &FileDigests,
- llvm::StringMap &FilesToUpdate) {
-  return [&FileDigests, &FilesToUpdate](const SourceManager &SM

[PATCH] D56592: [clangd] Fix updated file detection logic in indexing

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 181545.
kadircet added a comment.

- Update comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56592

Files:
  clangd/index/Background.cpp
  clangd/index/Background.h
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -351,11 +351,82 @@
   EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
 
   // Check if the new symbol has arrived.
-  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardSource, nullptr);
   EXPECT_THAT(*ShardSource->Symbols,
   Contains(AllOf(Named("f_b"), Declared(), Defined(;
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageEmptyFile) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/B.h")] = R"cpp(
+  #include "A.h"
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"B.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check that A.cc, A.h and B.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_THAT(Storage.keys(),
+  UnorderedElementsAre(testPath("root/A.cc"), testPath("root/A.h"),
+   testPath("root/B.h")));
+  auto ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_TRUE(ShardHeader->Symbols->empty());
+
+  // Check that A.cc, A.h and B.h has been loaded.
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+
+  // Update B.h to contain some symbols.
+  FS.Files[testPath("root/B.h")] = R"cpp(
+  #include "A.h"
+  void new_func();
+  )cpp";
+  // Check that B.h has been stored with new contents.
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 3U);
+  ShardHeader = MSS.loadShard(testPath("root/B.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols,
+  Contains(AllOf(Named("new_func"), Declared(), Not(Defined();
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -91,10 +91,11 @@
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
 private:
-  /// Given index results from a TU, only update files in \p FilesToUpdate.
-  /// Also stores new index information on IndexStorage.
+  /// Given index results from a TU, only update symbols coming from files with
+  /// different digests than \p DigestsSnapshot. Also stores new index
+  /// information on IndexStorage.
   void update(llvm::StringRef MainFile, IndexFileIn Index,
-  const llvm::StringMap &FilesToUpdate,
+  const llvm::StringMap &DigestsSnapshot,
   BackgroundIndexStorage *IndexStorage);
 
   // configuration
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -91,12 +91,10 @@
 
 // Creates a filter to not collect index results from files with unchanged
 // digests.
-// \p FileDigests contains file digests for the current indexed files, and all
-// changed files will be added to \p FilesToUpdate.
+// \p FileDigests contains file digests for the current indexed files.
 decltype(SymbolCollector::Options::FileFilter)
-createFileFilter(const llvm::StringMap &FileDigests,
- llvm::StringMap &Fi

[PATCH] D56642: NFC: Move dump of type nodes to NodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Mostly looks good (a few minor nits), but I did have a design question about 
whether we should prefer calling a `Visit` method for type hierarchies instead 
of reimplementing the functionality.




Comment at: lib/AST/ASTDumper.cpp:145
 void VisitVariableArrayType(const VariableArrayType *T) {
-  OS << " ";
-  NodeDumper.dumpSourceRange(T->getBracketsRange());
-  VisitArrayType(T);
+  dumpTypeAsChild(T->getElementType());
   dumpStmt(T->getSizeExpr());

Why this approach instead of deferring to `VisitArrayType()` as before? I 
prefer calling the Visit function rather than reimplementing the functionality 
because we may decide to later improve the base class printing and expect 
subclasses to automatically pick that up. WDYT?



Comment at: lib/AST/ASTDumper.cpp:164
 void VisitFunctionProtoType(const FunctionProtoType *T) {
-  auto EPI = T->getExtProtoInfo();
-  if (EPI.HasTrailingReturn) OS << " trailing_return";
-
-  if (!T->getTypeQuals().empty())
-OS << " " << T->getTypeQuals().getAsString();
-
-  switch (EPI.RefQualifier) {
-case RQ_None: break;
-case RQ_LValue: OS << " &"; break;
-case RQ_RValue: OS << " &&"; break;
-  }
-  // FIXME: Exception specification.
-  // FIXME: Consumed parameters.
-  VisitFunctionType(T);
+  dumpTypeAsChild(T->getReturnType());
   for (QualType PT : T->getParamTypes())

Why this approach as opposed to deferring to `VisitFunctionType()` as before?



Comment at: lib/AST/TextNodeDumper.cpp:15
 #include "clang/AST/TextNodeDumper.h"
 
+#include "clang/AST/DeclTemplate.h"

Remove newline



Comment at: lib/AST/TextNodeDumper.cpp:960
+void TextNodeDumper::VisitFunctionType(const FunctionType *T) {
+  auto EI = T->getExtInfo();
+  if (EI.getNoReturn())

It'd be nice to remove this use of `auto`.



Comment at: lib/AST/TextNodeDumper.cpp:971
+void TextNodeDumper::VisitFunctionProtoType(const FunctionProtoType *T) {
+  auto EPI = T->getExtProtoInfo();
+  if (EPI.HasTrailingReturn)

It'd be nice to remove this use of `auto`.



Comment at: lib/AST/TextNodeDumper.cpp:1047
+void TextNodeDumper::VisitPackExpansionType(const PackExpansionType *T) {
+  if (auto N = T->getNumExpansions())
+OS << " expansions " << *N;

It'd be nice to remove this use of `auto`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56642



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

[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/XRefs.cpp:578
+bool Invalid;
+StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
+if (!Invalid) {

ilya-biryukov wrote:
> ilya-biryukov wrote:
> > malaperle wrote:
> > > ilya-biryukov wrote:
> > > > Unfortunately we can't get the buffer here, because for the preamble 
> > > > macros the files might change on disk after we build the preamble and 
> > > > we can end up reading a different version of the file. Which can in 
> > > > turn lead to crashes as the offsets obtained from the source locations 
> > > > can point outside the buffer for the corresponding file.
> > > > 
> > > > This is really annoying and generally the solution is to try find a 
> > > > different way to obtain the same information, e.g. by looking at what 
> > > > `MacroInfo` has available.
> > > > However, I don't know of a good workaround for this. Happy to look into 
> > > > ways of providing something close to a macro definition from 
> > > > `MacroInfo` or other sources, can scout for this tomorrow.
> > > I tried to do something like MacroInfo::dump. One problem is that we 
> > > don't always have enough information about literals. For example:
> > > 
> > > ```
> > > #define MACRO 0
> > > int main() { return MACRO; }
> > > ```
> > > Becomes:
> > > ```
> > > #define MACRO numeric_constant
> > > ```
> > > 
> > > I poked around the Token code and I didn't find a way to retrieve the 
> > > literal without going through the buffer (Preprocessor::getSpelling for 
> > > example).
> > > 
> > > What you mention is only a problem when you use the option of storing 
> > > preambles on disk, right? So something would need to modify the preamble 
> > > files on disk, which are stored in some temp folder. It doesn't sound 
> > > like that could happen frequently. There is also bounds checking in the 
> > > code above so it shouldn't crash, right? Even if the bounds are 
> > > incorrect, it will be no different than the Range we return to the client 
> > > for document/definition, i.e. the client can use the range on a newly 
> > > modified header and get it wrong. I also tested renaming the pch file and 
> > > then editing the source file and it results in an invalid AST. So either 
> > > way modifying the pch would be bad news not for just macro hover.
> > The problem shows up when the header files are modified, not the preamble 
> > files (otherwise we'll be fine, in clangd we do assume the .pch files we 
> > produce are not externally modified).
> > Which is pretty frequent, and we've seen real crashes coming from fetching 
> > documentation from the preambles.
> > 
> > I'll try to investigate how Preprocessor accesses the tokens of macro 
> > definitions from preabmle. There are two possible outcomes I expect:
> > 1. we find a way to access the spelling of the tokens in the same way PP 
> > does and it's safe,
> > 2. we realize preprocessor might crash with the preabmle because it 
> > accesses the spelling of the tokens.
> > 
> > I hope to find (1), but (2) is also very realistic, unfortunately :-(
> I think the preprocessor does the same thing, therefore the compiler can 
> potentially access the header files to get spelling of the tokens coming from 
> headers.
> This means that clangd can potentially crash if the headers are changed 
> concurrently with the operations that use preabmles (code completion, being 
> the most common one).
> 
> In that case, I agree that it's fine to use the buffers for headers here and 
> we should fix this separately, probably by snapshotting the contents of 
> accessed headers.
And I'm happy to take a look at reproducing the PP crash that I'm talking about 
here and fixing this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250



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

[PATCH] D56642: NFC: Move dump of type nodes to NodeDumper

[Stephen Kelly via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTDumper.cpp:145
 void VisitVariableArrayType(const VariableArrayType *T) {
-  OS << " ";
-  NodeDumper.dumpSourceRange(T->getBracketsRange());
-  VisitArrayType(T);
+  dumpTypeAsChild(T->getElementType());
   dumpStmt(T->getSizeExpr());

aaron.ballman wrote:
> Why this approach instead of deferring to `VisitArrayType()` as before? I 
> prefer calling the Visit function rather than reimplementing the 
> functionality because we may decide to later improve the base class printing 
> and expect subclasses to automatically pick that up. WDYT?
I think it's more clear to read what each visit method does, but I don't feel 
strongly about it. I can change this if you do.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56642



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

[PATCH] D56663: [MSP430] Improve support of 'interrupt' attribute

[Kristina Bessonova via Phabricator via cfe-commits]

krisb created this revision.
Herald added a subscriber: cfe-commits.
krisb edited the summary of this revision.
krisb added a reviewer: asl.
krisb edited the summary of this revision.

- Accept as an argument constants in range 0..63 (aligned with TI headers and 
linker scripts provided with TI GCC toolchain).

- Emit function attribute 'interrupt'='xx' instead of aliases (used in the 
backend to create a section for particular interrupt vector).

- Add more diagnostics.


Repository:
  rC Clang

https://reviews.llvm.org/D56663

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/attr-msp430.c
  test/Sema/attr-msp430.c

Index: test/Sema/attr-msp430.c
===
--- test/Sema/attr-msp430.c
+++ test/Sema/attr-msp430.c
@@ -1,6 +1,13 @@
 // RUN: %clang_cc1 -triple msp430-unknown-unknown -fsyntax-only -verify %s
 
+__attribute__((interrupt(1))) int t; // expected-warning {{'interrupt' attribute only applies to functions}}
+
 int i;
-void f(void) __attribute__((interrupt(i))); /* expected-error {{'interrupt' attribute requires an integer constant}} */
+__attribute__((interrupt(i))) void f(void); // expected-error {{'interrupt' attribute requires an integer constant}}
+__attribute__((interrupt(1, 2))) void f2(void); // expected-error {{'interrupt' attribute takes one argument}}
+__attribute__((interrupt(1))) int f3(void); // expected-warning {{MSP430 'interrupt' attribute only applies to functions that have a 'void' return type}}
+__attribute__((interrupt(1))) void f4(int a); // expected-warning {{MSP430 'interrupt' attribute only applies to functions that have no parameters}}
+__attribute__((interrupt(64))) void f5(void); // expected-error {{'interrupt' attribute parameter 64 is out of bounds}}
 
-void f2(void) __attribute__((interrupt(12)));
+__attribute__((interrupt(0))) void f6(void);
+__attribute__((interrupt(63))) void f7(void);
Index: test/CodeGen/attr-msp430.c
===
--- /dev/null
+++ test/CodeGen/attr-msp430.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple msp430-unknown-unknown -emit-llvm < %s| FileCheck %s
+
+__attribute__((interrupt(1))) void foo(void) {}
+// CHECK: @llvm.used
+// CHECK-SAME: @foo
+
+// CHECK: define msp430_intrcc void @foo() #0
+// CHECK: attributes #0
+// CHECK-SAME: noinline
+// CHECK-SAME: "interrupt"="1"
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -5377,6 +5377,27 @@
 }
 
 static void handleMSP430InterruptAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  // MSP430 'interrupt' attribute is applied to
+  // a function with no parameters and void return type
+  if (!isFunctionOrMethod(D)) {
+S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+<< "'interrupt'" << ExpectedFunctionOrMethod;
+return;
+  }
+
+  if (hasFunctionProto(D) && getFunctionOrMethodNumParams(D) != 0) {
+S.Diag(D->getLocation(), diag::warn_msp430_interrupt_attribute)
+<< 0;
+return;
+  }
+
+  if (!getFunctionOrMethodResultType(D)->isVoidType()) {
+S.Diag(D->getLocation(), diag::warn_msp430_interrupt_attribute)
+<< 1;
+return;
+  }
+
+  // The attribute takes one integer argument
   if (!checkAttributeNumArgs(S, AL, 1))
 return;
 
@@ -5386,8 +5407,6 @@
 return;
   }
 
-  // FIXME: Check for decl - it should be void ()(void).
-
   Expr *NumParamsExpr = static_cast(AL.getArgAsExpr(0));
   llvm::APSInt NumParams(32);
   if (!NumParamsExpr->isIntegerConstantExpr(NumParams, S.Context)) {
@@ -5396,9 +5415,9 @@
 << NumParamsExpr->getSourceRange();
 return;
   }
-
+  // The argument should be in range 0..63
   unsigned Num = NumParams.getLimitedValue(255);
-  if ((Num & 1) || Num > 30) {
+  if (Num > 63) {
 S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds)
 << AL << (int)NumParams.getSExtValue()
 << NumParamsExpr->getSourceRange();
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -6774,21 +6774,19 @@
   if (GV->isDeclaration())
 return;
   if (const FunctionDecl *FD = dyn_cast_or_null(D)) {
-if (const MSP430InterruptAttr *attr = FD->getAttr()) {
-  // Handle 'interrupt' attribute:
-  llvm::Function *F = cast(GV);
+const MSP430InterruptAttr *Attr = FD->getAttr();
+if (!Attr)
+  return;
 
-  // Step 1: Set ISR calling convention.
-  F->setCallingConv(llvm::CallingConv::MSP430_INTR);
+// Handle 'interrupt' attribute:
+llvm::Function *F = cast(GV);
 
-  // Step 2: Add attributes goodness.
-  F->addFnAttr(llvm::Attribute::NoInline);
+// Step 1: Set ISR calling convention.
+F->setCallingConv(llvm::CallingConv::MSP430_INTR);
 
-  // Step

[PATCH] D56642: NFC: Move dump of type nodes to NodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTDumper.cpp:145
 void VisitVariableArrayType(const VariableArrayType *T) {
-  OS << " ";
-  NodeDumper.dumpSourceRange(T->getBracketsRange());
-  VisitArrayType(T);
+  dumpTypeAsChild(T->getElementType());
   dumpStmt(T->getSizeExpr());

steveire wrote:
> aaron.ballman wrote:
> > Why this approach instead of deferring to `VisitArrayType()` as before? I 
> > prefer calling the Visit function rather than reimplementing the 
> > functionality because we may decide to later improve the base class 
> > printing and expect subclasses to automatically pick that up. WDYT?
> I think it's more clear to read what each visit method does, but I don't feel 
> strongly about it. I can change this if you do.
Given that there's an established type hierarchy, I prefer calling the 
`Visit()` function for the superclass rather than reimplementing. The extra 
indirection wasn't too annoying with the old code, so it's unlikely to be too 
problematic with the new code (and if it is, then we'll have reason to change).


Repository:
  rC Clang

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

https://reviews.llvm.org/D56642



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

[PATCH] D56663: [MSP430] Improve support of 'interrupt' attribute

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:5381
+  // MSP430 'interrupt' attribute is applied to
+  // a function with no parameters and void return type
+  if (!isFunctionOrMethod(D)) {

nit: code comments usually have a dot `.` at the end of the sentence.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56663



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

[PATCH] D56665: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: bkramer, klimek.
Herald added a subscriber: cfe-commits.

Prior to r351069, lambda classes were traversed or not depending on the
{Function, Class, Namespace, TU} DeclContext containing them.
If it was a function (common case) they were not traversed.
If it was a namespace or TU (top-level lambda) they were traversed as part of
that DeclContext traversal.

r351069 "fixed" RAV to traverse these as part of the LambdaExpr, which is the
right place. But top-level lambdas are now traversed twice.
We fix that as blocks and block captures were apparently fixed in the past.

Maybe it would be nicer to avoid adding the lambda classes to the DeclContext
in the first place, but I can't work out the implications of that.


Repository:
  rC Clang

https://reviews.llvm.org/D56665

Files:
  include/clang/AST/RecursiveASTVisitor.h
  unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp


Index: unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
+++ unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
@@ -65,6 +65,17 @@
   EXPECT_FALSE(Visitor.allClassesHaveBeenTraversed());
 }
 
+TEST(RecursiveASTVisitor, TopLevelLambda) {
+  LambdaExprVisitor Visitor;
+  Visitor.VisitImplicitCode = true;
+  Visitor.ExpectMatch("", 1, 10);
+  Visitor.ExpectMatch("", 1, 14);
+  EXPECT_TRUE(Visitor.runOver("auto x = []{ [] {}; };",
+  LambdaExprVisitor::Lang_CXX11));
+  EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
+  EXPECT_TRUE(Visitor.allClassesHaveBeenTraversed());
+}
+
 TEST(RecursiveASTVisitor, VisitsLambdaExprAndImplicitClass) {
   LambdaExprVisitor Visitor;
   Visitor.VisitImplicitCode = true;
Index: include/clang/AST/RecursiveASTVisitor.h
===
--- include/clang/AST/RecursiveASTVisitor.h
+++ include/clang/AST/RecursiveASTVisitor.h
@@ -1368,9 +1368,14 @@
 template 
 bool 
RecursiveASTVisitor::canIgnoreChildDeclWhileTraversingDeclContext(
 const Decl *Child) {
-  // BlockDecls and CapturedDecls are traversed through BlockExprs and
-  // CapturedStmts respectively.
-  return isa(Child) || isa(Child);
+  // BlockDecls are traversed through BlockExprs,
+  // CapturedDecls are traversed through CapturedStmts.
+  if (isa(Child) || isa(Child))
+return true;
+  // Lambda classes are traversed through LambdaExprs.
+  if (const CXXRecordDecl* Cls = dyn_cast(Child))
+return Cls->isLambda();
+  return false;
 }
 
 template 


Index: unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
+++ unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
@@ -65,6 +65,17 @@
   EXPECT_FALSE(Visitor.allClassesHaveBeenTraversed());
 }
 
+TEST(RecursiveASTVisitor, TopLevelLambda) {
+  LambdaExprVisitor Visitor;
+  Visitor.VisitImplicitCode = true;
+  Visitor.ExpectMatch("", 1, 10);
+  Visitor.ExpectMatch("", 1, 14);
+  EXPECT_TRUE(Visitor.runOver("auto x = []{ [] {}; };",
+  LambdaExprVisitor::Lang_CXX11));
+  EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
+  EXPECT_TRUE(Visitor.allClassesHaveBeenTraversed());
+}
+
 TEST(RecursiveASTVisitor, VisitsLambdaExprAndImplicitClass) {
   LambdaExprVisitor Visitor;
   Visitor.VisitImplicitCode = true;
Index: include/clang/AST/RecursiveASTVisitor.h
===
--- include/clang/AST/RecursiveASTVisitor.h
+++ include/clang/AST/RecursiveASTVisitor.h
@@ -1368,9 +1368,14 @@
 template 
 bool RecursiveASTVisitor::canIgnoreChildDeclWhileTraversingDeclContext(
 const Decl *Child) {
-  // BlockDecls and CapturedDecls are traversed through BlockExprs and
-  // CapturedStmts respectively.
-  return isa(Child) || isa(Child);
+  // BlockDecls are traversed through BlockExprs,
+  // CapturedDecls are traversed through CapturedStmts.
+  if (isa(Child) || isa(Child))
+return true;
+  // Lambda classes are traversed through LambdaExprs.
+  if (const CXXRecordDecl* Cls = dyn_cast(Child))
+return Cls->isLambda();
+  return false;
 }
 
 template 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/AttrDocs.td:3780
+represent an implicit "this" pointer in class methods. If there is no implicit
+"this" pointer it shall not be referenced. The index '-1', or the name "?",
+represents an unknown callback callee argument. This can be a value which is

The `'?'` here needs to change to be `'__'` instead.



Comment at: include/clang/Basic/Builtins.def:96
 //  V:N: -> requires vectors of at least N bits to be legal
+//  C -> callback behavior: argument N is called with argument 
M_0, ..., M_k as payload
 //  FIXME: gcc has nonnull

Wrap to 80-col



Comment at: lib/Sema/SemaDeclAttr.cpp:3483
+
+  NameIdxMapping["__this"] = 0;
+

jdoerfert wrote:
> aaron.ballman wrote:
> > This doesn't match the documentation either, but I'm less clear on why the 
> > double underscores are used.
> If you use `this`, the lexer will generate the special "this" token. That one 
> is checked explicitly to be only used inside of non-static class methods. If 
> you have an idea how to avoid this check or make it consider uses in the 
> attribute as OK, please let me know.
Why do you want to avoid that check? It seems like that's a very good check to 
have -- what circumstances would you expect a user to use `this` in a something 
other than a non-static member function?



Comment at: lib/Sema/SemaDeclAttr.cpp:3492
+  SmallVector EncodingIndices;
+  for (unsigned u = 0, e = AL.getNumArgs(); u < e; u++) {
+

jdoerfert wrote:
> aaron.ballman wrote:
> > Identifiers don't match the usual naming conventions.  Prefer `++U` as well.
> OK.
> 
> 
> > Prefer ++U as well.
> 
> Out of curiosity, why?
Because the previous value of `U` is not needed (you're executing the 
instruction only for its side effects, not its value).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55483



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

[PATCH] D56665: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp:72
+  Visitor.ExpectMatch("", 1, 10);
+  Visitor.ExpectMatch("", 1, 14);
+  EXPECT_TRUE(Visitor.runOver("auto x = []{ [] {}; };",

Dirty secret: this is the line that catches the old bug.
The outer lambda was only traversed once (because the *lambdaexpr* was only 
traversed once) but its body, and thus the inner lambda was traversed twice 
(causing an assertion).

It'd be possible to express more directly, but it would require hacking up the 
fixture more, and this new test conveniently tests the 
lambda-in-lambda-with-VisitImplicitCode case that I forgot in the last patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56665



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 181552.
mgorny retitled this revision from "[lld] [ELF] Support inferring target triple 
from filename" to "[lld] [ELF] Support customizing behavior on target triple".
mgorny edited the summary of this revision.

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

https://reviews.llvm.org/D56650

Files:
  ELF/Config.h
  ELF/Driver.cpp
  ELF/Driver.h
  ELF/Options.td

Index: ELF/Options.td
===
--- ELF/Options.td
+++ ELF/Options.td
@@ -313,6 +313,8 @@
 
 defm sysroot: Eq<"sysroot", "Set the system root">;
 
+defm target: Eq<"target", "Apply configuration defaults for a given target">;
+
 def target1_rel: F<"target1-rel">, HelpText<"Interpret R_ARM_TARGET1 as R_ARM_REL32">;
 
 def target1_abs: F<"target1-abs">, HelpText<"Interpret R_ARM_TARGET1 as R_ARM_ABS32 (default)">;
Index: ELF/Driver.h
===
--- ELF/Driver.h
+++ ELF/Driver.h
@@ -31,6 +31,7 @@
   void addLibrary(StringRef Name);
 
 private:
+  void setTargetTriple(StringRef argv0, llvm::opt::InputArgList &Args);
   void readConfigs(llvm::opt::InputArgList &Args);
   void createFiles(llvm::opt::InputArgList &Args);
   void inferMachineType();
Index: ELF/Driver.cpp
===
--- ELF/Driver.cpp
+++ ELF/Driver.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TarWriter.h"
+#include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -378,6 +379,25 @@
 return;
   }
 
+  if (const char *Path = getReproduceOption(Args)) {
+// Note that --reproduce is a debug option so you can ignore it
+// if you are trying to understand the whole picture of the code.
+Expected> ErrOrWriter =
+TarWriter::create(Path, path::stem(Path));
+if (ErrOrWriter) {
+  Tar = std::move(*ErrOrWriter);
+  Tar->append("response.txt", createResponseFile(Args));
+  Tar->append("version.txt", getLLDVersion() + "\n");
+} else {
+  error("--reproduce: " + toString(ErrOrWriter.takeError()));
+}
+  }
+
+  initLLVM();
+  setTargetTriple(ArgsArr[0], Args);
+  readConfigs(Args);
+  checkZOptions(Args);
+
   // Handle -v or -version.
   //
   // A note about "compatible with GNU linkers" message: this is a hack for
@@ -393,26 +413,11 @@
   // lot of "configure" scripts out there that are generated by old version
   // of Libtool. We cannot convince every software developer to migrate to
   // the latest version and re-generate scripts. So we have this hack.
-  if (Args.hasArg(OPT_v) || Args.hasArg(OPT_version))
+  if (Args.hasArg(OPT_v) || Args.hasArg(OPT_version)) {
 message(getLLDVersion() + " (compatible with GNU linkers)");
-
-  if (const char *Path = getReproduceOption(Args)) {
-// Note that --reproduce is a debug option so you can ignore it
-// if you are trying to understand the whole picture of the code.
-Expected> ErrOrWriter =
-TarWriter::create(Path, path::stem(Path));
-if (ErrOrWriter) {
-  Tar = std::move(*ErrOrWriter);
-  Tar->append("response.txt", createResponseFile(Args));
-  Tar->append("version.txt", getLLDVersion() + "\n");
-} else {
-  error("--reproduce: " + toString(ErrOrWriter.takeError()));
-}
+message("Target: " + Config->TargetTriple.str());
   }
 
-  readConfigs(Args);
-  checkZOptions(Args);
-
   // The behavior of -v or --version is a bit strange, but this is
   // needed for compatibility with GNU linkers.
   if (Args.hasArg(OPT_v) && !Args.hasArg(OPT_INPUT))
@@ -420,7 +425,6 @@
   if (Args.hasArg(OPT_version))
 return;
 
-  initLLVM();
   createFiles(Args);
   if (errorCount())
 return;
@@ -746,6 +750,34 @@
   error(Msg + ": " + StringRef(Err).trim());
 }
 
+void LinkerDriver::setTargetTriple(StringRef argv0, opt::InputArgList &Args) {
+  std::string TargetError;
+
+  // Firstly, see if user specified explicit --target
+  StringRef TargetOpt = Args.getLastArgValue(OPT_target);
+  if (!TargetOpt.empty()) {
+if (llvm::TargetRegistry::lookupTarget(TargetOpt, TargetError))
+  Config->TargetTriple = llvm::Triple(TargetOpt);
+else
+  error("Unsupported --target=" + TargetOpt + ": " + TargetError);
+return;
+  }
+
+  // Secondly, try to get it from program name prefix
+  std::string ProgName = llvm::sys::path::stem(argv0);
+  size_t LastComponent = ProgName.rfind('-');
+  if (LastComponent != std::string::npos) {
+std::string Prefix = ProgName.substr(0, LastComponent);
+if (llvm::TargetRegistry::lookupTarget(Prefix, TargetError)) {
+  Config->TargetTriple = llvm::Triple(Prefix);
+  return;
+}
+  }
+
+  // Finally, use the default target triple
+  Config->TargetTriple = llvm::Triple(getDefaultTargetTriple());
+}
+
 // Initializes Config members by the command line

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Michał Górny via Phabricator via cfe-commits]

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



Comment at: ELF/Driver.cpp:757
+  if (!TargetOpt.empty()) {
+// TODO: do we want to verify it and fail on unsupported?
+Config->TargetTriple = llvm::Triple(TargetOpt);

arichardson wrote:
> I think failing on an unknown triple would be good since it can avoid user 
> error where the wrong defaults are chosen just because of a small typo.
Somewhat done. Note that LLD apparently doesn't fail on errors immediately, so 
it behaves a bit weird.


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

https://reviews.llvm.org/D56650



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 181554.
mgorny marked an inline comment as done.
mgorny added a comment.

Fixed leaving triple unset on invalid `--target`.


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

https://reviews.llvm.org/D56650

Files:
  ELF/Config.h
  ELF/Driver.cpp
  ELF/Driver.h
  ELF/Options.td

Index: ELF/Options.td
===
--- ELF/Options.td
+++ ELF/Options.td
@@ -313,6 +313,8 @@
 
 defm sysroot: Eq<"sysroot", "Set the system root">;
 
+defm target: Eq<"target", "Apply configuration defaults for a given target">;
+
 def target1_rel: F<"target1-rel">, HelpText<"Interpret R_ARM_TARGET1 as R_ARM_REL32">;
 
 def target1_abs: F<"target1-abs">, HelpText<"Interpret R_ARM_TARGET1 as R_ARM_ABS32 (default)">;
Index: ELF/Driver.h
===
--- ELF/Driver.h
+++ ELF/Driver.h
@@ -31,6 +31,7 @@
   void addLibrary(StringRef Name);
 
 private:
+  void setTargetTriple(StringRef argv0, llvm::opt::InputArgList &Args);
   void readConfigs(llvm::opt::InputArgList &Args);
   void createFiles(llvm::opt::InputArgList &Args);
   void inferMachineType();
Index: ELF/Driver.cpp
===
--- ELF/Driver.cpp
+++ ELF/Driver.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TarWriter.h"
+#include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
@@ -378,6 +379,25 @@
 return;
   }
 
+  if (const char *Path = getReproduceOption(Args)) {
+// Note that --reproduce is a debug option so you can ignore it
+// if you are trying to understand the whole picture of the code.
+Expected> ErrOrWriter =
+TarWriter::create(Path, path::stem(Path));
+if (ErrOrWriter) {
+  Tar = std::move(*ErrOrWriter);
+  Tar->append("response.txt", createResponseFile(Args));
+  Tar->append("version.txt", getLLDVersion() + "\n");
+} else {
+  error("--reproduce: " + toString(ErrOrWriter.takeError()));
+}
+  }
+
+  initLLVM();
+  setTargetTriple(ArgsArr[0], Args);
+  readConfigs(Args);
+  checkZOptions(Args);
+
   // Handle -v or -version.
   //
   // A note about "compatible with GNU linkers" message: this is a hack for
@@ -393,26 +413,11 @@
   // lot of "configure" scripts out there that are generated by old version
   // of Libtool. We cannot convince every software developer to migrate to
   // the latest version and re-generate scripts. So we have this hack.
-  if (Args.hasArg(OPT_v) || Args.hasArg(OPT_version))
+  if (Args.hasArg(OPT_v) || Args.hasArg(OPT_version)) {
 message(getLLDVersion() + " (compatible with GNU linkers)");
-
-  if (const char *Path = getReproduceOption(Args)) {
-// Note that --reproduce is a debug option so you can ignore it
-// if you are trying to understand the whole picture of the code.
-Expected> ErrOrWriter =
-TarWriter::create(Path, path::stem(Path));
-if (ErrOrWriter) {
-  Tar = std::move(*ErrOrWriter);
-  Tar->append("response.txt", createResponseFile(Args));
-  Tar->append("version.txt", getLLDVersion() + "\n");
-} else {
-  error("--reproduce: " + toString(ErrOrWriter.takeError()));
-}
+message("Target: " + Config->TargetTriple.str());
   }
 
-  readConfigs(Args);
-  checkZOptions(Args);
-
   // The behavior of -v or --version is a bit strange, but this is
   // needed for compatibility with GNU linkers.
   if (Args.hasArg(OPT_v) && !Args.hasArg(OPT_INPUT))
@@ -420,7 +425,6 @@
   if (Args.hasArg(OPT_version))
 return;
 
-  initLLVM();
   createFiles(Args);
   if (errorCount())
 return;
@@ -746,6 +750,34 @@
   error(Msg + ": " + StringRef(Err).trim());
 }
 
+void LinkerDriver::setTargetTriple(StringRef argv0, opt::InputArgList &Args) {
+  std::string TargetError;
+
+  // Firstly, see if user specified explicit --target
+  StringRef TargetOpt = Args.getLastArgValue(OPT_target);
+  if (!TargetOpt.empty()) {
+if (llvm::TargetRegistry::lookupTarget(TargetOpt, TargetError)) {
+  Config->TargetTriple = llvm::Triple(TargetOpt);
+  return;
+} else
+  error("Unsupported --target=" + TargetOpt + ": " + TargetError);
+  }
+
+  // Secondly, try to get it from program name prefix
+  std::string ProgName = llvm::sys::path::stem(argv0);
+  size_t LastComponent = ProgName.rfind('-');
+  if (LastComponent != std::string::npos) {
+std::string Prefix = ProgName.substr(0, LastComponent);
+if (llvm::TargetRegistry::lookupTarget(Prefix, TargetError)) {
+  Config->TargetTriple = llvm::Triple(Prefix);
+  return;
+}
+  }
+
+  // Finally, use the default target triple
+  Config->TargetTriple = llvm::Triple(getDefaultTargetTriple());
+}
+
 // Initializes Config members by the command line options.
 void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   errorHand

[PATCH] D56632: [analyzer] Track region liveness only through base regions.

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

This seems to be an important fix. Thank you!

Did you measure decrease in the false-positive rate or an increase in the 
true-positive rate on real code? I expect some.




Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:390
 
-  // FIXME: Remove when we migrate over to just using ValueManager.
   SymbolManager &getSymbolManager() { return SymMgr; }

Is this comment intentionally deleted?


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

https://reviews.llvm.org/D56632



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

[PATCH] D56267: [clangd] Interfaces for writing code actions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

To be a bit more concrete...

One approach which won't win any OO design points but might be quite easy to 
navigate: combine the ActionProvider and PreparedAction into one class.

  class SwapIfBranches : public MiniRefactoringThing {
SwapIfBranches(); // must have a default constructor.
const char* id() const override { return "swapIfBranches"; } // must be 
constant for the subclass.
bool prepare(const Inputs&) override; // may only be called once. Returns 
false if this operation doesn't apply.
std::string title() override; // REQUIRES: prepare() returned true
Expected apply() override; // REQUIRES: prepare() 
returned true
  }

note that the "subclasses of a common interface, have a default constructor" 
fits exactly with the LLVM Registry pattern, if we want to move to that.
Otherwise, a `StringMap>`

A more... complicated version could use something like CRTP to give a generic 
external interface while keeping the internal dataflow/signatures clean:

  class SwapIfBranches : public MiniRefactoringThing {
struct State { ... }; // completing forward declaration from CRTP base
Optional prepare(const Inputs&) const override;
std::string getTitle(const State&) const;
Expected apply(const State&) const override;
  }
  const char MiniRefactoringThing::ID[] = "swapIfBranches";

Similarly we could use Registry or a 
`StringMap>` where the CRTP base 
implements the interface.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56267



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

[PATCH] D56267: [clangd] Interfaces for writing code actions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Name ideas:

- "Refactoring" - I think the main drawback is the conflicts with potential 
global-refactorings that we do in clangd. I don't think the conflict with 
`tooling/refactor` is bad enough to avoid this. I also don't think "some of 
these don't look like refactoring to me" is a strong reason to avoid it.
- "Code action" - I think the partial overlap with the protocol concept is 
confusing. (Worse, the protocol has two things called code action, that 
partially overlap). It's also not really a standard term outside LSP/VSCode, so 
we should be able to come up with something as good.
- "Action" - much too vague, I think
- "RefactoringAction" (or refactoring/Action, or refactoring::Action...) - not 
terrible but I think dominated by just "Refactoring" or "Refactor".
- "AutoEdit" or something equally descriptive - this is bland along the lines 
of Code Action, may be ok if it's sufficiently short and unique
- "Swizzle" or something equally unique-and-semantics-free - I'd be happy with 
this kind of alternative if we can't find something that clearly means what we 
want


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56267



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

[PATCH] D56597: [clangd] Add Limit parameter for xref.

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: unittests/clangd/DexTests.cpp:684
+  }
+  {
+Req.Limit = 1;

instead of splitting scopes here, might be more obvious just to `Files.clear()` 
between?



Comment at: unittests/clangd/IndexTests.cpp:306
+  {
+Request.Limit = 1;
+size_t RefsCount = 0;

hokein wrote:
> sammccall wrote:
> > why new scope here?
> To align with the case above. 
FWIW I'd find it easier to read if neither this nor the one above used new 
scopes, which tend to suggest some spooky scoped object side effects to me.
Up to you, though.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56597



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

[PATCH] D56632: [analyzer] Track region liveness only through base regions.

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

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

Awesome detective work! I glanced over the code, it looks great. I'd love to 
dedicate more time to your liveness-related patches, but university is a thing, 
so finding typos and the like is the best I can do for a while.

Wild thought, would `debug.DumpLiveStmts` be of any use here?




Comment at: test/Analysis/symbol-reaper.cpp:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify %s
+

Core intentionally left out?



Comment at: test/Analysis/symbol-reaper.cpp:31
+  clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+}

N00b question: What does `SYMBOL DEAD` mean here exactly?



Comment at: unittests/StaticAnalyzer/CMakeLists.txt:8
   RegisterCustomCheckersTest.cpp
+  SymbolReaperTest.cpp
   )

Woohoo!


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

https://reviews.llvm.org/D56632



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

[PATCH] D55394: Re-order type param children of ObjC nodes

[Stephen Kelly via Phabricator via cfe-commits]

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



Comment at: test/AST/ast-dump-decl.m:90
 // CHECK-NEXT:   -ObjCProtocol {{.+}} 'P'
+// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
 

dblaikie wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > It seems strange to me to print out the type parameter after the 
> > > superclass information given the source order. My understanding of the 
> > > AST dumping order is that we try to keep the order of nodes in source 
> > > order whenever possible.
> > That is not really a possible thing to try to do, because the AST dump 
> > doesn't relate to a single language. It should be seen as language 
> > independent.
> > 
> > The principle I'm follow is that nodes dump themselves in entirety before 
> > starting to dump their child nodes. That is a principle already followed by 
> > most nodes. Changing this seems to be low cost, low impact and high benefit 
> > to the code.
> >That is not really a possible thing to try to do, because the AST dump 
> >doesn't relate to a single language. It should be seen as language 
> >independent.
> 
> Is this particular aspect different between the different source languages 
> Clang supports? (could you give examples?)
Hmm, maybe that wasn't a good point, particularly because these methods relate 
to ObjC. 

Other languages (eg Go https://gobyexample.com/functions) have different order 
of param types and param names, and different order of params and return types 
etc. So, the more general AST nodes have less reason for their order based on 
'the order in the source'.

Anyway, the principle I'm following is 'the node dumps itself before dumping 
its children'. That's the principle that will allow separating traversal from 
output.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55394



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

[PATCH] D56665: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

[Benjamin Kramer via Phabricator via cfe-commits]

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

This makes sense to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56665



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

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Dan McGregor via Phabricator via cfe-commits]

dankm updated this revision to Diff 181562.
dankm added a comment.

Restored original test case file names.


Repository:
  rC Clang

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

https://reviews.llvm.org/D49466

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/Options.td
  include/clang/Lex/PreprocessorOptions.h
  lib/CodeGen/CGDebugInfo.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/FreeBSD.cpp
  lib/Driver/ToolChains/Gnu.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Lex/PPMacroExpansion.cpp
  test/CodeGen/debug-prefix-map.c
  test/Driver/debug-prefix-map.S
  test/Driver/debug-prefix-map.c
  test/Preprocessor/file_test.c
  test/Preprocessor/file_test.h

Index: test/Preprocessor/file_test.h
===
--- /dev/null
+++ test/Preprocessor/file_test.h
@@ -0,0 +1,2 @@
+filename: __FILE__
+basefile: __BASE_FILE__
Index: test/Preprocessor/file_test.c
===
--- /dev/null
+++ test/Preprocessor/file_test.c
@@ -0,0 +1,22 @@
+// RUN: %clang -E -ffile-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | FileCheck %s
+// RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH/empty -c -o - %s | FileCheck %s
+// RUN: %clang -E -fmacro-prefix-map=%p=/UNLIKELY_PATH=empty -c -o - %s | FileCheck %s -check-prefix CHECK-EVIL
+// RUN: %clang -E -fmacro-prefix-map=%p/= -c -o - %s | FileCheck %s --check-prefix CHECK-REMOVE
+
+filename: __FILE__
+#include "file_test.h"
+
+// CHECK: filename: "/UNLIKELY_PATH/empty{{[/\\]}}file_test.c"
+// CHECK: filename: "/UNLIKELY_PATH/empty{{[/\\]}}file_test.h"
+// CHECK: basefile: "/UNLIKELY_PATH/empty{{[/\\]}}file_test.c"
+// CHECK-NOT: filename:
+
+// CHECK-EVIL: filename: "/UNLIKELY_PATH=empty{{[/\\]}}file_test.c"
+// CHECK-EVIL: filename: "/UNLIKELY_PATH=empty{{[/\\]}}file_test.h"
+// CHECK-EVIL: basefile: "/UNLIKELY_PATH=empty{{[/\\]}}file_test.c"
+// CHECK-EVIL-NOT: filename:
+
+// CHECK-REMOVE: filename: "file_test.c"
+// CHECK-REMOVE: filename: "file_test.h"
+// CHECK-REMOVE: basefile: "file_test.c"
+// CHECK-REMOVE-NOT: filename:
Index: test/Driver/debug-prefix-map.c
===
--- test/Driver/debug-prefix-map.c
+++ test/Driver/debug-prefix-map.c
@@ -1,9 +1,28 @@
-// RUN: %clang -### -fdebug-prefix-map=old %s 2>&1 | FileCheck %s -check-prefix CHECK-INVALID
-// RUN: %clang -### -fdebug-prefix-map=old=new %s 2>&1 | FileCheck %s -check-prefix CHECK-SIMPLE
-// RUN: %clang -### -fdebug-prefix-map=old=n=ew %s 2>&1 | FileCheck %s -check-prefix CHECK-COMPLEX
-// RUN: %clang -### -fdebug-prefix-map=old= %s 2>&1 | FileCheck %s -check-prefix CHECK-EMPTY
+// RUN: %clang -### -fdebug-prefix-map=old %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-INVALID
+// RUN: %clang -### -fmacro-prefix-map=old %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-INVALID
+// RUN: %clang -### -ffile-prefix-map=old %s 2>&1 | FileCheck %s -check-prefix CHECK-FILE-INVALID
 
-// CHECK-INVALID: error: invalid argument 'old' to -fdebug-prefix-map
-// CHECK-SIMPLE: fdebug-prefix-map=old=new
-// CHECK-COMPLEX: fdebug-prefix-map=old=n=ew
-// CHECK-EMPTY: fdebug-prefix-map=old=
+// RUN: %clang -### -fdebug-prefix-map=old=new %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-SIMPLE
+// RUN: %clang -### -fmacro-prefix-map=old=new %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-SIMPLE
+// RUN: %clang -### -ffile-prefix-map=old=new %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-SIMPLE
+// RUN: %clang -### -ffile-prefix-map=old=new %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-SIMPLE
+
+// RUN: %clang -### -fdebug-prefix-map=old=n=ew %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-COMPLEX
+// RUN: %clang -### -fmacro-prefix-map=old=n=ew %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-COMPLEX
+// RUN: %clang -### -ffile-prefix-map=old=n=ew %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-COMPLEX
+// RUN: %clang -### -ffile-prefix-map=old=n=ew %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-COMPLEX
+
+// RUN: %clang -### -fdebug-prefix-map=old= %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-EMPTY
+// RUN: %clang -### -fmacro-prefix-map=old= %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-EMPTY
+// RUN: %clang -### -ffile-prefix-map=old= %s 2>&1 | FileCheck %s -check-prefix CHECK-DEBUG-EMPTY
+// RUN: %clang -### -ffile-prefix-map=old= %s 2>&1 | FileCheck %s -check-prefix CHECK-MACRO-EMPTY
+
+// CHECK-DEBUG-INVALID: error: invalid argument 'old' to -fdebug-prefix-map
+// CHECK-MACRO-INVALID: error: invalid argument 'old' to -fmacro-prefix-map
+// CHECK-FILE-INVALID: error: invalid argument 'old' to -ffile-prefix-map
+// CHECK-DEBUG-SIMPLE: fdebug-prefix-map=old=new
+// CHECK-MACRO-SIMPLE: fmacro-prefix-map=old=new
+// CHECK-DEBUG-COMPLEX: fdebug-prefix-map=old=n=ew
+// CHECK-MACRO-COMPLEX: fmacro-prefix-map=old=n=ew
+// CHECK-DEBUG-EMPTY: fdebug-prefix-map=old=
+// CHECK-MACRO-EMPTY: fmacro-prefix

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Dan McGregor via Phabricator via cfe-commits]

dankm marked 3 inline comments as done.
dankm added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:612
+if (Map.find('=') == StringRef::npos)
+  D.Diag(diag::err_drv_invalid_argument_to_fdebug_prefix_map) << Map;
+else

Lekensteyn wrote:
> joerg wrote:
> > I'd prefer the bailout style here, i.e. `if (...) { D.diag(...); continue }`
> Wouldn't using `if (...) { D.diag(...); continue; }` also skip the 
> `A->claim()` call? Presumably that could result in spurious errors as well 
> about unused arguments?
It would, and did. I had @joerg's suggestion in an earlier patch on this review.


Repository:
  rC Clang

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

https://reviews.llvm.org/D49466



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

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[Russell McClellan via Phabricator via cfe-commits]

russellmcc updated this revision to Diff 181563.
russellmcc added a comment.

Respond to feedback


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

https://reviews.llvm.org/D54881

Files:
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineFormatter.h
  lib/Format/WhitespaceManager.cpp
  lib/Format/WhitespaceManager.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -36,12 +36,12 @@
 SC_DoNotCheck
   };
 
-  std::string format(llvm::StringRef Code,
- const FormatStyle &Style = getLLVMStyle(),
- StatusCheck CheckComplete = SC_ExpectComplete) {
+  std::string formatRange(llvm::StringRef Code, tooling::Range Range,
+  const FormatStyle &Style = getLLVMStyle(),
+  StatusCheck CheckComplete = SC_ExpectComplete) {
 LLVM_DEBUG(llvm::errs() << "---\n");
 LLVM_DEBUG(llvm::errs() << Code << "\n\n");
-std::vector Ranges(1, tooling::Range(0, Code.size()));
+std::vector Ranges(1, std::move(Range));
 FormattingAttemptStatus Status;
 tooling::Replacements Replaces =
 reformat(Style, Code, Ranges, "", &Status);
@@ -57,6 +57,13 @@
 return *Result;
   }
 
+  std::string format(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle(),
+ StatusCheck CheckComplete = SC_ExpectComplete) {
+return formatRange(Code, tooling::Range(0, Code.size()), Style,
+   CheckComplete);
+  }
+
   FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
 Style.ColumnLimit = ColumnLimit;
 return Style;
@@ -110,6 +117,14 @@
 format(Code, Style, SC_DoNotCheck);
   }
 
+  void verifyWithPrefixAndSuffix(llvm::StringRef Expected, llvm::StringRef Code,
+ llvm::StringRef prefix, llvm::StringRef suffix,
+ const FormatStyle &Style = getLLVMStyle()) {
+EXPECT_EQ(llvm::Twine(prefix + Expected + suffix).str(),
+  formatRange(llvm::Twine(prefix + Code + suffix).str(),
+  tooling::Range(prefix.size(), Code.size()), Style));
+  }
+
   int ReplacementCount;
 };
 
@@ -9156,6 +9171,18 @@
Tab);
 }
 
+TEST_F(FormatTest, FormattingIndentationDoesNotAffectOtherLines) {
+  FormatStyle Tab = getLLVMStyleWithColumns(42);
+  Tab.TabWidth = 4;
+  Tab.UseTab = FormatStyle::UT_Always;
+  verifyWithPrefixAndSuffix("\tfoobar();", "foobar();",
+"int f() {\nint a;\n", "\nint b;\n}\n",
+Tab);
+  Tab.UseTab = FormatStyle::UT_Never;
+  verifyWithPrefixAndSuffix("foobar();", "\tfoobar();",
+"int f() {\n\tint a;\n", "\n\tint b;\n}\n", Tab);
+}
+
 TEST_F(FormatTest, CalculatesOriginalColumn) {
   EXPECT_EQ("\"qq\\\n"
 "q\"; /* some\n"
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -49,7 +49,8 @@
   /// into tabs and spaces for some format styles.
   void replaceWhitespace(FormatToken &Tok, unsigned Newlines, unsigned Spaces,
  unsigned StartOfTokenColumn,
- bool InPPDirective = false);
+ bool InPPDirective = false,
+ bool OnlyUntilLastNewline = false);
 
   /// Adds information about an unchangeable token's whitespace.
   ///
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -46,13 +46,23 @@
 void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
   unsigned Spaces,
   unsigned StartOfTokenColumn,
-  bool InPPDirective) {
+  bool InPPDirective,
+  bool OnlyUntilLastNewline) {
   if (Tok.Finalized)
 return;
   Tok.Decision = (Newlines > 0) ? FD_Break : FD_Continue;
-  Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
-   Spaces, StartOfTokenColumn, Newlines, "", "",
-   InPPDirective && !Tok.IsFirst,
+  SourceRange OriginalWhitespaceRange = Tok.WhitespaceRange;
+
+  if (OnlyUntilLastNewline) {
+OriginalWhitespaceRange.setEnd(
+OriginalWhitespaceRange.getBegin().getLocWithOffset(
+Tok.LastNewlineOffset));
+Spaces = 0;
+StartOfTokenColumn = 0;
+  }
+  Changes.push_back(Change(Tok, /*CreateReplacement=*/true,
+ 

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[Russell McClellan via Phabricator via cfe-commits]

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

Thanks for the feedback!


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

https://reviews.llvm.org/D54881



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

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Russell McClellan via Phabricator via cfe-commits]

russellmcc added a comment.

Ping!  Thanks for your consideration.  I'm still quite motivated to land this, 
please let me know if there's anything I can do, or if it's an unwanted patch.


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

https://reviews.llvm.org/D40988



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

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
 comes into mind.


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

https://reviews.llvm.org/D54881



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

[PATCH] D40988: Clang-format: add finer-grained options for putting all arguments on one line

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
 comes into mind.


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

https://reviews.llvm.org/D40988



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

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn accepted this revision.
Lekensteyn added a comment.

Still fine by me, thanks!

As for the commit message, perhaps reference:
https://bugs.llvm.org/show_bug.cgi?id=38135


Repository:
  rC Clang

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

https://reviews.llvm.org/D49466



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

[PATCH] D56632: [analyzer] Track region liveness only through base regions.

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

xazax.hun added a comment.

I really like all this detective work and it would be sad to have it forgotten. 
I would love to see some of your comments in the documentation of symbol reaper.
More specifically:

> When a memory region is live, all its sub-regions and super-regions are 
> automatically live (and vice versa), so it is only necessary to track 
> liveness of base regions.

I think this is non-obvious. If we had all the information it would make 
perfect sense to have a dead field in an alive struct. But since the liveness 
analysis is intraprocedural and we cannot know what happens to a struct when it 
escapes we have no choice but keep the field alive. A more sophisticated 
analysis (which also likely to be more expensive) could have dead fields in an 
alive struct in case the struct never escapes.

> The answer is yes, it does work correctly, because scanReachableSymbols 
> always scans the whole super-region chain of every region. Which means that 
> every time the Environment or the Store marks a region as live, all of its 
> super-regions are added to RegionRoots. However, i would still like to add 
> conversion to the base region into markLive(), because this behavior is 
> something that should be guaranteed by SymbolReaper rather implied by callers 
> manually, even if the callers have to do that anyway.

I did not really follow, but probably my understanding if how memory regions 
work is not correct. If we work with base regions, why do we still need to scan 
the whole super-region chain?


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

https://reviews.llvm.org/D56632



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I think we need to be very careful here. If I've understood this correctly then 
if this functionality is used for anything critical then a manually supplied 
target will be needed when doing cross-linking. For example my host LLD is 
x86_64, is just called ld.lld and will have an inferred x86_64 target triple. 
If someone customises the behaviour of LLD on the triple in a way that doesn't 
get caught by the test suite then we could get some strange breakages when 
doing cross-linking. I personally would prefer to see any option like this not 
try and auto-infer the target unless it can do it reliably and accurately from 
the input objects and I don't know if that is possible for all supported 
targets.

I think this might be better raised on lllvm-dev as I suspect that we need to 
give this wider visibility. I'm not totally opposed to this as I can see that 
--target has some advantages over adding extra emulations, or relying on the 
--target in the compiler driver but I think we need to be careful to define how 
this will interact with the emulation, and what the bounds of what we can 
customise with the option are?


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

https://reviews.llvm.org/D56650



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

[PATCH] D56631: [MSVC Compat] Fix typo correction for inclusion directives.

[Christy Lee via Phabricator via cfe-commits]

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

Thanks for catching that!


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

https://reviews.llvm.org/D56631



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

[PATCH] D55394: Re-order type param children of ObjC nodes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rjmccall.
aaron.ballman added a subscriber: rjmccall.
aaron.ballman added a comment.

Adding @rjmccall as an Obj-C expert to see if he has opinions on the output 
changes. Also, pinging @rsmith in case he'd like to weigh in.

I think the change in behavior here is reasonable, especially because it eases 
the transition to a more generic AST dump traverser. If Richard and John don't 
have strong objections (or don't come back with opinions) in the next week or 
two, I think we should move this forward.




Comment at: test/AST/ast-dump-decl.m:90
 // CHECK-NEXT:   -ObjCProtocol {{.+}} 'P'
+// CHECK-NEXT:   -ObjCTypeParamDecl {{.+}}  col:33 T 'id':'id'
 

steveire wrote:
> dblaikie wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > It seems strange to me to print out the type parameter after the 
> > > > superclass information given the source order. My understanding of the 
> > > > AST dumping order is that we try to keep the order of nodes in source 
> > > > order whenever possible.
> > > That is not really a possible thing to try to do, because the AST dump 
> > > doesn't relate to a single language. It should be seen as language 
> > > independent.
> > > 
> > > The principle I'm follow is that nodes dump themselves in entirety before 
> > > starting to dump their child nodes. That is a principle already followed 
> > > by most nodes. Changing this seems to be low cost, low impact and high 
> > > benefit to the code.
> > >That is not really a possible thing to try to do, because the AST dump 
> > >doesn't relate to a single language. It should be seen as language 
> > >independent.
> > 
> > Is this particular aspect different between the different source languages 
> > Clang supports? (could you give examples?)
> Hmm, maybe that wasn't a good point, particularly because these methods 
> relate to ObjC. 
> 
> Other languages (eg Go https://gobyexample.com/functions) have different 
> order of param types and param names, and different order of params and 
> return types etc. So, the more general AST nodes have less reason for their 
> order based on 'the order in the source'.
> 
> Anyway, the principle I'm following is 'the node dumps itself before dumping 
> its children'. That's the principle that will allow separating traversal from 
> output.
I'm not opposed to changing the order of the nodes so long as the resulting 
output is still sufficiently clear. This test case looks reasonable to me, but 
it also has very little information printed between the `ObjCInterfaceDecl` 
node and the `ObjCTypeParamDecl` node. I don't know whether it's common to have 
long protocol lists in ObjC or not where that distance will widen to the point 
of being hard to understand the relationship between the two nodes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55394



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

[PATCH] D56241: expose a control flag for interger to pointer ext warning

[abdoul-kader keita via Phabricator via cfe-commits]

Kader added a comment.

friendly ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D56241



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

r351075 - [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

[Sam McCall via cfe-commits]

Author: sammccall
Date: Mon Jan 14 09:16:00 2019
New Revision: 351075

URL: http://llvm.org/viewvc/llvm-project?rev=351075&view=rev
Log:
[AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

Summary:
Prior to r351069, lambda classes were traversed or not depending on the
{Function, Class, Namespace, TU} DeclContext containing them.
If it was a function (common case) they were not traversed.
If it was a namespace or TU (top-level lambda) they were traversed as part of
that DeclContext traversal.

r351069 "fixed" RAV to traverse these as part of the LambdaExpr, which is the
right place. But top-level lambdas are now traversed twice.
We fix that as blocks and block captures were apparently fixed in the past.

Maybe it would be nicer to avoid adding the lambda classes to the DeclContext
in the first place, but I can't work out the implications of that.

Reviewers: bkramer, klimek

Subscribers: cfe-commits

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

Modified:
cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp

Modified: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=351075&r1=351074&r2=351075&view=diff
==
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h (original)
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h Mon Jan 14 09:16:00 2019
@@ -1368,9 +1368,14 @@ DEF_TRAVERSE_TYPELOC(PipeType, { TRY_TO(
 template 
 bool 
RecursiveASTVisitor::canIgnoreChildDeclWhileTraversingDeclContext(
 const Decl *Child) {
-  // BlockDecls and CapturedDecls are traversed through BlockExprs and
-  // CapturedStmts respectively.
-  return isa(Child) || isa(Child);
+  // BlockDecls are traversed through BlockExprs,
+  // CapturedDecls are traversed through CapturedStmts.
+  if (isa(Child) || isa(Child))
+return true;
+  // Lambda classes are traversed through LambdaExprs.
+  if (const CXXRecordDecl* Cls = dyn_cast(Child))
+return Cls->isLambda();
+  return false;
 }
 
 template 

Modified: cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp?rev=351075&r1=351074&r2=351075&view=diff
==
--- cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp 
(original)
+++ cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp Mon Jan 
14 09:16:00 2019
@@ -65,6 +65,17 @@ TEST(RecursiveASTVisitor, LambdaInLambda
   EXPECT_FALSE(Visitor.allClassesHaveBeenTraversed());
 }
 
+TEST(RecursiveASTVisitor, TopLevelLambda) {
+  LambdaExprVisitor Visitor;
+  Visitor.VisitImplicitCode = true;
+  Visitor.ExpectMatch("", 1, 10);
+  Visitor.ExpectMatch("", 1, 14);
+  EXPECT_TRUE(Visitor.runOver("auto x = []{ [] {}; };",
+  LambdaExprVisitor::Lang_CXX11));
+  EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
+  EXPECT_TRUE(Visitor.allClassesHaveBeenTraversed());
+}
+
 TEST(RecursiveASTVisitor, VisitsLambdaExprAndImplicitClass) {
   LambdaExprVisitor Visitor;
   Visitor.VisitImplicitCode = true;


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

[PATCH] D56665: [AST] Fix double-traversal of code in top-level lambdas in RAV(implicit = yes).

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351075: [AST] Fix double-traversal of code in top-level 
lambdas in RAV(implicit = yes). (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56665

Files:
  cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
  cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp


Index: cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
===
--- cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
+++ cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
@@ -65,6 +65,17 @@
   EXPECT_FALSE(Visitor.allClassesHaveBeenTraversed());
 }
 
+TEST(RecursiveASTVisitor, TopLevelLambda) {
+  LambdaExprVisitor Visitor;
+  Visitor.VisitImplicitCode = true;
+  Visitor.ExpectMatch("", 1, 10);
+  Visitor.ExpectMatch("", 1, 14);
+  EXPECT_TRUE(Visitor.runOver("auto x = []{ [] {}; };",
+  LambdaExprVisitor::Lang_CXX11));
+  EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
+  EXPECT_TRUE(Visitor.allClassesHaveBeenTraversed());
+}
+
 TEST(RecursiveASTVisitor, VisitsLambdaExprAndImplicitClass) {
   LambdaExprVisitor Visitor;
   Visitor.VisitImplicitCode = true;
Index: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
===
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
@@ -1368,9 +1368,14 @@
 template 
 bool 
RecursiveASTVisitor::canIgnoreChildDeclWhileTraversingDeclContext(
 const Decl *Child) {
-  // BlockDecls and CapturedDecls are traversed through BlockExprs and
-  // CapturedStmts respectively.
-  return isa(Child) || isa(Child);
+  // BlockDecls are traversed through BlockExprs,
+  // CapturedDecls are traversed through CapturedStmts.
+  if (isa(Child) || isa(Child))
+return true;
+  // Lambda classes are traversed through LambdaExprs.
+  if (const CXXRecordDecl* Cls = dyn_cast(Child))
+return Cls->isLambda();
+  return false;
 }
 
 template 


Index: cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
===
--- cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
+++ cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaExpr.cpp
@@ -65,6 +65,17 @@
   EXPECT_FALSE(Visitor.allClassesHaveBeenTraversed());
 }
 
+TEST(RecursiveASTVisitor, TopLevelLambda) {
+  LambdaExprVisitor Visitor;
+  Visitor.VisitImplicitCode = true;
+  Visitor.ExpectMatch("", 1, 10);
+  Visitor.ExpectMatch("", 1, 14);
+  EXPECT_TRUE(Visitor.runOver("auto x = []{ [] {}; };",
+  LambdaExprVisitor::Lang_CXX11));
+  EXPECT_TRUE(Visitor.allBodiesHaveBeenTraversed());
+  EXPECT_TRUE(Visitor.allClassesHaveBeenTraversed());
+}
+
 TEST(RecursiveASTVisitor, VisitsLambdaExprAndImplicitClass) {
   LambdaExprVisitor Visitor;
   Visitor.VisitImplicitCode = true;
Index: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
===
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
@@ -1368,9 +1368,14 @@
 template 
 bool RecursiveASTVisitor::canIgnoreChildDeclWhileTraversingDeclContext(
 const Decl *Child) {
-  // BlockDecls and CapturedDecls are traversed through BlockExprs and
-  // CapturedStmts respectively.
-  return isa(Child) || isa(Child);
+  // BlockDecls are traversed through BlockExprs,
+  // CapturedDecls are traversed through CapturedStmts.
+  if (isa(Child) || isa(Child))
+return true;
+  // Lambda classes are traversed through LambdaExprs.
+  if (const CXXRecordDecl* Cls = dyn_cast(Child))
+return Cls->isLambda();
+  return false;
 }
 
 template 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits