r348738 - [X86] Remove the addcarry builtins. Leaving only the addcarryx builtins since that matches gcc.

[Craig Topper via cfe-commits]

Author: ctopper
Date: Sun Dec  9 22:07:59 2018
New Revision: 348738

URL: http://llvm.org/viewvc/llvm-project?rev=348738=rev
Log:
[X86] Remove the addcarry builtins. Leaving only the addcarryx builtins since 
that matches gcc.

The addcarry and addcarryx builtins do the same thing. The only difference is 
that addcarryx previously required adx feature.

This commit removes the adx feature check from addcarryx and removes the 
addcarry builtin. This matches the builtins that gcc has. We don't guarantee 
compatibility in builtins, but we generally try to be consistent if its not a 
burden.

Modified:
cfe/trunk/include/clang/Basic/BuiltinsX86.def
cfe/trunk/include/clang/Basic/BuiltinsX86_64.def
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/Headers/adxintrin.h
cfe/trunk/test/CodeGen/adc-builtins.c
cfe/trunk/test/CodeGen/adx-builtins.c

Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/BuiltinsX86.def?rev=348738=348737=348738=diff
==
--- cfe/trunk/include/clang/Basic/BuiltinsX86.def (original)
+++ cfe/trunk/include/clang/Basic/BuiltinsX86.def Sun Dec  9 22:07:59 2018
@@ -719,8 +719,7 @@ TARGET_BUILTIN(__builtin_ia32_wbinvd, "v
 TARGET_BUILTIN(__builtin_ia32_wbnoinvd, "v", "n", "wbnoinvd")
 
 // ADX
-TARGET_BUILTIN(__builtin_ia32_addcarryx_u32, "UcUcUiUiUi*", "n", "adx")
-TARGET_BUILTIN(__builtin_ia32_addcarry_u32, "UcUcUiUiUi*", "n", "")
+TARGET_BUILTIN(__builtin_ia32_addcarryx_u32, "UcUcUiUiUi*", "n", "")
 TARGET_BUILTIN(__builtin_ia32_subborrow_u32, "UcUcUiUiUi*", "n", "")
 
 // RDSEED

Modified: cfe/trunk/include/clang/Basic/BuiltinsX86_64.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/BuiltinsX86_64.def?rev=348738=348737=348738=diff
==
--- cfe/trunk/include/clang/Basic/BuiltinsX86_64.def (original)
+++ cfe/trunk/include/clang/Basic/BuiltinsX86_64.def Sun Dec  9 22:07:59 2018
@@ -76,8 +76,7 @@ TARGET_BUILTIN(__builtin_ia32_incsspq, "
 TARGET_BUILTIN(__builtin_ia32_rdsspq, "ULLiULLi", "n", "shstk")
 TARGET_BUILTIN(__builtin_ia32_wrssq, "vULLiv*", "n", "shstk")
 TARGET_BUILTIN(__builtin_ia32_wrussq, "vULLiv*", "n", "shstk")
-TARGET_BUILTIN(__builtin_ia32_addcarryx_u64, "UcUcULLiULLiULLi*", "n", "adx")
-TARGET_BUILTIN(__builtin_ia32_addcarry_u64, "UcUcULLiULLiULLi*", "n", "")
+TARGET_BUILTIN(__builtin_ia32_addcarryx_u64, "UcUcULLiULLiULLi*", "n", "")
 TARGET_BUILTIN(__builtin_ia32_subborrow_u64, "UcUcULLiULLiULLi*", "n", "")
 TARGET_BUILTIN(__builtin_ia32_rdrand64_step, "UiULLi*", "n", "rdrnd")
 TARGET_BUILTIN(__builtin_ia32_rdseed64_step, "UiULLi*", "n", "rdseed")

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=348738=348737=348738=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Sun Dec  9 22:07:59 2018
@@ -10930,30 +10930,22 @@ Value *CodeGenFunction::EmitX86BuiltinEx
   }
   case X86::BI__builtin_ia32_addcarryx_u32:
   case X86::BI__builtin_ia32_addcarryx_u64:
-  case X86::BI__builtin_ia32_addcarry_u32:
-  case X86::BI__builtin_ia32_addcarry_u64:
   case X86::BI__builtin_ia32_subborrow_u32:
   case X86::BI__builtin_ia32_subborrow_u64: {
 Intrinsic::ID IID;
 switch (BuiltinID) {
 default: llvm_unreachable("Unsupported intrinsic!");
 case X86::BI__builtin_ia32_addcarryx_u32:
-  IID = Intrinsic::x86_addcarryx_u32;
+  IID = Intrinsic::x86_addcarry_32;
   break;
 case X86::BI__builtin_ia32_addcarryx_u64:
-  IID = Intrinsic::x86_addcarryx_u64;
-  break;
-case X86::BI__builtin_ia32_addcarry_u32:
-  IID = Intrinsic::x86_addcarry_u32;
-  break;
-case X86::BI__builtin_ia32_addcarry_u64:
-  IID = Intrinsic::x86_addcarry_u64;
+  IID = Intrinsic::x86_addcarry_64;
   break;
 case X86::BI__builtin_ia32_subborrow_u32:
-  IID = Intrinsic::x86_subborrow_u32;
+  IID = Intrinsic::x86_subborrow_32;
   break;
 case X86::BI__builtin_ia32_subborrow_u64:
-  IID = Intrinsic::x86_subborrow_u64;
+  IID = Intrinsic::x86_subborrow_64;
   break;
 }
 

Modified: cfe/trunk/lib/Headers/adxintrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/adxintrin.h?rev=348738=348737=348738=diff
==
--- cfe/trunk/lib/Headers/adxintrin.h (original)
+++ cfe/trunk/lib/Headers/adxintrin.h Sun Dec  9 22:07:59 2018
@@ -53,7 +53,7 @@ static __inline unsigned char __DEFAULT_
 _addcarry_u32(unsigned char __cf, unsigned int __x, unsigned int __y,
   unsigned int *__p)
 {
-  return __builtin_ia32_addcarry_u32(__cf, __x, __y, __p);
+  return 

[PATCH] D55415: Revert removal of tidy plugin support from libclang

[Benjamin Kramer via Phabricator via cfe-commits]

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

I think that's a fair point for bringing it back for now. It's not supported 
though and we will get rid of it eventually.


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

https://reviews.llvm.org/D55415



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

[PATCH] D55484: ComputeLineNumbers: delete SSE2 vectorization

[Benjamin Kramer via Phabricator via cfe-commits]

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

The performance difference on preprocessing huge files was tiny back then, 
doesn't surprise me that it disappeared. What did you test this on?

Dropping it is fine with me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55484



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

[PATCH] D47344: LWG 2843 "Unclear behavior of std::pmr::memory_resource::do_allocate()"

[Eric Fiselier via Phabricator via cfe-commits]

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

I don't think this patch is correct or desirable any longer.

I think what we should do is throw an exception right away if 
`__is_overaligned_for_new(align)`, and not even try to allocate. The behavior 
is quite surprising anyway. If new *happens* to return correctly aligned 
memory, then the call spuriously succeeds seemingly at random.




Comment at: src/experimental/memory_resource.cpp:29
+static bool is_aligned_to(void *ptr, size_t align)
+{
+void *p2 = ptr;

Quuxplusone wrote:
> EricWF wrote:
> > Wait, can't this be written `reinterpret_cast(ptr) % align == 0`?
> Yes on sane platforms, but that's technically implementation-defined 
> behavior: the low bits of the uintptr_t may not correspond to the "alignment" 
> of the original pointer (for example, on some hypothetical tagged-pointer 
> architecture?). I don't know if libc++ cares about those platforms, but it 
> seems like `std::align` was added specifically to solve this problem in a 
> portable way, so I thought it would be best to use it.
> 
> If you reply and say "yeah but please change it anyway," I'll change it 
> anyway. :)
Please change it anyway. This isn't what `std::align` was meant to do. Plus 
`std::align` suffers from the same problems.



Comment at: src/experimental/memory_resource.cpp:49
+if (!is_aligned_to(result, align)) {
+_VSTD::__libcpp_deallocate(result, bytes, align);
+__throw_bad_alloc();

Oh boy... God knows if this call is correct since the value it's passing for 
`align` doesn't match the pointers alignment.



Comment at: src/experimental/memory_resource.cpp:59
+
+bool do_is_equal(const memory_resource& other) const _NOEXCEPT override
+{ return  == this; }

These whitespace/naming changes are unrelated.



Comment at: src/experimental/memory_resource.cpp:68
 {
-public:
-~__null_memory_resource_imp() = default;
-
-protected:
-virtual void* do_allocate(size_t, size_t) {
-__throw_bad_alloc();
-}
-virtual void do_deallocate(void *, size_t, size_t) {}
-virtual bool do_is_equal(memory_resource const & __other) const _NOEXCEPT
-{ return &__other == this; }
+void *do_allocate(size_t, size_t) override { __throw_bad_alloc(); }
+void do_deallocate(void *, size_t, size_t) override {}

These changes are all unrelated. 


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D47344



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

[PATCH] D47111: : Implement monotonic_buffer_resource.

[Eric Fiselier via Phabricator via cfe-commits]

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

Some of the tests aren't passing with Trunk clang. Please fix these.




Comment at: include/experimental/memory_resource:427
+{
+static const size_t __default_buffer_capacity = 1024;
+static const size_t __default_buffer_alignment = 16;

`constexpr` these constants if they're const?



Comment at: 
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp:32
+void *ret = r1.allocate(2048, big_alignment);
+assert(ret != nullptr);
+assert(globalMemCounter.checkNewCalledEq(1));

This check isn't passing with ToT clang.



Comment at: 
test/std/experimental/memory/memory.resource.monotonic.buffer/monotonic.buffer.mem/allocate_overaligned_request.pass.cpp:43
+assert(globalMemCounter.checkNewCalledEq(2));
+// assert(globalMemCounter.last_new_align >= 4);
+// assert(globalMemCounter.last_new_align < big_alignment);

Either remove this or uncomment them. Preferably the latter. More test coverage 
is better.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D47111



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

[PATCH] D55500: [Builtins] Implement __builtin_is_constant_evaluated for use in C++2a

[Eric Fiselier via Phabricator via cfe-commits]

EricWF created this revision.
EricWF added reviewers: rsmith, MaskRay, bruno, void.
Herald added a subscriber: kristina.

This patch implements `__builtin_is_constant_evaluated` as specifier by P0595R2 
. It is built on the back of Bill Wendling's work 
for `__builtin_constant_p()`.

More tests to come, but early feedback is appreciated.

I plan to implement warnings for common mis-usages like those belowe in a 
following patch:

  void foo(int x) {
if constexpr (std::is_constant_evaluated())) { // condition is always 
`true`. Should use plain `if` instead.
 foo_constexpr(x);
} else {
  foo_runtime(x);
}
  }




Repository:
  rC Clang

https://reviews.llvm.org/D55500

Files:
  include/clang/Basic/Builtins.def
  lib/AST/ExprConstant.cpp
  lib/Basic/Builtins.cpp
  test/CodeGenCXX/builtin-is-constant-evaluated.cpp
  test/Sema/builtins.c
  test/SemaCXX/builtin-is-constant-evaluated.cpp

Index: test/SemaCXX/builtin-is-constant-evaluated.cpp
===
--- /dev/null
+++ test/SemaCXX/builtin-is-constant-evaluated.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -std=c++2a -verify %s -fcxx-exceptions -triple=x86_64-linux-gnu
+
+using size_t = decltype(sizeof(int));
+
+namespace std {
+inline constexpr bool is_constant_evaluated() noexcept {
+  return __builtin_is_constant_evaluated();
+}
+} // namespace std
+
+extern int dummy; // expected-note 1+ {{declared here}}
+
+static_assert(__builtin_is_constant_evaluated());
+
+constexpr bool b = __builtin_is_constant_evaluated();
+static_assert(b);
+
+const int n = __builtin_is_constant_evaluated() ? 4 : dummy;
+static_assert(n == 4);
+constexpr int cn = __builtin_is_constant_evaluated() ? 11 : dummy;
+static_assert(cn == 11);
+// expected-error@+1 {{'bn' must be initialized by a constant expression}}
+constexpr int bn = __builtin_is_constant_evaluated() ? dummy : 42; // expected-note {{non-const variable 'dummy' is not allowed}}
+
+
+const int n2 = __builtin_is_constant_evaluated() ? dummy : 42; // expected-note {{declared here}}
+static_assert(n2 == 42); // expected-error {{static_assert expression is not an integral constant}}
+// expected-note@-1 {{initializer of 'n2' is not a constant expression}}
+
+template struct Templ { static_assert(V);};
+Templ<__builtin_is_constant_evaluated()> x; // type X
+
+template 
+void test_if_constexpr() {
+  if constexpr (__builtin_is_constant_evaluated()) {
+static_assert(__is_same(T, int));
+  } else {
+using Test = typename T::DOES_NOT_EXIST;
+  }
+}
+template void test_if_constexpr();
+
+void test_array_decl() {
+  char x[__builtin_is_constant_evaluated() + std::is_constant_evaluated()];
+  static_assert(sizeof(x) == 2, "");
+}
+
+void test_case_stmt(int x) {
+  switch (x) {
+  case 0:// OK
+  case __builtin_is_constant_evaluated():// expected-note {{previous case}}
+  case std::is_constant_evaluated() + __builtin_is_constant_evaluated(): // expected-note {{previous case}}
+  case 1:// expected-error {{duplicate case value '1'}}
+  case 2:// expected-error {{duplicate case value '2'}}
+break;
+  }
+}
+
+constexpr size_t good_array_size() {
+  return std::is_constant_evaluated() ? 42 : static_cast(-1);
+}
+
+constexpr size_t bad_array_size() {
+  return std::is_constant_evaluated() ? static_cast(-1) : 13;
+}
+
+template 
+constexpr T require_constexpr(T v) {
+  if (!std::is_constant_evaluated())
+throw "BOOM";
+  return v;
+}
+
+void test_new_expr() {
+  constexpr size_t TooLarge = -1;
+  auto *x = new int[std::is_constant_evaluated() ? 1 : TooLarge];  // expected-error {{array is too large}}
+  auto *x2 = new int[std::is_constant_evaluated() ? TooLarge : 1]; // OK
+  auto *y = new int[1][std::is_constant_evaluated() ? TooLarge : 1]{}; // expected-error {{array is too large}}
+  auto *y2 = new int[1][require_constexpr(42)];
+}
Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -296,3 +296,9 @@
   memcpy(buf, src, 11); // expected-warning{{'memcpy' will always overflow; destination buffer has size 10, but size argument is 11}}
   my_memcpy(buf, src, 11); // expected-warning{{'__builtin___memcpy_chk' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
+
+// Test that __builtin_is_constant_evaluated() is not allowed in C
+int test_cxx_builtin() {
+  // expected-error@+1 {{use of unknown builtin '__builtin_is_constant_evaluated'}}
+  return __builtin_is_constant_evaluated();
+}
Index: test/CodeGenCXX/builtin-is-constant-evaluated.cpp
===
--- /dev/null
+++ test/CodeGenCXX/builtin-is-constant-evaluated.cpp
@@ 

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

Meinersbur wrote:
> dblaikie wrote:
> > Meinersbur wrote:
> > > dblaikie wrote:
> > > > This might be more legible as "if (std::get<0>(Pair) || 
> > > > std::get<1>(Pair))", I think? (optionally using "hasValue", if 
> > > > preferred - but comparing the hasValues to each other, rather than 
> > > > testing if either is false, seems a bit opaque to me at least)
> > > The idea was 'if the items are unequal, the list is unequal', where when 
> > > either one is undefined, but not the the other, is considered unequal. 
> > > The test for the elements themselves have the same structure (Cand1ID != 
> > > Cand2ID).
> > Sorry, looks like I made a mistake in the suggested change - should be a ! 
> > before each of the gets (I wonder if the change as you have it now/as I 
> > originally suggested, is causing any test failures - one really hopes it 
> > does... because as written it looks like it'd cause this loop to always 
> > return false on the first iteration):
> > 
> >   if (!std::get<0>(Pair) || !std::get<1>(Pair))
> > 
> > & thanks for the explanation about the approach you were using - I see 
> > where you're coming from. I'd personally still lean this ^ way, I think.
> > 
> > (maybe if we get super ridiculous one day, and have a monadic (not sure if 
> > that's the right word) sort of conditional accessor for Optional (where you 
> > pass in a lambda over T, returning U, and the result is Optional) we 
> > could write this in terms of that & then the != would fit right in... 
> > though would be a bit verbose/quirky to be sure)
> I knew exactly what you suggested -- I considered before going with the `!=` 
> version -- it seems It also only saw what I wanted to see. I still just 
> copy from your comment to save some keystrokes. Maybe the `!=` is less 
> error-prone, as just demonstrated? Test cases did not fail.
I'd still probably go with the !A || !B form - and either encourage you to add 
the missing test coverage, or maybe go find who owns/contributed/reviewed the 
code to request/suggest some testing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

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



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

dblaikie wrote:
> Meinersbur wrote:
> > dblaikie wrote:
> > > This might be more legible as "if (std::get<0>(Pair) || 
> > > std::get<1>(Pair))", I think? (optionally using "hasValue", if preferred 
> > > - but comparing the hasValues to each other, rather than testing if 
> > > either is false, seems a bit opaque to me at least)
> > The idea was 'if the items are unequal, the list is unequal', where when 
> > either one is undefined, but not the the other, is considered unequal. The 
> > test for the elements themselves have the same structure (Cand1ID != 
> > Cand2ID).
> Sorry, looks like I made a mistake in the suggested change - should be a ! 
> before each of the gets (I wonder if the change as you have it now/as I 
> originally suggested, is causing any test failures - one really hopes it 
> does... because as written it looks like it'd cause this loop to always 
> return false on the first iteration):
> 
>   if (!std::get<0>(Pair) || !std::get<1>(Pair))
> 
> & thanks for the explanation about the approach you were using - I see where 
> you're coming from. I'd personally still lean this ^ way, I think.
> 
> (maybe if we get super ridiculous one day, and have a monadic (not sure if 
> that's the right word) sort of conditional accessor for Optional (where you 
> pass in a lambda over T, returning U, and the result is Optional) we could 
> write this in terms of that & then the != would fit right in... though would 
> be a bit verbose/quirky to be sure)
I knew exactly what you suggested -- I considered before going with the `!=` 
version -- it seems It also only saw what I wanted to see. I still just 
copy from your comment to save some keystrokes. Maybe the `!=` is less 
error-prone, as just demonstrated? Test cases did not fail.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

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

Overall I'm not sure this construct/pattern improves readability, but not too 
fussed either way.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 177447.
Meinersbur added a comment.

- Fix xor
- Store tuple elements in variables


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468

Files:
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8977,25 +8977,28 @@
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
 
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (!Cand1A)
   return Comparison::Worse;
+if (!Cand2A)
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*Cand2A)->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate ,


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,30 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+Optional Cand1A = std::get<0>(Pair);
+Optional Cand2A = std::get<1>(Pair);
+
+// Return false if the number of enable_if attributes is different.
+if (!Cand1A || !Cand2A)
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+

[PATCH] D47111: : Implement monotonic_buffer_resource.

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

@ericwf ping? This still needs someone to land it; I don't have commit privs. 
Thanks!


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D47111



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

[PATCH] D40218: [Clang] Add __builtin_launder

[Eric Fiselier via Phabricator via cfe-commits]

EricWF updated this revision to Diff 177445.
EricWF marked 6 inline comments as done.
EricWF added a comment.

Address review comments.

@rsmith I think this is good to go. Just need your input on one change.


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

https://reviews.llvm.org/D40218

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Sema/SemaChecking.cpp
  test/CodeGen/builtins.c
  test/CodeGenCXX/builtin-launder.cpp
  test/Preprocessor/feature_tests.c
  test/Sema/builtins.c
  test/SemaCXX/builtins.cpp

Index: test/SemaCXX/builtins.cpp
===
--- test/SemaCXX/builtins.cpp
+++ test/SemaCXX/builtins.cpp
@@ -53,3 +53,95 @@
 void synchronize_args() {
   __sync_synchronize(0); // expected-error {{too many arguments}}
 }
+
+namespace test_launder {
+#define TEST_TYPE(Ptr, Type) \
+  static_assert(__is_same(decltype(__builtin_launder(Ptr)), Type), "expected same type")
+
+struct Dummy {};
+
+using FnType = int(char);
+using MemFnType = int (Dummy::*)(char);
+using ConstMemFnType = int (Dummy::*)() const;
+
+void foo() {}
+
+void test_builtin_launder_diags(void *vp, const void *cvp, FnType *fnp,
+MemFnType mfp, ConstMemFnType cmfp, int ()[5]) {
+  __builtin_launder(vp);   // expected-error {{void pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(cvp);  // expected-error {{void pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(fnp);  // expected-error {{function pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(mfp);  // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(cmfp); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  (void)__builtin_launder();
+  __builtin_launder(42);  // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(nullptr); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+  __builtin_launder(foo); // expected-error {{function pointer argument to '__builtin_launder' is not allowed}}
+  (void)__builtin_launder(Arr);
+}
+
+void test_builtin_launder(char *p, const volatile int *ip, const float *,
+  double *__restrict dp) {
+  int x;
+  __builtin_launder(x); // expected-error {{non-pointer argument to '__builtin_launder' is not allowed}}
+
+  TEST_TYPE(p, char*);
+  TEST_TYPE(ip, const volatile int*);
+  TEST_TYPE(fp, const float*);
+  TEST_TYPE(dp, double *__restrict);
+
+  char *d = __builtin_launder(p);
+  const volatile int *id = __builtin_launder(ip);
+  int *id2 = __builtin_launder(ip); // expected-error {{cannot initialize a variable of type 'int *' with an rvalue of type 'const volatile int *'}}
+  const float* fd = __builtin_launder(fp);
+}
+
+void test_launder_return_type(const int ()[101], int ()[42][13],
+  void (**)()) {
+  TEST_TYPE(ArrayRef, const int *);
+  TEST_TYPE(MArrRef, int(*)[13]);
+  TEST_TYPE(FuncPtrRef, void (**)());
+}
+
+template 
+constexpr Tp *test_constexpr_launder(Tp *tp) {
+  return __builtin_launder(tp);
+}
+constexpr int const_int = 42;
+constexpr int const_int2 = 101;
+constexpr const int *const_ptr = test_constexpr_launder(_int);
+static_assert(_int == const_ptr, "");
+static_assert(const_ptr != test_constexpr_launder(_int2), "");
+
+void test_non_constexpr() {
+  constexpr int i = 42;// expected-note {{declared here}}
+  constexpr const int *ip = __builtin_launder(); // expected-error {{constexpr variable 'ip' must be initialized by a constant expression}}
+  // expected-note@-1 {{pointer to 'i' is not a constant expression}}
+}
+
+constexpr bool test_in_constexpr(const int ) {
+  return (__builtin_launder() == );
+}
+
+static_assert(test_in_constexpr(const_int), "");
+void f() {
+  constexpr int i = 42;
+  static_assert(test_in_constexpr(i), "");
+}
+
+struct Incomplete; // expected-note {{forward declaration}}
+struct IncompleteMember {
+  Incomplete 
+};
+void test_incomplete(Incomplete *i, IncompleteMember *im) {
+  // expected-error@+1 {{incomplete type 'test_launder::Incomplete' where a complete type is required}}
+  __builtin_launder(i);
+  __builtin_launder(); // OK
+  __builtin_launder(im); // OK
+}
+
+void test_noexcept(int *i) {
+  static_assert(noexcept(__builtin_launder(i)), "");
+}
+#undef TEST_TYPE
+} // end namespace test_launder
Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -258,6 +258,24 @@
 return buf;
 }
 
+typedef void (fn_t)(int);
+
+void test_builtin_launder(char *p, void *vp, const void *cvp,
+  const volatile int *ip, float *restrict fp,
+  fn_t 

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:8979
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (std::get<0>(Pair) == None)
   return Comparison::Worse;

Meinersbur wrote:
> dblaikie wrote:
> > I'd probably write this as "if (!std::get<0>(Pair))" - but I understand 
> > that opinions differ on such things easily enough.
> Here I tried do be explicit that it's an `llvm::Optional` and not testing for 
> null pointers (or whatever the payload of the llvm::Optional is, which might 
> itself have an overload bool conversion operator). It seemed worthwhile 
> especially because `llvm::Optional` itself does not appear itself around 
> these lines.
*nod* Up to you - though, given std::get<0> and std::get<1> appear twice in 
this loop, perhaps it makes sense to pull them out into named variables and 
then you can name the Optional too?



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

Meinersbur wrote:
> dblaikie wrote:
> > This might be more legible as "if (std::get<0>(Pair) || 
> > std::get<1>(Pair))", I think? (optionally using "hasValue", if preferred - 
> > but comparing the hasValues to each other, rather than testing if either is 
> > false, seems a bit opaque to me at least)
> The idea was 'if the items are unequal, the list is unequal', where when 
> either one is undefined, but not the the other, is considered unequal. The 
> test for the elements themselves have the same structure (Cand1ID != Cand2ID).
Sorry, looks like I made a mistake in the suggested change - should be a ! 
before each of the gets (I wonder if the change as you have it now/as I 
originally suggested, is causing any test failures - one really hopes it 
does... because as written it looks like it'd cause this loop to always return 
false on the first iteration):

  if (!std::get<0>(Pair) || !std::get<1>(Pair))

& thanks for the explanation about the approach you were using - I see where 
you're coming from. I'd personally still lean this ^ way, I think.

(maybe if we get super ridiculous one day, and have a monadic (not sure if 
that's the right word) sort of conditional accessor for Optional (where you 
pass in a lambda over T, returning U, and the result is Optional) we could 
write this in terms of that & then the != would fit right in... though would be 
a bit verbose/quirky to be sure)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D40218: [Clang] Add __builtin_launder

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 22 inline comments as done.
EricWF added inline comments.



Comment at: lib/CodeGen/CGBuiltin.cpp:1425-1426
+
+  // FIXME: We either have an incomplete class type, or we have a class 
template
+  // whose instantiation has not been forced. Example:
+  //

rsmith wrote:
> I think it's a bug that `launder` doesn't require `T` to be a complete type. 
> Can you file an LWG issue?
> 
> We should also decide whether we want to proactively fix this issue (require 
> the type to be complete from the `Sema` checking of the builtin and assert 
> that it's defined here) or not.
Apparently I misread the specification. The blanket wording in 
[[res.on.functions](http://eel.is/c++draft/res.on.functions#2.5)]  already 
prohibits this.

Though it's worth noting that GCC doesn't enforce this requirement (nor the one 
about function pointers).



Comment at: lib/CodeGen/CGBuiltin.cpp:1451
+return false;
+  llvm::DenseSet Seen;
+  return TypeRequiresBuiltinLaunderImp(CGM.getContext(), Ty, Seen);

rsmith wrote:
> Would `SmallPtrSet` be a better choice here?
I have no idea. I'm ignorant to the internals of both containers. Since you 
asked the question I'll assume it is and make the change.
If you would like, I can do further investigation upon request.



Comment at: lib/Sema/SemaChecking.cpp:885-887
+  // Don't perform LValue conversions since they may strip things like the
+  // restrict qualifier
+  ExprResult Arg = S.DefaultFunctionArrayConversion(OrigArg);

rsmith wrote:
> Instead of performing some of the conversions here and some of them as part 
> of initialization, I think it'd be more obvious to compute the builtin's 
> parameter type here (which is the type of the argument if it's not of array 
> [or function] type, and the decayed type of the argument otherwise), and do 
> the decay and lvalue-to-rvalue conversion as part of the parameter 
> initialization below.
> 
> The current code arrangement (and especially this comment) leaves a reader 
> thinking "but you *need* an lvalue-to-rvalue conversion if the argument is an 
> lvalue".
Ack.

I think I've implemented what you requested. Could you please verify?



Comment at: lib/Sema/SemaChecking.cpp:935
   return true;
+
 }

rsmith wrote:
> Did you mean to add this blank line?
Oh Richard, you're too kind. Of course I didn't mean to do that.



Comment at: test/CodeGen/builtins.c:404-409
+  // CHECK: entry
+  // CHECK-NEXT: %p.addr = alloca i32*
+  // CHECK-NEXT: %d = alloca i32*
+  // CHECK-NEXT: store i32* %p, i32** %p.addr, align 8
+  // CHECK-NEXT: [[TMP:%.*]] = load i32*, i32** %p.addr
+  // CHECK-NEXT: store i32* [[TMP]], i32** %d

rsmith wrote:
> This test is not robust against minor IR differences such as variable or 
> basic block names changing (some of these change in a release build), and is 
> testing things that are not related to this builtin (eg, that we produce an 
> alloca for a function parameter and its relative order to an alloca for a 
> local variable).
> 
> I would remove everything here other than the load and the store, and add an 
> explicit check that we don't generate a launder call:
> 
> ```
>   // CHECK: [[TMP:%.*]] = load i32*,
>   // CHECK-NOT: @llvm.launder
>   // CHECK: store i32* [[TMP]],
> ```
If you're referring to the behavior of discarding value names, Clang CC1 only 
does that if `-discard-value-names` is explicitly passed, regardless of how the 
compiler is built, and so the names should be stable. (The Clang driver still 
conditionally passes `-discard-value-names` in release builds).

Are there other IR changes you have in mind?

None the less, I'll simply this test as requested. 



Comment at: test/CodeGenCXX/builtin-launder.cpp:16
+extern "C" void test_builtin_launder_virtual_fn(TestVirtualFn *p) {
+  // CHECK: entry
+  // CHECK-NEXT: %p.addr = alloca [[TYPE:%.*]], align 8

rsmith wrote:
> This is likewise likely to fail with a release build of clang.
I just verified that they pass in both release and debug builds. I previously 
changed Clang and LLVM to make the label names consistent between the two 
configurations for the purposes of testing.

I'll still attempt to clean unnecessary checks up.



Comment at: test/SemaCXX/builtins.cpp:120
+  constexpr int i = 42;
+  // FIXME: Should this work? Since `` doesn't.
+  static_assert(test_in_constexpr(i), "");

rsmith wrote:
> `` doesn't what?
That's a great question. Not sure what I was up to when I wrote that. I tried a 
couple of things I though I might have been thinking, but they all passed.

Removing the comment.


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

https://reviews.llvm.org/D40218



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:8979
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (std::get<0>(Pair) == None)
   return Comparison::Worse;

dblaikie wrote:
> I'd probably write this as "if (!std::get<0>(Pair))" - but I understand that 
> opinions differ on such things easily enough.
Here I tried do be explicit that it's an `llvm::Optional` and not testing for 
null pointers (or whatever the payload of the llvm::Optional is, which might 
itself have an overload bool conversion operator). It seemed worthwhile 
especially because `llvm::Optional` itself does not appear itself around these 
lines.



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

dblaikie wrote:
> This might be more legible as "if (std::get<0>(Pair) || std::get<1>(Pair))", 
> I think? (optionally using "hasValue", if preferred - but comparing the 
> hasValues to each other, rather than testing if either is false, seems a bit 
> opaque to me at least)
The idea was 'if the items are unequal, the list is unequal', where when either 
one is undefined, but not the the other, is considered unequal. The test for 
the elements themselves have the same structure (Cand1ID != Cand2ID).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 177444.
Meinersbur marked 4 inline comments as done.
Meinersbur added a comment.

- Address @dblaikie's review


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468

Files:
  lib/Sema/SemaOverload.cpp
  lib/Serialization/ASTReaderDecl.cpp


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,27 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != 
BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair) || std::get<1>(Pair))
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, A->getASTContext(), 
true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, B->getASTContext(), 
true);
+
+// Return false if any of the enable_if expressions of A and B are
+// different.
 if (Cand1ID != Cand2ID)
   return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == 
BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8977,25 +8977,25 @@
   auto Cand1Attrs = Cand1->specific_attrs();
   auto Cand2Attrs = Cand2->specific_attrs();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-Cand1ID.clear();
-Cand2ID.clear();
-
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
 // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-// has fewer enable_if attributes than Cand2.
-auto Cand1A = Cand1I++;
-if (Cand1A == Cand1Attrs.end())
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (!std::get<0>(Pair))
   return Comparison::Worse;
+if (!std::get<1>(Pair))
+  return Comparison::Better;
+
+Cand1ID.clear();
+Cand2ID.clear();
 
-Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, S.getASTContext(), true);
 if (Cand1ID != Cand2ID)
   return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate ,


Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2913,25 +2913,27 @@
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs();
   auto BEnableIfAttrs = B->specific_attrs();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-   ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair) || std::get<1>(Pair))
+  return false;
+
 Cand1ID.clear();
 Cand2ID.clear();
 
-AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+(*std::get<0>(Pair))->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+(*std::get<1>(Pair))->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+// Return false if any of the enable_if expressions 

[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:8979
+// has fewer enable_if attributes than Cand2, and vice versa.
+if (std::get<0>(Pair) == None)
   return Comparison::Worse;

I'd probably write this as "if (!std::get<0>(Pair))" - but I understand that 
opinions differ on such things easily enough.



Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+// Return false if the number of enable_if attributes is different.
+if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+  return false;

This might be more legible as "if (std::get<0>(Pair) || std::get<1>(Pair))", I 
think? (optionally using "hasValue", if preferred - but comparing the hasValues 
to each other, rather than testing if either is false, seems a bit opaque to me 
at least)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468



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

[PATCH] D55492: Implement Attr dumping in terms of visitors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/AttrVisitor.h:23-24
+
+template  struct make_ptr { using type = T *; };
+template  struct make_const_ptr { using type = const T *; };
+

aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > Oh, this is cute. We have StmtVisitor which does not use a namespace, but 
> > > defines these functions. Then we duplicate these function definitions in 
> > > each of the comment, decl, and attribute visitors under distinct 
> > > namespaces so we don't get ODR violations.
> > > 
> > > I think we should add `llvm::make_const_ptr` to STLExtras.h so we can use 
> > > the same definition from everywhere. We can use `std::add_pointer` in 
> > > place of `make_ptr`, because that is a one-to-one mapping, so we can drop 
> > > `make_ptr` entirely. However, I don't think we can use 
> > > `std::add_pointer` similarly because there's no way to 
> > > bind that to the template template parameter directly. I'm happy to make 
> > > these changes if you'd like.
> > Yes, I've thought about this duplication too. If you deduplicate, I'll 
> > rebase this change.  
> Sounds good, I'll try to get that in shortly.
r348729 (LLVM) and r348730 (Clang).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55492



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

r348730 - Move the make_const_ptr trait into STLExtras; use add_pointer where possible; NFC.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Sun Dec  9 11:53:24 2018
New Revision: 348730

URL: http://llvm.org/viewvc/llvm-project?rev=348730=rev
Log:
Move the make_const_ptr trait into STLExtras; use add_pointer where possible; 
NFC.

Modified:
cfe/trunk/include/clang/AST/CommentVisitor.h
cfe/trunk/include/clang/AST/DeclVisitor.h
cfe/trunk/include/clang/AST/EvaluatedExprVisitor.h
cfe/trunk/include/clang/AST/StmtVisitor.h

Modified: cfe/trunk/include/clang/AST/CommentVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CommentVisitor.h?rev=348730=348729=348730=diff
==
--- cfe/trunk/include/clang/AST/CommentVisitor.h (original)
+++ cfe/trunk/include/clang/AST/CommentVisitor.h Sun Dec  9 11:53:24 2018
@@ -11,14 +11,11 @@
 #define LLVM_CLANG_AST_COMMENTVISITOR_H
 
 #include "clang/AST/Comment.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/ErrorHandling.h"
 
 namespace clang {
 namespace comments {
-
-template  struct make_ptr { using type = T *; };
-template  struct make_const_ptr { using type = const T *; };
-
 template  class Ptr, typename ImplClass,
   typename RetTy = void, class... ParamTys>
 class CommentVisitorBase {
@@ -59,13 +56,13 @@ public:
 };
 
 template 
-class CommentVisitor
-: public CommentVisitorBase {};
+class CommentVisitor : public CommentVisitorBase {};
 
 template 
 class ConstCommentVisitor
-: public CommentVisitorBase 
{
-};
+: public CommentVisitorBase {};
 
 } // namespace comments
 } // namespace clang

Modified: cfe/trunk/include/clang/AST/DeclVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclVisitor.h?rev=348730=348729=348730=diff
==
--- cfe/trunk/include/clang/AST/DeclVisitor.h (original)
+++ cfe/trunk/include/clang/AST/DeclVisitor.h Sun Dec  9 11:53:24 2018
@@ -21,15 +21,12 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclOpenMP.h"
 #include "clang/AST/DeclTemplate.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/ErrorHandling.h"
 
 namespace clang {
 
 namespace declvisitor {
-
-template  struct make_ptr { using type = T *; };
-template  struct make_const_ptr { using type = const T *; };
-
 /// A simple visitor class that helps create declaration visitors.
 template class Ptr, typename ImplClass, typename 
RetTy=void>
 class Base {
@@ -66,16 +63,16 @@ public:
 ///
 /// This class does not preserve constness of Decl pointers (see also
 /// ConstDeclVisitor).
-template
+template 
 class DeclVisitor
- : public declvisitor::Base {};
+: public declvisitor::Base {};
 
 /// A simple visitor class that helps create declaration visitors.
 ///
 /// This class preserves constness of Decl pointers (see also DeclVisitor).
-template
+template 
 class ConstDeclVisitor
- : public declvisitor::Base {};
+: public declvisitor::Base {};
 
 } // namespace clang
 

Modified: cfe/trunk/include/clang/AST/EvaluatedExprVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/EvaluatedExprVisitor.h?rev=348730=348729=348730=diff
==
--- cfe/trunk/include/clang/AST/EvaluatedExprVisitor.h (original)
+++ cfe/trunk/include/clang/AST/EvaluatedExprVisitor.h Sun Dec  9 11:53:24 2018
@@ -19,6 +19,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/StmtVisitor.h"
+#include "llvm/ADT/STLExtras.h"
 
 namespace clang {
 
@@ -107,23 +108,22 @@ public:
 };
 
 /// EvaluatedExprVisitor - This class visits 'Expr *'s
-template
+template 
 class EvaluatedExprVisitor
- : public EvaluatedExprVisitorBase {
+: public EvaluatedExprVisitorBase {
 public:
-  explicit EvaluatedExprVisitor(const ASTContext ) :
-EvaluatedExprVisitorBase(Context) { }
+  explicit EvaluatedExprVisitor(const ASTContext )
+  : EvaluatedExprVisitorBase(Context) {}
 };
 
 /// ConstEvaluatedExprVisitor - This class visits 'const Expr *'s.
-template
+template 
 class ConstEvaluatedExprVisitor
- : public EvaluatedExprVisitorBase {
+: public EvaluatedExprVisitorBase {
 public:
-  explicit ConstEvaluatedExprVisitor(const ASTContext ) :
-EvaluatedExprVisitorBase(Context) { }
+  explicit ConstEvaluatedExprVisitor(const ASTContext )
+  : EvaluatedExprVisitorBase(Context) {}
 };
-
 }
 
 #endif // LLVM_CLANG_AST_EVALUATEDEXPRVISITOR_H

Modified: cfe/trunk/include/clang/AST/StmtVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/StmtVisitor.h?rev=348730=348729=348730=diff
==
--- cfe/trunk/include/clang/AST/StmtVisitor.h (original)
+++ cfe/trunk/include/clang/AST/StmtVisitor.h Sun Dec  9 11:53:24 2018
@@ -22,15 +22,12 @@
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/StmtOpenMP.h"
 #include "clang/Basic/LLVM.h"
+#include 

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D45898#1324625 , @orivej wrote:

> I have noticed that this change breaks seemingly valid code:
>
>   class A { protected: ~A(); };
>   struct B : A {};
>   B f() { return B(); }
>   B g() { return {}; }
>
>
> `f` compiles, but `g` fails with `temporary of type 'A' has protected 
> destructor`. (g++ 8.2 compiles this file.)


We talked about this in the analysis for https://reviews.llvm.org/D53860, and 
Richard decided that it is indeed invalid under the standard.

As a language designer I'm not sure that's a good language rule, but it's the 
realm of a standard defect, not a compiler bug.


Repository:
  rC Clang

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

https://reviews.llvm.org/D45898



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

[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.

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




Comment at: test/PCH/cxx-static_assert.cpp:17
 
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is 
not 2!"}}
 T<1> t1; // expected-note {{in instantiation of template class 'T<1>' 
requested here}}

Quuxplusone wrote:
> aaron.ballman wrote:
> > Quuxplusone wrote:
> > > courbet wrote:
> > > > aaron.ballman wrote:
> > > > > I'm not certain how I feel about now printing the failure condition 
> > > > > when there's an explicit message provided. From what I understand, a 
> > > > > fair amount of code in the wild does `static_assert(some_condition, 
> > > > > "some_condition")` because of older language modes where the message 
> > > > > was not optional. I worry we're going to start seeing a lot of 
> > > > > diagnostics like: `static_assert failed due to requirement '1 == 2' 
> > > > > "N == 2"`, which seems a bit low-quality. See 
> > > > > `DFAPacketizer::DFAPacketizer()` in LLVM as an example of something 
> > > > > similar.
> > > > > 
> > > > > Given that the user entered a message, do we still want to show the 
> > > > > requirement? Do we feel the same way if the requirement is fairly 
> > > > > large?
> > > > The issue is that `"N == 2"` is a useless error message. Actually, 
> > > > since the  error message has to be a string literal, there is no way 
> > > > for the user to print a debuggable output. So I really think we should 
> > > > print the failed condition.
> > > FWIW, I also don't agree with Aaron's concern.
> > > 
> > > I do think there is a lot of code in the wild whose string literal was 
> > > "phoned in." After all, this is why C++17 allows us to omit the string 
> > > literal: to avoid boilerplate like this.
> > > 
> > > static_assert(some-condition, "some-condition");
> > > static_assert(some-condition, "some-condition was not satisfied");
> > > static_assert(some-condition, "some-condition must be satisfied");
> > > static_assert(some-condition, "");
> > > 
> > > But should Clang go _out of its way_ to suppress such "redundant" string 
> > > literals? First of all, such suppression would be C++14-and-earlier: if a 
> > > C++17-native program contains a string literal, we should maybe assume 
> > > it's on purpose. Second, it's not clear how a machine could detect 
> > > "redundant" literals with high fidelity.
> > > 
> > > static_assert(std::is_integral, "std::is_integral");
> > > // clearly redundant
> > > static_assert(std::is_integral, "T must be integral");
> > > // redundant, but unlikely to be machine-detectable as such
> > > 
> > > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 * 
> > > sizeof(DFAInput)),
> > > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 * 
> > > sizeof(DFAInput))");
> > > // redundant, but unlikely to be machine-detectable as such
> > > // thanks to the substitution of > for <=
> > > 
> > > static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 * 
> > > sizeof(DFAInput)),
> > > "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
> > > // redundant, but unlikely to be machine-detectable as such
> > > 
> > > In any event, I agree with @courbet that Clang should print the expansion 
> > > of the failed condition in any case.
> > > 
> > > Also: One might argue that if the `static_assert` fits completely on one 
> > > source line, then we could omit the string-literal from our error message 
> > > because the string literal will be shown anyway as part of the offending 
> > > source line — but I believe IDE-users would complain about that, so, we 
> > > shouldn't do that. And even then, we'd still want to print the failed 
> > > condition!
> > > 
> > > Checking my understanding: The `static_assert` above (taken from 
> > > `DFAPacketizer::DFAPacketizer()` in LLVM) would be unchanged by 
> > > @courbet's patches, because none of its subexpressions are 
> > > template-dependent. Right?
> > > But should Clang go _out of its way_ to suppress such "redundant" string 
> > > literals? 
> > 
> > I wasn't suggesting it should; I was suggesting that Clang should be 
> > conservative and suppress printing the conditional when a message is 
> > present, not when they look to be redundant enough.
> > 
> > > if a C++17-native program contains a string literal, we should maybe 
> > > assume it's on purpose. 
> > 
> > This is the exact scenario I was envisioning.
> > 
> > It's a relatively weak preference, but I kind of prefer not displaying the 
> > conditional information in the presence of a message (at least in C++17 and 
> > above), especially as the conditional can be huge. I'm thinking of 
> > scenarios where the user does something like:
> > ```
> > static_assert(condition1 && condition2 && (condition3 || 

[PATCH] D55492: Implement Attr dumping in terms of visitors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/AttrVisitor.h:21
+
+namespace attrvisitor {
+

steveire wrote:
> aaron.ballman wrote:
> > I don't think the namespace adds value.
> I think the point is to separate the implementation detail. I don't care 
> either way, but the other visitors should be changed first and this one can 
> follow the same pattern. 
Yeah, let's leave this in place and deal with it later.



Comment at: include/clang/AST/AttrVisitor.h:23-24
+
+template  struct make_ptr { using type = T *; };
+template  struct make_const_ptr { using type = const T *; };
+

steveire wrote:
> aaron.ballman wrote:
> > Oh, this is cute. We have StmtVisitor which does not use a namespace, but 
> > defines these functions. Then we duplicate these function definitions in 
> > each of the comment, decl, and attribute visitors under distinct namespaces 
> > so we don't get ODR violations.
> > 
> > I think we should add `llvm::make_const_ptr` to STLExtras.h so we can use 
> > the same definition from everywhere. We can use `std::add_pointer` in place 
> > of `make_ptr`, because that is a one-to-one mapping, so we can drop 
> > `make_ptr` entirely. However, I don't think we can use 
> > `std::add_pointer` similarly because there's no way to bind 
> > that to the template template parameter directly. I'm happy to make these 
> > changes if you'd like.
> Yes, I've thought about this duplication too. If you deduplicate, I'll rebase 
> this change.  
Sounds good, I'll try to get that in shortly.



Comment at: include/clang/AST/AttrVisitor.h:27
+/// A simple visitor class that helps create attribute visitors.
+template  class Ptr, typename ImplClass,
+  typename RetTy = void, class... ParamTys>

steveire wrote:
> aaron.ballman wrote:
> > s/class/typename
> I think c++17 is the first to allow typename here. 
Huh, how neat. I'm fine leaving it as `class` if there are concerns about this 
being accepted by all our base compilers, it was a minor nit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55492



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

[PATCH] D55492: Implement Attr dumping in terms of visitors

[Stephen Kelly via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/AttrVisitor.h:21
+
+namespace attrvisitor {
+

aaron.ballman wrote:
> I don't think the namespace adds value.
I think the point is to separate the implementation detail. I don't care either 
way, but the other visitors should be changed first and this one can follow the 
same pattern. 



Comment at: include/clang/AST/AttrVisitor.h:23-24
+
+template  struct make_ptr { using type = T *; };
+template  struct make_const_ptr { using type = const T *; };
+

aaron.ballman wrote:
> Oh, this is cute. We have StmtVisitor which does not use a namespace, but 
> defines these functions. Then we duplicate these function definitions in each 
> of the comment, decl, and attribute visitors under distinct namespaces so we 
> don't get ODR violations.
> 
> I think we should add `llvm::make_const_ptr` to STLExtras.h so we can use the 
> same definition from everywhere. We can use `std::add_pointer` in place of 
> `make_ptr`, because that is a one-to-one mapping, so we can drop `make_ptr` 
> entirely. However, I don't think we can use 
> `std::add_pointer` similarly because there's no way to bind 
> that to the template template parameter directly. I'm happy to make these 
> changes if you'd like.
Yes, I've thought about this duplication too. If you deduplicate, I'll rebase 
this change.  



Comment at: include/clang/AST/AttrVisitor.h:27
+/// A simple visitor class that helps create attribute visitors.
+template  class Ptr, typename ImplClass,
+  typename RetTy = void, class... ParamTys>

aaron.ballman wrote:
> s/class/typename
I think c++17 is the first to allow typename here. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D55492



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

[PATCH] D55492: Implement Attr dumping in terms of visitors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/AttrVisitor.h:21
+
+namespace attrvisitor {
+

I don't think the namespace adds value.



Comment at: include/clang/AST/AttrVisitor.h:23-24
+
+template  struct make_ptr { using type = T *; };
+template  struct make_const_ptr { using type = const T *; };
+

Oh, this is cute. We have StmtVisitor which does not use a namespace, but 
defines these functions. Then we duplicate these function definitions in each 
of the comment, decl, and attribute visitors under distinct namespaces so we 
don't get ODR violations.

I think we should add `llvm::make_const_ptr` to STLExtras.h so we can use the 
same definition from everywhere. We can use `std::add_pointer` in place of 
`make_ptr`, because that is a one-to-one mapping, so we can drop `make_ptr` 
entirely. However, I don't think we can use `std::add_pointer` 
similarly because there's no way to bind that to the template template 
parameter directly. I'm happy to make these changes if you'd like.



Comment at: include/clang/AST/AttrVisitor.h:27
+/// A simple visitor class that helps create attribute visitors.
+template  class Ptr, typename ImplClass,
+  typename RetTy = void, class... ParamTys>

s/class/typename



Comment at: include/clang/AST/TextNodeDumper.h:199
 
+#include "clang/AST/AttrTextNodeDump.inc"
+

I'd appreciate some comments gently reminding the reader that this include 
introduces declarations which are expected to be members of a class.



Comment at: lib/AST/ASTDumper.cpp:343
+
+#include "clang/AST/AttrNodeTraverse.inc"
   };

Similarly, add some comments about how this adds implementations to the class.



Comment at: utils/TableGen/ClangAttrEmitter.cpp:3714
+
+std::string functionContent;
+llvm::raw_string_ostream SS(functionContent);

`FunctionContent`



Comment at: utils/TableGen/ClangAttrEmitter.cpp:3726
+functionContent = SS.str();
+if (SS.tell()) {
+  OS << "  void Visit" << R.getName() << "Attr(const " << R.getName()

Wouldn't this be `!FunctionContent.empty()`?



Comment at: utils/TableGen/ClangAttrEmitter.cpp:3747
 
-  for (const auto *AI : Args)
-createArgument(*AI, R.getName())->writeDumpChildren(OS);
+std::string functionContent;
+llvm::raw_string_ostream SS(functionContent);

`FunctionContent`


Repository:
  rC Clang

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

https://reviews.llvm.org/D55492



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

[PATCH] D55475: Misc typos fixes in ./lib folder

[luzpaz via Phabricator via cfe-commits]

luzpaz added a comment.

Please commit as I do not have access/privs.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55475



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

[PATCH] D55495: Change InitListExpr dump to label and pointer

[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 a small nit.




Comment at: lib/AST/ASTDumper.cpp:1956-1959
+  OS << " array_filler";
+  NodeDumper.dumpPointer(Filler);
+
   dumpStmt(Filler);

Indented too far now.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55495



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

[PATCH] D55490: Add dumpMethodDeclOverrides to NodeDumper

[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 a minor nit.




Comment at: lib/AST/ASTDumper.cpp:921
 
   if (const CXXMethodDecl *MD = dyn_cast(D)) {
+dumpMethodDeclOverrides(MD);

Elide braces, and since you're touching the line, might as well do `const auto 
*`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55490



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

[PATCH] D55489: Implement dumpFunctionDeclParameters in NodeDumper

[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/D55489/new/

https://reviews.llvm.org/D55489



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

[PATCH] D55393: Re-order content of template parameter dumps

[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:106
+ const Decl *From = nullptr,
+ const char *label = nullptr);
 void dumpTemplateArgumentList(const TemplateArgumentList );

s/label/Label per naming conventions (same below).



Comment at: lib/AST/ASTDumper.cpp:733
 
+if (From) {
+  dumpDeclRef(From, label);

Elide braces.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55393



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

[PATCH] D54109: [clang-query] continue querying even if files are skipped

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn abandoned this revision.
Lekensteyn added a comment.

This was addressed by https://reviews.llvm.org/D51183 (without new tests 
though).


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

https://reviews.llvm.org/D54109



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

Re: r348713 - Fix InitListExpr test

[Aaron Ballman via cfe-commits]

On Sun, Dec 9, 2018 at 8:16 AM Stephen Kelly via cfe-commits
 wrote:
>
> Author: steveire
> Date: Sun Dec  9 05:13:41 2018
> New Revision: 348713
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348713=rev
> Log:
> Fix InitListExpr test
>
> Wrong case of Check meant this has no effect.

Good catch! Thank you for fixing that up.

~Aaron

>
> Modified:
> cfe/trunk/test/AST/ast-dump-stmt.cpp
>
> Modified: cfe/trunk/test/AST/ast-dump-stmt.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-stmt.cpp?rev=348713=348712=348713=diff
> ==
> --- cfe/trunk/test/AST/ast-dump-stmt.cpp (original)
> +++ cfe/trunk/test/AST/ast-dump-stmt.cpp Sun Dec  9 05:13:41 2018
> @@ -89,12 +89,12 @@ union U {
>  void TestUnionInitList()
>  {
>U us[3] = {1};
> -// Check: VarDecl {{.+}}  col:5 us 'U [3]' cinit
> -// Check-NEXT: `-InitListExpr {{.+}}  'U [3]'
> -// Check-NEXT:   |-array filler
> -// Check-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
> 'int'
> -// Check-NEXT:   |-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
> 'int'
> -// Check-NEXT:   | `-IntegerLiteral {{.+}}  'int' 1
> +// CHECK: VarDecl {{.+}}  col:5 us 'U [3]' cinit
> +// CHECK-NEXT: `-InitListExpr {{.+}}  'U [3]'
> +// CHECK-NEXT:   |-array filler
> +// CHECK-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
> 'int'
> +// CHECK-NEXT:   `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
> 'int'
> +// CHECK-NEXT: `-IntegerLiteral {{.+}}  'int' 1
>  }
>
>  void TestSwitch(int i) {
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM with a small nit. @ABataev, are you okay with this approach?




Comment at: lib/AST/ASTDumper.cpp:1060
+  dumpStmt(D->getCombiner());
+  if (auto *Initializer = D->getInitializer()) {
 dumpStmt(Initializer);

Elide braces and may as well make this `const auto *`. (If you want to hit the 
one above, that would also be good -- maybe even hoist the call into an 
initializer for a local variable to be used in both places?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Thank you for the investigation! I'll try to take a look.


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



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

[PATCH] D55387: [analyzer] MoveChecker Pt.7: NFC: Misc refactoring.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Artem,
The overall idea is good but I have some remarks inline.




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:265
 
+void MoveChecker::checkUse(ProgramStateRef State, const MemRegion *Region,
+   const CXXRecordDecl *RD, MisuseKind MK,

I think that if the function is named "checkUse()", committing State changes is 
not what is really expected from it. Should we rename it or change the logic 
somehow? For example, return true if a report was emitted and add transition in 
the caller?



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:272
+  if (!RS || isAnyBaseRegionReported(State, Region)
+  || isInMoveSafeContext(C.getLocationContext())) {
+// Finalize changes made by the caller.

This formatting is different from what clang-format does.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:342
 
+  const CXXRecordDecl *RD = MethodDecl->getParent();
+

Should we move RD closer to its first use?



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:490
+  if (CtorDec->isMoveConstructor())
+checkUse(State, ArgRegion, RD, MK_Move, C);
+  else

```MisuseKind MK = CtorDecl->isMoveConstructor() ? MK_Move : MK_Copy;
checkUse(State, ArgRegion, RD, MK, C);```
?


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

https://reviews.llvm.org/D55387



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

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 177428.
steveire added a comment.

Use pointer approach


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395

Files:
  lib/AST/ASTDumper.cpp
  test/AST/dump.cpp


Index: test/AST/dump.cpp
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -13,14 +13,14 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) 
initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue 
'*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_out' 'char'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
@@ -28,7 +28,7 @@
 // CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner 0x{{.+}} initializer 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1040,7 +1040,7 @@
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
+  NodeDumper.dumpPointer(D->getCombiner());
   if (auto *Initializer = D->getInitializer()) {
 OS << " initializer";
 switch (D->getInitializerKind()) {
@@ -1053,6 +1053,11 @@
 case OMPDeclareReductionDecl::CallInit:
   break;
 }
+NodeDumper.dumpPointer(Initializer);
+  }
+
+  dumpStmt(D->getCombiner());
+  if (auto *Initializer = D->getInitializer()) {
 dumpStmt(Initializer);
   }
 }


Index: test/AST/dump.cpp
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -13,14 +13,14 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 operator+ 'int' combiner
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 operator+ 'int' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 'char' combiner
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 'char' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 'omp_out' 'char'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
@@ -28,7 +28,7 @@
 // CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 fun 'float' combiner initializer
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 fun 'float' combiner 0x{{.+}} initializer 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

I would prefer *not* to do what is in this patch. I'm just trying to find a way 
forward. I like the approach of using the existing precedent of printing the 
pointers as a compromise between removing such labels entirely and not having 
things like that at all. I did that for `InitListExpr` here: 
https://reviews.llvm.org/D55495


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



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

[PATCH] D55495: Change InitListExpr dump to label and pointer

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

My real preference is just to remove the `array filler` child node entirely and 
not add a pointer here. However, I'm just looking for something that has a 
chance of passing review.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55495



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

[PATCH] D55495: Change InitListExpr dump to label and pointer

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.

Don't add a child just for the label.


Repository:
  rC Clang

https://reviews.llvm.org/D55495

Files:
  lib/AST/ASTDumper.cpp
  test/AST/ast-dump-stmt.cpp


Index: test/AST/ast-dump-stmt.cpp
===
--- test/AST/ast-dump-stmt.cpp
+++ test/AST/ast-dump-stmt.cpp
@@ -90,9 +90,8 @@
 {
   U us[3] = {1};
 // CHECK: VarDecl {{.+}}  col:5 us 'U [3]' cinit
-// CHECK-NEXT: `-InitListExpr {{.+}}  'U [3]'
-// CHECK-NEXT:   |-array filler
-// CHECK-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
+// CHECK-NEXT: `-InitListExpr {{.+}}  'U [3]' array_filler 
0x{{.+}}
+// CHECK-NEXT:   |-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
 // CHECK-NEXT:   `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
 // CHECK-NEXT: `-IntegerLiteral {{.+}}  'int' 1
 }
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1951,11 +1951,12 @@
 OS << " field ";
 NodeDumper.dumpBareDeclRef(Field);
   }
+
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpChild([=] {
-  OS << "array filler";
+  OS << " array_filler";
+  NodeDumper.dumpPointer(Filler);
+
   dumpStmt(Filler);
-});
   }
 }
 


Index: test/AST/ast-dump-stmt.cpp
===
--- test/AST/ast-dump-stmt.cpp
+++ test/AST/ast-dump-stmt.cpp
@@ -90,9 +90,8 @@
 {
   U us[3] = {1};
 // CHECK: VarDecl {{.+}}  col:5 us 'U [3]' cinit
-// CHECK-NEXT: `-InitListExpr {{.+}}  'U [3]'
-// CHECK-NEXT:   |-array filler
-// CHECK-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 'int'
+// CHECK-NEXT: `-InitListExpr {{.+}}  'U [3]' array_filler 0x{{.+}}
+// CHECK-NEXT:   |-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 'int'
 // CHECK-NEXT:   `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 'int'
 // CHECK-NEXT: `-IntegerLiteral {{.+}}  'int' 1
 }
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1951,11 +1951,12 @@
 OS << " field ";
 NodeDumper.dumpBareDeclRef(Field);
   }
+
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpChild([=] {
-  OS << "array filler";
+  OS << " array_filler";
+  NodeDumper.dumpPointer(Filler);
+
   dumpStmt(Filler);
-});
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D55395#1324699 , @aaron.ballman 
wrote:

> This is a novel approach that's not used anywhere else in the AST dumper and 
> there are several ways we could handle this, including:
>
> - What's proposed (adding a new node to the tree that's not directly an AST 
> node)
> - Making use of the pointer information. e.g., https://pastebin.com/mh9dHT9L
> - Adding the label before the AST node. e.g., https://pastebin.com/L8YwJTqe
> - Adding the label after the AST node. e.g., https://pastebin.com/gbNahjsd
> - Probably others
>
>   Why this way?
>
>   I'm not a huge fan of adding a new node to the tree that's not an AST node. 
> It expands the tree both vertically (by adding a new node) and horizontally 
> (by indenting everything below that new node) which I find visually 
> distracting for the benefit provided. I personally prefer using the pointer 
> information as it's less structurally disruptive and still provides the same 
> information. I also find it a bit easier to match nodes up that way because 
> the indentation level and tree-like adornments sometimes make it hard for me 
> to determine relationships between when spatially far apart in the tree. 
> There is precedence for using labels + pointers in the AST dumper already -- 
> this is how we handle the prev and parent nodes for declarations, for 
> instance.
>
>   If we're going to invent a novel way to do this, I do not think it should 
> be done in an ad hoc manner, but should instead talk about whether we want to 
> see this done in a more uniform fashion. For instance, how is this 
> information any different than the list of overrides for a method, the 
> previous declaration in a redecl, the parent of an out-of-line function 
> definition, where a default template argument is inherited from, etc (all of 
> which use pointers for relationships)? I don't feel the same way if we go 
> with an existing practice that incrementally improves consistency.


I personally also prefer the first one. However is this really needed in this 
case ? The first sub-node will always be the combiner,
and then the second node will the the initializer if present. It seems to me 
that there is no ambiguity here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



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

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D55395#1324699 , @aaron.ballman 
wrote:

> This is a novel approach that's not used anywhere else in the AST dumper


Actually it's used by the InitListExpr dump. See https://reviews.llvm.org/D55488


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



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

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is a novel approach that's not used anywhere else in the AST dumper and 
there are several ways we could handle this, including:

- What's proposed (adding a new node to the tree that's not directly an AST 
node)
- Making use of the pointer information. e.g., https://pastebin.com/mh9dHT9L
- Adding the label before the AST node. e.g., https://pastebin.com/L8YwJTqe
- Adding the label after the AST node. e.g., https://pastebin.com/gbNahjsd
- Probably others

Why this way?

I'm not a huge fan of adding a new node to the tree that's not an AST node. It 
expands the tree both vertically (by adding a new node) and horizontally (by 
indenting everything below that new node) which I find visually distracting for 
the benefit provided. I personally prefer using the pointer information as it's 
less structurally disruptive and still provides the same information. I also 
find it a bit easier to match nodes up that way because the indentation level 
and tree-like adornments sometimes make it hard for me to determine 
relationships between when spatially far apart in the tree. There is precedence 
for using labels + pointers in the AST dumper already -- this is how we handle 
the prev and parent nodes for declarations, for instance.

If we're going to invent a novel way to do this, I do not think it should be 
done in an ad hoc manner, but should instead talk about whether we want to see 
this done in a more uniform fashion. For instance, how is this information any 
different than the list of overrides for a method, the previous declaration in 
a redecl, the parent of an out-of-line function definition, where a default 
template argument is inherited from, etc (all of which use pointers for 
relationships)? I don't feel the same way if we go with an existing practice 
that incrementally improves consistency.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



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

[PATCH] D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In D55388#1322601 , @xazax.hun wrote:

> Hm. I wonder if it would also make sense to model e.g. the get method to 
> return nullptr for moved from smart ptrs. This could help null dereference 
> checker and also aid false path prunning.


Great idea!

IIRC, we haven't model smart-pointer yet. There are no warnings fired in the 
simple code below.

  int foo() {
  auto ptr = std::make_unique(0);
  ptr = nullptr;
  return *ptr;  // <- Should emit a warning here
  }

If we want to model the `get()` method, not only the move action but also the 
assignment should be considered. `std::unique_ptr` may be fine, but is the 
reference count stuff too complex for `std::shared_ptr` to get the information 
available for analysis? Can the `lifetime` analysis cover the smart pointers?

  int unique_ptr_bad() {
  auto ptr = std::make_unique(0);
  ptr = nullptr;
  int *raw_p = ptr.get();   // <- 'raw_p' is definitely null here
  return *raw_p;  // <- Should emit a warning here
  }

Related dev-mail about smart pointer checkers, see 
http://lists.llvm.org/pipermail/cfe-dev/2012-January/019446.html


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

https://reviews.llvm.org/D55388



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

[PATCH] D55491: Implement TemplateArgument dumping in terms of Visitor

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 177421.
steveire added a comment.

Update


Repository:
  rC Clang

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

https://reviews.llvm.org/D55491

Files:
  include/clang/AST/TemplateArgumentVisitor.h
  include/clang/AST/TextNodeDumper.h
  lib/AST/ASTDumper.cpp
  lib/AST/TextNodeDumper.cpp

Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -161,6 +161,19 @@
   OS << " " << T.split().Quals.getAsString();
 }
 
+void TextNodeDumper::Visit(const TemplateArgument , SourceRange R,
+   const Decl *From, const char *label) {
+  OS << "TemplateArgument";
+  if (R.isValid())
+dumpSourceRange(R);
+
+  if (From) {
+dumpDeclRef(From, label);
+  }
+
+  ConstTemplateArgumentVisitor::Visit(TA);
+}
+
 void TextNodeDumper::dumpPointer(const void *Ptr) {
   ColorScope Color(OS, ShowColors, AddressColor);
   OS << ' ' << Ptr;
@@ -1022,3 +1035,46 @@
   if (auto N = T->getNumExpansions())
 OS << " expansions " << *N;
 }
+
+void TextNodeDumper::VisitNullTemplateArgument(const TemplateArgument ) {
+  OS << " null";
+}
+
+void TextNodeDumper::VisitTypeTemplateArgument(const TemplateArgument ) {
+  OS << " type";
+  dumpType(TA.getAsType());
+}
+
+void TextNodeDumper::VisitDeclarationTemplateArgument(
+const TemplateArgument ) {
+  OS << " decl";
+  dumpDeclRef(TA.getAsDecl());
+}
+
+void TextNodeDumper::VisitNullPtrTemplateArgument(const TemplateArgument ) {
+  OS << " nullptr";
+}
+
+void TextNodeDumper::VisitIntegralTemplateArgument(const TemplateArgument ) {
+  OS << " integral " << TA.getAsIntegral();
+}
+
+void TextNodeDumper::VisitTemplateTemplateArgument(const TemplateArgument ) {
+  OS << " template ";
+  TA.getAsTemplate().dump(OS);
+}
+
+void TextNodeDumper::VisitTemplateExpansionTemplateArgument(
+const TemplateArgument ) {
+  OS << " template expansion ";
+  TA.getAsTemplateOrTemplatePattern().dump(OS);
+}
+
+void TextNodeDumper::VisitExpressionTemplateArgument(
+const TemplateArgument ) {
+  OS << " expr";
+}
+
+void TextNodeDumper::VisitPackTemplateArgument(const TemplateArgument ) {
+  OS << " pack";
+}
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -23,6 +23,7 @@
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/LocInfoType.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/AST/TemplateArgumentVisitor.h"
 #include "clang/AST/TextNodeDumper.h"
 #include "clang/AST/TypeVisitor.h"
 #include "clang/Basic/Builtins.h"
@@ -42,7 +43,8 @@
   : public ConstDeclVisitor,
 public ConstStmtVisitor,
 public ConstCommentVisitor,
-public TypeVisitor {
+public TypeVisitor,
+public ConstTemplateArgumentVisitor {
 
 TextTreeStructure TreeStructure;
 TextNodeDumper NodeDumper;
@@ -328,6 +330,16 @@
 
 // Comments.
 void dumpComment(const Comment *C, const FullComment *FC);
+
+void VisitExpressionTemplateArgument(const TemplateArgument ) {
+  dumpStmt(TA.getAsExpr());
+}
+void VisitPackTemplateArgument(const TemplateArgument ) {
+  for (TemplateArgument::pack_iterator I = TA.pack_begin(),
+   E = TA.pack_end();
+   I != E; ++I)
+dumpTemplateArgument(*I);
+}
   };
 }
 
@@ -534,51 +546,8 @@
 void ASTDumper::dumpTemplateArgument(const TemplateArgument , SourceRange R,
  const Decl *From, const char *label) {
   dumpChild([=] {
-OS << "TemplateArgument";
-if (R.isValid())
-  NodeDumper.dumpSourceRange(R);
-
-if (From) {
-  NodeDumper.dumpDeclRef(From, label);
-}
-
-switch (A.getKind()) {
-case TemplateArgument::Null:
-  OS << " null";
-  break;
-case TemplateArgument::Type:
-  OS << " type";
-  NodeDumper.dumpType(A.getAsType());
-  break;
-case TemplateArgument::Declaration:
-  OS << " decl";
-  NodeDumper.dumpDeclRef(A.getAsDecl());
-  break;
-case TemplateArgument::NullPtr:
-  OS << " nullptr";
-  break;
-case TemplateArgument::Integral:
-  OS << " integral " << A.getAsIntegral();
-  break;
-case TemplateArgument::Template:
-  OS << " template ";
-  A.getAsTemplate().dump(OS);
-  break;
-case TemplateArgument::TemplateExpansion:
-  OS << " template expansion ";
-  A.getAsTemplateOrTemplatePattern().dump(OS);
-  break;
-case TemplateArgument::Expression:
-  OS << " expr";
-  dumpStmt(A.getAsExpr());
-  break;
-case TemplateArgument::Pack:
-  OS << " pack";
-  for (TemplateArgument::pack_iterator I = A.pack_begin(), E = A.pack_end();
-   I != E; ++I)
-dumpTemplateArgument(*I);
-  break;
-}
+NodeDumper.Visit(A, R, From, label);
+

[PATCH] D55475: Misc typos fixes in ./lib folder

[Raphael Isemann via Phabricator via cfe-commits]

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

This looks all good to me and from what see this was requested in a previous 
review (D44188 ). Do you need someone to 
commit this or did you receive commit access?




Comment at: lib/Lex/PPDirectives.cpp:121
 // vends a module map, one might want to avoid hitting intermediate build
-// products containig the the module map or avoid finding the system installed
+// products containimg the the module map or avoid finding the system installed
 // modulemap for that library.

Containing 


Repository:
  rC Clang

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

https://reviews.llvm.org/D55475



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

[PATCH] D55488: Add utility for dumping a label with child nodes

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 177419.
steveire added a comment.

Use std::string


Repository:
  rC Clang

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

https://reviews.llvm.org/D55488

Files:
  include/clang/AST/ASTDumperUtils.h
  lib/AST/ASTDumper.cpp


Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -62,6 +62,10 @@
 template void dumpChild(Fn doDumpChild) {
   TreeStructure.addChild(doDumpChild);
 }
+template 
+void dumpChild(const std::string , Fn doDumpChild) {
+  TreeStructure.addChild(label, doDumpChild);
+}
 
   public:
 ASTDumper(raw_ostream , const CommandTraits *Traits,
@@ -82,7 +86,7 @@
 void setDeserialize(bool D) { Deserialize = D; }
 
 void dumpDecl(const Decl *D);
-void dumpStmt(const Stmt *S);
+void dumpStmt(const Stmt *S, const std::string  = {});
 
 // Utilities
 void dumpType(QualType T) { NodeDumper.dumpType(T); }
@@ -1685,8 +1689,8 @@
 //  Stmt dumping methods.
 
//===--===//
 
-void ASTDumper::dumpStmt(const Stmt *S) {
-  dumpChild([=] {
+void ASTDumper::dumpStmt(const Stmt *S, const std::string ) {
+  dumpChild(label, [=] {
 if (!S) {
   ColorScope Color(OS, ShowColors, NullColor);
   OS << "<<>>";
@@ -1952,10 +1956,7 @@
 NodeDumper.dumpBareDeclRef(Field);
   }
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpChild([=] {
-  OS << "array filler";
-  dumpStmt(Filler);
-});
+dumpStmt(Filler, "array filler");
   }
 }
 
Index: include/clang/AST/ASTDumperUtils.h
===
--- include/clang/AST/ASTDumperUtils.h
+++ include/clang/AST/ASTDumperUtils.h
@@ -109,6 +109,17 @@
   std::string Prefix;
 
 public:
+  /// Add a child of the current node with a label.
+  /// Calls doAddChild without arguments
+  template 
+  void addChild(const std::string , Fn doAddChild) {
+if (label.empty())
+  return addChild(doAddChild);
+addChild([=] {
+  OS << label;
+  addChild(doAddChild);
+});
+  }
   /// Add a child of the current node.  Calls doAddChild without arguments
   template  void addChild(Fn doAddChild) {
 // If we're at the top level, there's nothing interesting to do; just


Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -62,6 +62,10 @@
 template void dumpChild(Fn doDumpChild) {
   TreeStructure.addChild(doDumpChild);
 }
+template 
+void dumpChild(const std::string , Fn doDumpChild) {
+  TreeStructure.addChild(label, doDumpChild);
+}
 
   public:
 ASTDumper(raw_ostream , const CommandTraits *Traits,
@@ -82,7 +86,7 @@
 void setDeserialize(bool D) { Deserialize = D; }
 
 void dumpDecl(const Decl *D);
-void dumpStmt(const Stmt *S);
+void dumpStmt(const Stmt *S, const std::string  = {});
 
 // Utilities
 void dumpType(QualType T) { NodeDumper.dumpType(T); }
@@ -1685,8 +1689,8 @@
 //  Stmt dumping methods.
 //===--===//
 
-void ASTDumper::dumpStmt(const Stmt *S) {
-  dumpChild([=] {
+void ASTDumper::dumpStmt(const Stmt *S, const std::string ) {
+  dumpChild(label, [=] {
 if (!S) {
   ColorScope Color(OS, ShowColors, NullColor);
   OS << "<<>>";
@@ -1952,10 +1956,7 @@
 NodeDumper.dumpBareDeclRef(Field);
   }
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpChild([=] {
-  OS << "array filler";
-  dumpStmt(Filler);
-});
+dumpStmt(Filler, "array filler");
   }
 }
 
Index: include/clang/AST/ASTDumperUtils.h
===
--- include/clang/AST/ASTDumperUtils.h
+++ include/clang/AST/ASTDumperUtils.h
@@ -109,6 +109,17 @@
   std::string Prefix;
 
 public:
+  /// Add a child of the current node with a label.
+  /// Calls doAddChild without arguments
+  template 
+  void addChild(const std::string , Fn doAddChild) {
+if (label.empty())
+  return addChild(doAddChild);
+addChild([=] {
+  OS << label;
+  addChild(doAddChild);
+});
+  }
   /// Add a child of the current node.  Calls doAddChild without arguments
   template  void addChild(Fn doAddChild) {
 // If we're at the top level, there's nothing interesting to do; just
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 177420.
steveire added a comment.

Use child node labels


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395

Files:
  lib/AST/ASTDumper.cpp
  test/AST/dump.cpp


Index: test/AST/dump.cpp
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -13,33 +13,37 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) 
initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue 
'*=' ComputeLHSTy='int' ComputeResultTy='int'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_in' 'int'
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int'
+// CHECK-NEXT: | |-combiner
+// CHECK-NEXT: | | `-CompoundAssignOperator {{.+}}  'int' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
+// CHECK-NEXT: | |   |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
+// CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'int' 
+// CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_out' 'char'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
-// CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'char' 
-// CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char'
+// CHECK-NEXT: | |-combiner
+// CHECK-NEXT: | `-CompoundAssignOperator {{.+}}  'char' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
+// CHECK-NEXT: |   |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_out' 'char'
+// CHECK-NEXT: |   `-ImplicitCastExpr {{.+}}  'int' 
+// CHECK-NEXT: | `-ImplicitCastExpr {{.+}}  'char' 
+// CHECK-NEXT: |   `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
-// CHECK-NEXT: | |-BinaryOperator {{.+}}  'float' lvalue '='
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_priv' 'float'
-// CHECK-NEXT: | | `-BinaryOperator {{.+}}  'float' '+'
-// CHECK-NEXT: | |   |-ImplicitCastExpr {{.+}}  'float' 

-// CHECK-NEXT: | |   | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_orig' 'float'
-// CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'float' 

-// CHECK-NEXT: | | `-IntegerLiteral {{.+}}  'int' 15
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float'
+// CHECK-NEXT: | |-combiner
+// CHECK-NEXT: | `-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
+// CHECK-NEXT: |   |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
+// CHECK-NEXT: |   `-ImplicitCastExpr {{.+}}  'float' 
+// CHECK-NEXT: | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
+// CHECK-NEXT: | |-initializer
+// CHECK-NEXT: | `-BinaryOperator {{.+}}  'float' lvalue '='
+// CHECK-NEXT: |   |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_priv' 'float'
+// CHECK-NEXT: |   `-BinaryOperator {{.+}}  'float' '+'
+// CHECK-NEXT: | |-ImplicitCastExpr {{.+}}  'float' 

+// CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_orig' 'float'
+// CHECK-NEXT: | `-ImplicitCastExpr {{.+}}  'float' 

+// CHECK-NEXT: |   `-IntegerLiteral {{.+}}  'int' 15
 
 struct S {
   int a, b;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1043,21 +1043,21 @@
 void ASTDumper::VisitOMPDeclareReductionDecl(const OMPDeclareReductionDecl *D) 
{
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
-  OS << " combiner";
-  dumpStmt(D->getCombiner());
+
+  dumpStmt(D->getCombiner(), "combiner");
   if (auto *Initializer 

[PATCH] D55492: Implement Attr dumping in terms of visitors

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, mgorny.

Repository:
  rC Clang

https://reviews.llvm.org/D55492

Files:
  include/clang/AST/AttrVisitor.h
  include/clang/AST/CMakeLists.txt
  include/clang/AST/TextNodeDumper.h
  lib/AST/ASTDumper.cpp
  lib/AST/TextNodeDumper.cpp
  utils/TableGen/ClangAttrEmitter.cpp
  utils/TableGen/TableGen.cpp
  utils/TableGen/TableGenBackends.h

Index: utils/TableGen/TableGenBackends.h
===
--- utils/TableGen/TableGenBackends.h
+++ utils/TableGen/TableGenBackends.h
@@ -45,7 +45,10 @@
 void EmitClangAttrParsedAttrList(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrParsedAttrImpl(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitClangAttrParsedAttrKinds(llvm::RecordKeeper , llvm::raw_ostream );
-void EmitClangAttrDump(llvm::RecordKeeper , llvm::raw_ostream );
+void EmitClangAttrTextNodeDump(llvm::RecordKeeper ,
+   llvm::raw_ostream );
+void EmitClangAttrNodeTraverse(llvm::RecordKeeper ,
+   llvm::raw_ostream );
 
 void EmitClangDiagsDefs(llvm::RecordKeeper , llvm::raw_ostream ,
 const std::string );
Index: utils/TableGen/TableGen.cpp
===
--- utils/TableGen/TableGen.cpp
+++ utils/TableGen/TableGen.cpp
@@ -40,7 +40,8 @@
   GenClangAttrParsedAttrList,
   GenClangAttrParsedAttrImpl,
   GenClangAttrParsedAttrKinds,
-  GenClangAttrDump,
+  GenClangAttrTextNodeDump,
+  GenClangAttrNodeTraverse,
   GenClangDiagsDefs,
   GenClangDiagGroups,
   GenClangDiagsIndexName,
@@ -112,8 +113,10 @@
 clEnumValN(GenClangAttrParsedAttrKinds,
"gen-clang-attr-parsed-attr-kinds",
"Generate a clang parsed attribute kinds"),
-clEnumValN(GenClangAttrDump, "gen-clang-attr-dump",
-   "Generate clang attribute dumper"),
+clEnumValN(GenClangAttrTextNodeDump, "gen-clang-attr-text-node-dump",
+   "Generate clang attribute text node dumper"),
+clEnumValN(GenClangAttrNodeTraverse, "gen-clang-attr-node-traverse",
+   "Generate clang attribute traverser"),
 clEnumValN(GenClangDiagsDefs, "gen-clang-diags-defs",
"Generate Clang diagnostics definitions"),
 clEnumValN(GenClangDiagGroups, "gen-clang-diag-groups",
@@ -221,8 +224,11 @@
   case GenClangAttrParsedAttrKinds:
 EmitClangAttrParsedAttrKinds(Records, OS);
 break;
-  case GenClangAttrDump:
-EmitClangAttrDump(Records, OS);
+  case GenClangAttrTextNodeDump:
+EmitClangAttrTextNodeDump(Records, OS);
+break;
+  case GenClangAttrNodeTraverse:
+EmitClangAttrNodeTraverse(Records, OS);
 break;
   case GenClangDiagsDefs:
 EmitClangDiagsDefs(Records, OS, ClangComponent);
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -603,14 +603,15 @@
   OS << "OS << \"";
 }
 
-void writeDump(raw_ostream ) const override {}
+void writeDump(raw_ostream ) const override {
+  OS << "if (!SA->is" << getUpperName() << "Expr())\n";
+  OS << "  dumpType(SA->get" << getUpperName()
+ << "Type()->getType());\n";
+}
 
 void writeDumpChildren(raw_ostream ) const override {
   OS << "if (SA->is" << getUpperName() << "Expr())\n";
   OS << "  dumpStmt(SA->get" << getUpperName() << "Expr());\n";
-  OS << "else\n";
-  OS << "  dumpType(SA->get" << getUpperName()
- << "Type()->getType());\n";
 }
 
 void writeHasChildren(raw_ostream ) const override {
@@ -3697,39 +3698,68 @@
 }
 
 // Emits the code to dump an attribute.
-void EmitClangAttrDump(RecordKeeper , raw_ostream ) {
-  emitSourceFileHeader("Attribute dumper", OS);
+void EmitClangAttrTextNodeDump(RecordKeeper , raw_ostream ) {
+  emitSourceFileHeader("Attribute text node dumper", OS);
 
-  OS << "  switch (A->getKind()) {\n";
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr"), Args;
   for (const auto *Attr : Attrs) {
 const Record  = *Attr;
 if (!R.getValueAsBit("ASTNode"))
   continue;
-OS << "  case attr::" << R.getName() << ": {\n";
 
 // If the attribute has a semantically-meaningful name (which is determined
 // by whether there is a Spelling enumeration for it), then write out the
 // spelling used for the attribute.
+
+std::string functionContent;
+llvm::raw_string_ostream SS(functionContent);
+
 std::vector Spellings = GetFlattenedSpellings(R);
 if (Spellings.size() > 1 && !SpellingNamesAreCommon(Spellings))
-  OS << "OS << \" \" << A->getSpelling();\n";
+  SS << "OS << \" \" << A->getSpelling();\n";
 
 Args = R.getValueAsListOfDefs("Args");
-if 

[PATCH] D55491: Implement TemplateArgument dumping in terms of Visitor

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.

Split the output streaming from the traversal to other AST nodes.


Repository:
  rC Clang

https://reviews.llvm.org/D55491

Files:
  include/clang/AST/TemplateArgumentVisitor.h
  include/clang/AST/TextNodeDumper.h
  lib/AST/ASTDumper.cpp
  lib/AST/TextNodeDumper.cpp

Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -161,6 +161,18 @@
   OS << " " << T.split().Quals.getAsString();
 }
 
+void TextNodeDumper::Visit(const TemplateArgument , SourceRange R, const Decl* From, const char* label) {
+  OS << "TemplateArgument";
+  if (R.isValid())
+dumpSourceRange(R);
+
+  if (From) {
+dumpDeclRef(From, label);
+  }
+
+  TemplateArgumentVisitorType::Visit(TA, R);
+}
+
 void TextNodeDumper::dumpPointer(const void *Ptr) {
   ColorScope Color(OS, ShowColors, AddressColor);
   OS << ' ' << Ptr;
@@ -1022,3 +1034,52 @@
   if (auto N = T->getNumExpansions())
 OS << " expansions " << *N;
 }
+
+void TextNodeDumper::VisitNullTemplateArgument(const TemplateArgument ,
+   SourceRange) {
+  OS << " null";
+}
+
+void TextNodeDumper::VisitTypeTemplateArgument(const TemplateArgument ,
+   SourceRange) {
+  OS << " type";
+  dumpType(TA.getAsType());
+}
+
+void TextNodeDumper::VisitDeclarationTemplateArgument(
+const TemplateArgument , SourceRange) {
+  OS << " decl";
+  dumpDeclRef(TA.getAsDecl());
+}
+
+void TextNodeDumper::VisitNullPtrTemplateArgument(const TemplateArgument ,
+  SourceRange) {
+  OS << " nullptr";
+}
+
+void TextNodeDumper::VisitIntegralTemplateArgument(const TemplateArgument ,
+   SourceRange) {
+  OS << " integral " << TA.getAsIntegral();
+}
+
+void TextNodeDumper::VisitTemplateTemplateArgument(const TemplateArgument ,
+   SourceRange) {
+  OS << " template ";
+  TA.getAsTemplate().dump(OS);
+}
+
+void TextNodeDumper::VisitTemplateExpansionTemplateArgument(
+const TemplateArgument , SourceRange) {
+  OS << " template expansion ";
+  TA.getAsTemplateOrTemplatePattern().dump(OS);
+}
+
+void TextNodeDumper::VisitExpressionTemplateArgument(const TemplateArgument ,
+ SourceRange) {
+  OS << " expr";
+}
+
+void TextNodeDumper::VisitPackTemplateArgument(const TemplateArgument ,
+   SourceRange) {
+  OS << " pack";
+}
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -23,6 +23,7 @@
 #include "clang/AST/DeclVisitor.h"
 #include "clang/AST/LocInfoType.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/AST/TemplateArgumentVisitor.h"
 #include "clang/AST/TextNodeDumper.h"
 #include "clang/AST/TypeVisitor.h"
 #include "clang/Basic/Builtins.h"
@@ -42,7 +43,8 @@
   : public ConstDeclVisitor,
 public ConstStmtVisitor,
 public ConstCommentVisitor,
-public TypeVisitor {
+public TypeVisitor,
+public ConstTemplateArgumentVisitor {
 
 TextTreeStructure TreeStructure;
 TextNodeDumper NodeDumper;
@@ -327,6 +329,16 @@
 
 // Comments.
 void dumpComment(const Comment *C, const FullComment *FC);
+
+void VisitExpressionTemplateArgument(const TemplateArgument ) {
+  dumpStmt(TA.getAsExpr());
+}
+void VisitPackTemplateArgument(const TemplateArgument ) {
+  for (TemplateArgument::pack_iterator I = TA.pack_begin(),
+   E = TA.pack_end();
+   I != E; ++I)
+dumpTemplateArgument(*I);
+}
   };
 }
 
@@ -533,51 +545,8 @@
 void ASTDumper::dumpTemplateArgument(const TemplateArgument , SourceRange R,
  const Decl *From, const char *label) {
   dumpChild([=] {
-OS << "TemplateArgument";
-if (R.isValid())
-  NodeDumper.dumpSourceRange(R);
-
-if (From) {
-  NodeDumper.dumpDeclRef(From, label);
-}
-
-switch (A.getKind()) {
-case TemplateArgument::Null:
-  OS << " null";
-  break;
-case TemplateArgument::Type:
-  OS << " type";
-  NodeDumper.dumpType(A.getAsType());
-  break;
-case TemplateArgument::Declaration:
-  OS << " decl";
-  NodeDumper.dumpDeclRef(A.getAsDecl());
-  break;
-case TemplateArgument::NullPtr:
-  OS << " nullptr";
-  break;
-case TemplateArgument::Integral:
-  OS << " integral " << A.getAsIntegral();
-  break;
-case TemplateArgument::Template:
-  OS << " template ";
-  A.getAsTemplate().dump(OS);
-  break;
-case TemplateArgument::TemplateExpansion:
-  OS << " template 

[PATCH] D55490: Add dumpMethodDeclOverrides to NodeDumper

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

https://reviews.llvm.org/D55490

Files:
  include/clang/AST/TextNodeDumper.h
  lib/AST/ASTDumper.cpp
  lib/AST/TextNodeDumper.cpp


Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -168,6 +168,29 @@
 [=] { OS << ">"; });
 }
 
+void TextNodeDumper::dumpMethodDeclOverrides(const CXXMethodDecl *D) {
+  if (D->size_overridden_methods() != 0) {
+auto dumpOverride = [=](const CXXMethodDecl *D) {
+  SplitQualType T_split = D->getType().split();
+  OS << D << " " << D->getParent()->getName()
+ << "::" << D->getNameAsString() << " '"
+ << QualType::getAsString(T_split, PrintPolicy) << "'";
+};
+
+TreeStructure.addChild([=] {
+  auto Overrides = D->overridden_methods();
+  OS << "Overrides: [ ";
+  dumpOverride(*Overrides.begin());
+  for (const auto *Override :
+   llvm::make_range(Overrides.begin() + 1, Overrides.end())) {
+OS << ", ";
+dumpOverride(Override);
+  }
+  OS << " ]";
+});
+  }
+}
+
 void TextNodeDumper::dumpDeclRef(const Decl *D, const char *Label) {
   if (!D)
 return;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -101,6 +101,9 @@
 for (const ParmVarDecl *Parameter : D->parameters())
   dumpDecl(Parameter);
 }
+void dumpMethodDeclOverrides(const CXXMethodDecl *D) {
+  NodeDumper.dumpMethodDeclOverrides(D);
+}
 
 // C++ Utilities
 void dumpCXXCtorInitializer(const CXXCtorInitializer *Init);
@@ -916,26 +919,7 @@
   dumpCXXCtorInitializer(*I);
 
   if (const CXXMethodDecl *MD = dyn_cast(D)) {
-if (MD->size_overridden_methods() != 0) {
-  auto dumpOverride = [=](const CXXMethodDecl *D) {
-SplitQualType T_split = D->getType().split();
-OS << D << " " << D->getParent()->getName()
-   << "::" << D->getNameAsString() << " '"
-   << QualType::getAsString(T_split, PrintPolicy) << "'";
-  };
-
-  dumpChild([=] {
-auto Overrides = MD->overridden_methods();
-OS << "Overrides: [ ";
-dumpOverride(*Overrides.begin());
-for (const auto *Override :
- llvm::make_range(Overrides.begin() + 1, Overrides.end())) {
-  OS << ", ";
-  dumpOverride(Override);
-}
-OS << " ]";
-  });
-}
+dumpMethodDeclOverrides(MD);
   }
 
   if (D->doesThisDeclarationHaveABody())
Index: include/clang/AST/TextNodeDumper.h
===
--- include/clang/AST/TextNodeDumper.h
+++ include/clang/AST/TextNodeDumper.h
@@ -61,6 +61,7 @@
   void dumpAccessSpecifier(AccessSpecifier AS);
   void dumpCXXTemporary(const CXXTemporary *Temporary);
   void dumpFunctionDeclParameters(const FunctionDecl *D);
+  void dumpMethodDeclOverrides(const CXXMethodDecl *D);
 
   void dumpDeclRef(const Decl *D, const char *Label = nullptr);
 


Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -168,6 +168,29 @@
 [=] { OS << ">"; });
 }
 
+void TextNodeDumper::dumpMethodDeclOverrides(const CXXMethodDecl *D) {
+  if (D->size_overridden_methods() != 0) {
+auto dumpOverride = [=](const CXXMethodDecl *D) {
+  SplitQualType T_split = D->getType().split();
+  OS << D << " " << D->getParent()->getName()
+ << "::" << D->getNameAsString() << " '"
+ << QualType::getAsString(T_split, PrintPolicy) << "'";
+};
+
+TreeStructure.addChild([=] {
+  auto Overrides = D->overridden_methods();
+  OS << "Overrides: [ ";
+  dumpOverride(*Overrides.begin());
+  for (const auto *Override :
+   llvm::make_range(Overrides.begin() + 1, Overrides.end())) {
+OS << ", ";
+dumpOverride(Override);
+  }
+  OS << " ]";
+});
+  }
+}
+
 void TextNodeDumper::dumpDeclRef(const Decl *D, const char *Label) {
   if (!D)
 return;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -101,6 +101,9 @@
 for (const ParmVarDecl *Parameter : D->parameters())
   dumpDecl(Parameter);
 }
+void dumpMethodDeclOverrides(const CXXMethodDecl *D) {
+  NodeDumper.dumpMethodDeclOverrides(D);
+}
 
 // C++ Utilities
 void dumpCXXCtorInitializer(const CXXCtorInitializer *Init);
@@ -916,26 +919,7 @@
   dumpCXXCtorInitializer(*I);
 
   if (const CXXMethodDecl *MD = dyn_cast(D)) {
-if (MD->size_overridden_methods() != 0) {
-  auto dumpOverride = 

[PATCH] D55489: Implement dumpFunctionDeclParameters in NodeDumper

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

https://reviews.llvm.org/D55489

Files:
  include/clang/AST/TextNodeDumper.h
  lib/AST/ASTDumper.cpp
  lib/AST/TextNodeDumper.cpp


Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -162,6 +162,12 @@
   OS << ")";
 }
 
+void TextNodeDumper::dumpFunctionDeclParameters(const FunctionDecl *D) {
+  if (!D->param_begin() && D->getNumParams())
+TreeStructure.addChild(
+[=] { OS << ">"; });
+}
+
 void TextNodeDumper::dumpDeclRef(const Decl *D, const char *Label) {
   if (!D)
 return;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -95,6 +95,12 @@
 void dumpDeclContext(const DeclContext *DC);
 void dumpLookups(const DeclContext *DC, bool DumpDecls);
 void dumpAttr(const Attr *A);
+void dumpFunctionDeclParameters(const FunctionDecl *D) {
+  NodeDumper.dumpFunctionDeclParameters(D);
+  if (D->param_begin() || !D->getNumParams())
+for (const ParmVarDecl *Parameter : D->parameters())
+  dumpDecl(Parameter);
+}
 
 // C++ Utilities
 void dumpCXXCtorInitializer(const CXXCtorInitializer *Init);
@@ -901,11 +907,7 @@
   D->getTemplateSpecializationInfo())
 dumpTemplateArgumentList(*FTSI->TemplateArguments);
 
-  if (!D->param_begin() && D->getNumParams())
-dumpChild([=] { OS << ">"; });
-  else
-for (const ParmVarDecl *Parameter : D->parameters())
-  dumpDecl(Parameter);
+  dumpFunctionDeclParameters(D);
 
   if (const CXXConstructorDecl *C = dyn_cast(D))
 for (CXXConstructorDecl::init_const_iterator I = C->init_begin(),
Index: include/clang/AST/TextNodeDumper.h
===
--- include/clang/AST/TextNodeDumper.h
+++ include/clang/AST/TextNodeDumper.h
@@ -60,6 +60,7 @@
   void dumpName(const NamedDecl *ND);
   void dumpAccessSpecifier(AccessSpecifier AS);
   void dumpCXXTemporary(const CXXTemporary *Temporary);
+  void dumpFunctionDeclParameters(const FunctionDecl *D);
 
   void dumpDeclRef(const Decl *D, const char *Label = nullptr);
 


Index: lib/AST/TextNodeDumper.cpp
===
--- lib/AST/TextNodeDumper.cpp
+++ lib/AST/TextNodeDumper.cpp
@@ -162,6 +162,12 @@
   OS << ")";
 }
 
+void TextNodeDumper::dumpFunctionDeclParameters(const FunctionDecl *D) {
+  if (!D->param_begin() && D->getNumParams())
+TreeStructure.addChild(
+[=] { OS << ">"; });
+}
+
 void TextNodeDumper::dumpDeclRef(const Decl *D, const char *Label) {
   if (!D)
 return;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -95,6 +95,12 @@
 void dumpDeclContext(const DeclContext *DC);
 void dumpLookups(const DeclContext *DC, bool DumpDecls);
 void dumpAttr(const Attr *A);
+void dumpFunctionDeclParameters(const FunctionDecl *D) {
+  NodeDumper.dumpFunctionDeclParameters(D);
+  if (D->param_begin() || !D->getNumParams())
+for (const ParmVarDecl *Parameter : D->parameters())
+  dumpDecl(Parameter);
+}
 
 // C++ Utilities
 void dumpCXXCtorInitializer(const CXXCtorInitializer *Init);
@@ -901,11 +907,7 @@
   D->getTemplateSpecializationInfo())
 dumpTemplateArgumentList(*FTSI->TemplateArguments);
 
-  if (!D->param_begin() && D->getNumParams())
-dumpChild([=] { OS << ">"; });
-  else
-for (const ParmVarDecl *Parameter : D->parameters())
-  dumpDecl(Parameter);
+  dumpFunctionDeclParameters(D);
 
   if (const CXXConstructorDecl *C = dyn_cast(D))
 for (CXXConstructorDecl::init_const_iterator I = C->init_begin(),
Index: include/clang/AST/TextNodeDumper.h
===
--- include/clang/AST/TextNodeDumper.h
+++ include/clang/AST/TextNodeDumper.h
@@ -60,6 +60,7 @@
   void dumpName(const NamedDecl *ND);
   void dumpAccessSpecifier(AccessSpecifier AS);
   void dumpCXXTemporary(const CXXTemporary *Temporary);
+  void dumpFunctionDeclParameters(const FunctionDecl *D);
 
   void dumpDeclRef(const Decl *D, const char *Label = nullptr);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348720 - NFC: Rename TemplateDecl dump utilities

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:33:30 2018
New Revision: 348720

URL: http://llvm.org/viewvc/llvm-project?rev=348720=rev
Log:
NFC: Rename TemplateDecl dump utilities

There is a clang::TemplateDecl AST type, so a method called
VisitTemplateDecl looks like it should 'override' the method from the
base visitor, but it does not because of the extra parameters it takes.

In reality, these methods are utilities, so name them like utilities.

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348720=348719=348720=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Sun Dec  9 05:33:30 2018
@@ -102,6 +102,12 @@ namespace  {
 void dumpTemplateArgumentList(const TemplateArgumentList );
 void dumpTemplateArgument(const TemplateArgument ,
   SourceRange R = SourceRange());
+template 
+void dumpTemplateDeclSpecialization(const SpecializationDecl *D,
+bool DumpExplicitInst,
+bool DumpRefOnly);
+template 
+void dumpTemplateDecl(const TemplateDecl *D, bool DumpExplicitInst);
 
 // Objective-C utilities.
 void dumpObjCTypeParamList(const ObjCTypeParamList *typeParams);
@@ -316,12 +322,6 @@ namespace  {
 void VisitTypeAliasTemplateDecl(const TypeAliasTemplateDecl *D);
 void VisitCXXRecordDecl(const CXXRecordDecl *D);
 void VisitStaticAssertDecl(const StaticAssertDecl *D);
-template
-void VisitTemplateDeclSpecialization(const SpecializationDecl *D,
- bool DumpExplicitInst,
- bool DumpRefOnly);
-template
-void VisitTemplateDecl(const TemplateDecl *D, bool DumpExplicitInst);
 void VisitFunctionTemplateDecl(const FunctionTemplateDecl *D);
 void VisitClassTemplateDecl(const ClassTemplateDecl *D);
 void VisitClassTemplateSpecializationDecl(
@@ -1261,10 +1261,10 @@ void ASTDumper::VisitStaticAssertDecl(co
   dumpStmt(D->getMessage());
 }
 
-template
-void ASTDumper::VisitTemplateDeclSpecialization(const SpecializationDecl *D,
-bool DumpExplicitInst,
-bool DumpRefOnly) {
+template 
+void ASTDumper::dumpTemplateDeclSpecialization(const SpecializationDecl *D,
+   bool DumpExplicitInst,
+   bool DumpRefOnly) {
   bool DumpedAny = false;
   for (auto *RedeclWithBadType : D->redecls()) {
 // FIXME: The redecls() range sometimes has elements of a less-specific
@@ -1303,28 +1303,27 @@ void ASTDumper::VisitTemplateDeclSpecial
 dumpDeclRef(D);
 }
 
-template
-void ASTDumper::VisitTemplateDecl(const TemplateDecl *D,
-  bool DumpExplicitInst) {
+template 
+void ASTDumper::dumpTemplateDecl(const TemplateDecl *D, bool DumpExplicitInst) 
{
   NodeDumper.dumpName(D);
   dumpTemplateParameters(D->getTemplateParameters());
 
   dumpDecl(D->getTemplatedDecl());
 
   for (auto *Child : D->specializations())
-VisitTemplateDeclSpecialization(Child, DumpExplicitInst,
-!D->isCanonicalDecl());
+dumpTemplateDeclSpecialization(Child, DumpExplicitInst,
+   !D->isCanonicalDecl());
 }
 
 void ASTDumper::VisitFunctionTemplateDecl(const FunctionTemplateDecl *D) {
   // FIXME: We don't add a declaration of a function template specialization
   // to its context when it's explicitly instantiated, so dump explicit
   // instantiations when we dump the template itself.
-  VisitTemplateDecl(D, true);
+  dumpTemplateDecl(D, true);
 }
 
 void ASTDumper::VisitClassTemplateDecl(const ClassTemplateDecl *D) {
-  VisitTemplateDecl(D, false);
+  dumpTemplateDecl(D, false);
 }
 
 void ASTDumper::VisitClassTemplateSpecializationDecl(
@@ -1347,7 +1346,7 @@ void ASTDumper::VisitClassScopeFunctionS
 }
 
 void ASTDumper::VisitVarTemplateDecl(const VarTemplateDecl *D) {
-  VisitTemplateDecl(D, false);
+  dumpTemplateDecl(D, false);
 }
 
 void ASTDumper::VisitBuiltinTemplateDecl(const BuiltinTemplateDecl *D) {


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

r348719 - NFC: Move dump of individual comment nodes to NodeDumper

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:30:17 2018
New Revision: 348719

URL: http://llvm.org/viewvc/llvm-project?rev=348719=rev
Log:
NFC: Move dump of individual comment nodes to NodeDumper

Reviewers: aaron.ballman

Subscribers: cfe-commits

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

Modified:
cfe/trunk/include/clang/AST/TextNodeDumper.h
cfe/trunk/lib/AST/ASTDumper.cpp
cfe/trunk/lib/AST/TextNodeDumper.cpp

Modified: cfe/trunk/include/clang/AST/TextNodeDumper.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/TextNodeDumper.h?rev=348719=348718=348719=diff
==
--- cfe/trunk/include/clang/AST/TextNodeDumper.h (original)
+++ cfe/trunk/include/clang/AST/TextNodeDumper.h Sun Dec  9 05:30:17 2018
@@ -16,11 +16,15 @@
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDumperUtils.h"
+#include "clang/AST/CommentCommandTraits.h"
+#include "clang/AST/CommentVisitor.h"
 #include "clang/AST/ExprCXX.h"
 
 namespace clang {
 
-class TextNodeDumper {
+class TextNodeDumper
+: public comments::ConstCommentVisitor {
   raw_ostream 
   const bool ShowColors;
 
@@ -34,9 +38,16 @@ class TextNodeDumper {
   /// The policy to use for printing; can be defaulted.
   PrintingPolicy PrintPolicy;
 
+  const comments::CommandTraits *Traits;
+
+  const char *getCommandName(unsigned CommandID);
+
 public:
   TextNodeDumper(raw_ostream , bool ShowColors, const SourceManager *SM,
- const PrintingPolicy );
+ const PrintingPolicy ,
+ const comments::CommandTraits *Traits);
+
+  void Visit(const comments::Comment *C, const comments::FullComment *FC);
 
   void dumpPointer(const void *Ptr);
   void dumpLocation(SourceLocation Loc);
@@ -47,6 +58,28 @@ public:
   void dumpName(const NamedDecl *ND);
   void dumpAccessSpecifier(AccessSpecifier AS);
   void dumpCXXTemporary(const CXXTemporary *Temporary);
+
+  void visitTextComment(const comments::TextComment *C,
+const comments::FullComment *);
+  void visitInlineCommandComment(const comments::InlineCommandComment *C,
+ const comments::FullComment *);
+  void visitHTMLStartTagComment(const comments::HTMLStartTagComment *C,
+const comments::FullComment *);
+  void visitHTMLEndTagComment(const comments::HTMLEndTagComment *C,
+  const comments::FullComment *);
+  void visitBlockCommandComment(const comments::BlockCommandComment *C,
+const comments::FullComment *);
+  void visitParamCommandComment(const comments::ParamCommandComment *C,
+const comments::FullComment *FC);
+  void visitTParamCommandComment(const comments::TParamCommandComment *C,
+ const comments::FullComment *FC);
+  void visitVerbatimBlockComment(const comments::VerbatimBlockComment *C,
+ const comments::FullComment *);
+  void
+  visitVerbatimBlockLineComment(const comments::VerbatimBlockLineComment *C,
+const comments::FullComment *);
+  void visitVerbatimLineComment(const comments::VerbatimLineComment *C,
+const comments::FullComment *);
 };
 
 } // namespace clang

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348719=348718=348719=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Sun Dec  9 05:30:17 2018
@@ -48,7 +48,6 @@ namespace  {
 TextNodeDumper NodeDumper;
 
 raw_ostream 
-const CommandTraits *Traits;
 
 /// The policy to use for printing; can be defaulted.
 PrintingPolicy PrintPolicy;
@@ -77,7 +76,7 @@ namespace  {
   const SourceManager *SM, bool ShowColors,
   const PrintingPolicy )
 : TreeStructure(OS, ShowColors),
-  NodeDumper(OS, ShowColors, SM, PrintPolicy), OS(OS), Traits(Traits),
+  NodeDumper(OS, ShowColors, SM, PrintPolicy, Traits), OS(OS),
   PrintPolicy(PrintPolicy), ShowColors(ShowColors) {}
 
 void setDeserialize(bool D) { Deserialize = D; }
@@ -433,31 +432,7 @@ namespace  {
 void VisitObjCBoolLiteralExpr(const ObjCBoolLiteralExpr *Node);
 
 // Comments.
-const char *getCommandName(unsigned CommandID);
 void dumpComment(const Comment *C, const FullComment *FC);
-
-// Inline comments.
-void visitTextComment(const TextComment *C, const FullComment *FC);
-void visitInlineCommandComment(const InlineCommandComment *C,
-   const FullComment *FC);
-void visitHTMLStartTagComment(const HTMLStartTagComment *C,
-  const FullComment *FC);
-void visitHTMLEndTagComment(const 

[PATCH] D55190: Move dump of individual comment nodes to NodeDumper

[Stephen Kelly via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348719: NFC: Move dump of individual comment nodes to 
NodeDumper (authored by steveire, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55190?vs=176481=177414#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55190

Files:
  cfe/trunk/include/clang/AST/TextNodeDumper.h
  cfe/trunk/lib/AST/ASTDumper.cpp
  cfe/trunk/lib/AST/TextNodeDumper.cpp

Index: cfe/trunk/include/clang/AST/TextNodeDumper.h
===
--- cfe/trunk/include/clang/AST/TextNodeDumper.h
+++ cfe/trunk/include/clang/AST/TextNodeDumper.h
@@ -16,11 +16,15 @@
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDumperUtils.h"
+#include "clang/AST/CommentCommandTraits.h"
+#include "clang/AST/CommentVisitor.h"
 #include "clang/AST/ExprCXX.h"
 
 namespace clang {
 
-class TextNodeDumper {
+class TextNodeDumper
+: public comments::ConstCommentVisitor {
   raw_ostream 
   const bool ShowColors;
 
@@ -34,9 +38,16 @@
   /// The policy to use for printing; can be defaulted.
   PrintingPolicy PrintPolicy;
 
+  const comments::CommandTraits *Traits;
+
+  const char *getCommandName(unsigned CommandID);
+
 public:
   TextNodeDumper(raw_ostream , bool ShowColors, const SourceManager *SM,
- const PrintingPolicy );
+ const PrintingPolicy ,
+ const comments::CommandTraits *Traits);
+
+  void Visit(const comments::Comment *C, const comments::FullComment *FC);
 
   void dumpPointer(const void *Ptr);
   void dumpLocation(SourceLocation Loc);
@@ -47,6 +58,28 @@
   void dumpName(const NamedDecl *ND);
   void dumpAccessSpecifier(AccessSpecifier AS);
   void dumpCXXTemporary(const CXXTemporary *Temporary);
+
+  void visitTextComment(const comments::TextComment *C,
+const comments::FullComment *);
+  void visitInlineCommandComment(const comments::InlineCommandComment *C,
+ const comments::FullComment *);
+  void visitHTMLStartTagComment(const comments::HTMLStartTagComment *C,
+const comments::FullComment *);
+  void visitHTMLEndTagComment(const comments::HTMLEndTagComment *C,
+  const comments::FullComment *);
+  void visitBlockCommandComment(const comments::BlockCommandComment *C,
+const comments::FullComment *);
+  void visitParamCommandComment(const comments::ParamCommandComment *C,
+const comments::FullComment *FC);
+  void visitTParamCommandComment(const comments::TParamCommandComment *C,
+ const comments::FullComment *FC);
+  void visitVerbatimBlockComment(const comments::VerbatimBlockComment *C,
+ const comments::FullComment *);
+  void
+  visitVerbatimBlockLineComment(const comments::VerbatimBlockLineComment *C,
+const comments::FullComment *);
+  void visitVerbatimLineComment(const comments::VerbatimLineComment *C,
+const comments::FullComment *);
 };
 
 } // namespace clang
Index: cfe/trunk/lib/AST/TextNodeDumper.cpp
===
--- cfe/trunk/lib/AST/TextNodeDumper.cpp
+++ cfe/trunk/lib/AST/TextNodeDumper.cpp
@@ -17,8 +17,29 @@
 
 TextNodeDumper::TextNodeDumper(raw_ostream , bool ShowColors,
const SourceManager *SM,
-   const PrintingPolicy )
-: OS(OS), ShowColors(ShowColors), SM(SM), PrintPolicy(PrintPolicy) {}
+   const PrintingPolicy ,
+   const comments::CommandTraits *Traits)
+: OS(OS), ShowColors(ShowColors), SM(SM), PrintPolicy(PrintPolicy),
+  Traits(Traits) {}
+
+void TextNodeDumper::Visit(const comments::Comment *C,
+   const comments::FullComment *FC) {
+  if (!C) {
+ColorScope Color(OS, ShowColors, NullColor);
+OS << "<<>>";
+return;
+  }
+
+  {
+ColorScope Color(OS, ShowColors, CommentColor);
+OS << C->getCommentKindName();
+  }
+  dumpPointer(C);
+  dumpSourceRange(C->getSourceRange());
+
+  ConstCommentVisitor::visit(C, FC);
+}
 
 void TextNodeDumper::dumpPointer(const void *Ptr) {
   ColorScope Color(OS, ShowColors, AddressColor);
@@ -139,3 +160,126 @@
   dumpPointer(Temporary);
   OS << ")";
 }
+
+const char *TextNodeDumper::getCommandName(unsigned CommandID) {
+  if (Traits)
+return Traits->getCommandInfo(CommandID)->Name;
+  const comments::CommandInfo *Info =
+  comments::CommandTraits::getBuiltinCommandInfo(CommandID);
+  if (Info)
+return Info->Name;
+  return "";
+}
+
+void TextNodeDumper::visitTextComment(const comments::TextComment *C,
+

[PATCH] D55488: Add utility for dumping a label with child nodes

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.

Use it to add optional label nodes to Stmt dumps.  This preserves
behavior of InitExprList dump:

// CHECK-NEXT: `-InitListExpr {{.+}}  'U [3]'
// CHECK-NEXT:   |-array filler
// CHECK-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
// CHECK-NEXT:   `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 'int'
// CHECK-NEXT: `-IntegerLiteral {{.+}}  'int' 1


Repository:
  rC Clang

https://reviews.llvm.org/D55488

Files:
  include/clang/AST/ASTDumperUtils.h
  lib/AST/ASTDumper.cpp


Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -63,6 +63,9 @@
 template void dumpChild(Fn doDumpChild) {
   TreeStructure.addChild(doDumpChild);
 }
+template  void dumpChild(const char *label, Fn doDumpChild) {
+  TreeStructure.addChild(label, doDumpChild);
+}
 
   public:
 ASTDumper(raw_ostream , const CommandTraits *Traits,
@@ -83,7 +86,7 @@
 void setDeserialize(bool D) { Deserialize = D; }
 
 void dumpDecl(const Decl *D);
-void dumpStmt(const Stmt *S);
+void dumpStmt(const Stmt *S, const char *label = nullptr);
 
 // Utilities
 void dumpType(QualType T) { NodeDumper.dumpType(T); }
@@ -1711,8 +1714,8 @@
 //  Stmt dumping methods.
 
//===--===//
 
-void ASTDumper::dumpStmt(const Stmt *S) {
-  dumpChild([=] {
+void ASTDumper::dumpStmt(const Stmt *S, const char *label) {
+  dumpChild(label, [=] {
 if (!S) {
   ColorScope Color(OS, ShowColors, NullColor);
   OS << "<<>>";
@@ -1978,10 +1981,7 @@
 NodeDumper.dumpBareDeclRef(Field);
   }
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpChild([=] {
-  OS << "array filler";
-  dumpStmt(Filler);
-});
+dumpStmt(Filler, "array filler");
   }
 }
 
Index: include/clang/AST/ASTDumperUtils.h
===
--- include/clang/AST/ASTDumperUtils.h
+++ include/clang/AST/ASTDumperUtils.h
@@ -109,6 +109,16 @@
   std::string Prefix;
 
 public:
+  /// Add a child of the current node with a label.
+  /// Calls doAddChild without arguments
+  template  void addChild(const char *label, Fn doAddChild) {
+if (!label)
+  return addChild(doAddChild);
+addChild([=] {
+  OS << label;
+  addChild(doAddChild);
+});
+  }
   /// Add a child of the current node.  Calls doAddChild without arguments
   template  void addChild(Fn doAddChild) {
 // If we're at the top level, there's nothing interesting to do; just


Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -63,6 +63,9 @@
 template void dumpChild(Fn doDumpChild) {
   TreeStructure.addChild(doDumpChild);
 }
+template  void dumpChild(const char *label, Fn doDumpChild) {
+  TreeStructure.addChild(label, doDumpChild);
+}
 
   public:
 ASTDumper(raw_ostream , const CommandTraits *Traits,
@@ -83,7 +86,7 @@
 void setDeserialize(bool D) { Deserialize = D; }
 
 void dumpDecl(const Decl *D);
-void dumpStmt(const Stmt *S);
+void dumpStmt(const Stmt *S, const char *label = nullptr);
 
 // Utilities
 void dumpType(QualType T) { NodeDumper.dumpType(T); }
@@ -1711,8 +1714,8 @@
 //  Stmt dumping methods.
 //===--===//
 
-void ASTDumper::dumpStmt(const Stmt *S) {
-  dumpChild([=] {
+void ASTDumper::dumpStmt(const Stmt *S, const char *label) {
+  dumpChild(label, [=] {
 if (!S) {
   ColorScope Color(OS, ShowColors, NullColor);
   OS << "<<>>";
@@ -1978,10 +1981,7 @@
 NodeDumper.dumpBareDeclRef(Field);
   }
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpChild([=] {
-  OS << "array filler";
-  dumpStmt(Filler);
-});
+dumpStmt(Filler, "array filler");
   }
 }
 
Index: include/clang/AST/ASTDumperUtils.h
===
--- include/clang/AST/ASTDumperUtils.h
+++ include/clang/AST/ASTDumperUtils.h
@@ -109,6 +109,16 @@
   std::string Prefix;
 
 public:
+  /// Add a child of the current node with a label.
+  /// Calls doAddChild without arguments
+  template  void addChild(const char *label, Fn doAddChild) {
+if (!label)
+  return addChild(doAddChild);
+addChild([=] {
+  OS << label;
+  addChild(doAddChild);
+});
+  }
   /// Add a child of the current node.  Calls doAddChild without arguments
   template  void addChild(Fn doAddChild) {
 // If we're at the top level, there's nothing interesting to do; just
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348718 - Revert "Introduce optional labels to dumpStmt"

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:24:40 2018
New Revision: 348718

URL: http://llvm.org/viewvc/llvm-project?rev=348718=rev
Log:
Revert "Introduce optional labels to dumpStmt"

This reverts commit 933402caa09963792058198578522a95f013c69c.

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348718=348717=348718=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Sun Dec  9 05:24:40 2018
@@ -83,8 +83,7 @@ namespace  {
 void setDeserialize(bool D) { Deserialize = D; }
 
 void dumpDecl(const Decl *D);
-void dumpStmt(const Stmt *S, const char *label = nullptr);
-void dumpStmtImpl(const Stmt *S);
+void dumpStmt(const Stmt *S);
 
 // Utilities
 void dumpType(QualType T) { NodeDumper.dumpType(T); }
@@ -1712,18 +1711,7 @@ void ASTDumper::VisitBlockDecl(const Blo
 //  Stmt dumping methods.
 
//===--===//
 
-void ASTDumper::dumpStmt(const Stmt *S, const char *label) {
-  if (label) {
-dumpChild([=] {
-  OS << label;
-  dumpStmtImpl(S);
-});
-  } else {
-dumpStmtImpl(S);
-  }
-}
-
-void ASTDumper::dumpStmtImpl(const Stmt *S) {
+void ASTDumper::dumpStmt(const Stmt *S) {
   dumpChild([=] {
 if (!S) {
   ColorScope Color(OS, ShowColors, NullColor);
@@ -1990,7 +1978,10 @@ void ASTDumper::VisitInitListExpr(const
 NodeDumper.dumpBareDeclRef(Field);
   }
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpStmt(Filler, "array filler");
+dumpChild([=] {
+  OS << "array filler";
+  dumpStmt(Filler);
+});
   }
 }
 


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

r348717 - Introduce optional labels to dumpStmt

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:23:07 2018
New Revision: 348717

URL: http://llvm.org/viewvc/llvm-project?rev=348717=rev
Log:
Introduce optional labels to dumpStmt

If the label is present, it is added as a child, with the statement a
child of the label.  This preserves behavior of the InitListExpr dump
output.

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348717=348716=348717=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Sun Dec  9 05:23:07 2018
@@ -83,7 +83,8 @@ namespace  {
 void setDeserialize(bool D) { Deserialize = D; }
 
 void dumpDecl(const Decl *D);
-void dumpStmt(const Stmt *S);
+void dumpStmt(const Stmt *S, const char *label = nullptr);
+void dumpStmtImpl(const Stmt *S);
 
 // Utilities
 void dumpType(QualType T) { NodeDumper.dumpType(T); }
@@ -1711,7 +1712,18 @@ void ASTDumper::VisitBlockDecl(const Blo
 //  Stmt dumping methods.
 
//===--===//
 
-void ASTDumper::dumpStmt(const Stmt *S) {
+void ASTDumper::dumpStmt(const Stmt *S, const char *label) {
+  if (label) {
+dumpChild([=] {
+  OS << label;
+  dumpStmtImpl(S);
+});
+  } else {
+dumpStmtImpl(S);
+  }
+}
+
+void ASTDumper::dumpStmtImpl(const Stmt *S) {
   dumpChild([=] {
 if (!S) {
   ColorScope Color(OS, ShowColors, NullColor);
@@ -1978,10 +1990,7 @@ void ASTDumper::VisitInitListExpr(const
 NodeDumper.dumpBareDeclRef(Field);
   }
   if (auto *Filler = ILE->getArrayFiller()) {
-dumpChild([=] {
-  OS << "array filler";
-  dumpStmt(Filler);
-});
+dumpStmt(Filler, "array filler");
   }
 }
 


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

r348716 - Inline hasNodes into only caller

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:20:43 2018
New Revision: 348716

URL: http://llvm.org/viewvc/llvm-project?rev=348716=rev
Log:
Inline hasNodes into only caller

It is easier to refactor with fewer utility methods.

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348716=348715=348716=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Sun Dec  9 05:20:43 2018
@@ -91,7 +91,6 @@ namespace  {
 void dumpTypeAsChild(const Type *T);
 void dumpDeclRef(const Decl *Node, const char *Label = nullptr);
 void dumpBareDeclRef(const Decl *Node) { NodeDumper.dumpBareDeclRef(Node); 
}
-bool hasNodes(const DeclContext *DC);
 void dumpDeclContext(const DeclContext *DC);
 void dumpLookups(const DeclContext *DC, bool DumpDecls);
 void dumpAttr(const Attr *A);
@@ -539,15 +538,6 @@ void ASTDumper::dumpDeclRef(const Decl *
   });
 }
 
-bool ASTDumper::hasNodes(const DeclContext *DC) {
-  if (!DC)
-return false;
-
-  return DC->hasExternalLexicalStorage() ||
- (Deserialize ? DC->decls_begin() != DC->decls_end()
-  : DC->noload_decls_begin() != DC->noload_decls_end());
-}
-
 void ASTDumper::dumpDeclContext(const DeclContext *DC) {
   if (!DC)
 return;
@@ -833,9 +823,14 @@ void ASTDumper::dumpDecl(const Decl *D)
   dumpComment(Comment, Comment);
 
 // Decls within functions are visited by the body.
-if (!isa(*D) && !isa(*D) &&
-hasNodes(dyn_cast(D)))
-  dumpDeclContext(cast(D));
+if (!isa(*D) && !isa(*D)) {
+  auto DC = dyn_cast(D);
+  if (DC &&
+  (DC->hasExternalLexicalStorage() ||
+   (Deserialize ? DC->decls_begin() != DC->decls_end()
+: DC->noload_decls_begin() != DC->noload_decls_end(
+dumpDeclContext(DC);
+}
   });
 }
 


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

r348715 - Inline dumpFullComment into callers

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:18:55 2018
New Revision: 348715

URL: http://llvm.org/viewvc/llvm-project?rev=348715=rev
Log:
Inline dumpFullComment into callers

It causes confusion over whether it or dumpComment is the more
important. It is easier to refactor with fewer utility methods.

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348715=348714=348715=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Sun Dec  9 05:18:55 2018
@@ -84,7 +84,6 @@ namespace  {
 
 void dumpDecl(const Decl *D);
 void dumpStmt(const Stmt *S);
-void dumpFullComment(const FullComment *C);
 
 // Utilities
 void dumpType(QualType T) { NodeDumper.dumpType(T); }
@@ -831,7 +830,7 @@ void ASTDumper::dumpDecl(const Decl *D)
 
 if (const FullComment *Comment =
 D->getASTContext().getLocalCommentForDeclUncached(D))
-  dumpFullComment(Comment);
+  dumpComment(Comment, Comment);
 
 // Decls within functions are visited by the body.
 if (!isa(*D) && !isa(*D) &&
@@ -2304,12 +2303,6 @@ const char *ASTDumper::getCommandName(un
   return "";
 }
 
-void ASTDumper::dumpFullComment(const FullComment *C) {
-  if (!C)
-return;
-  dumpComment(C, C);
-}
-
 void ASTDumper::dumpComment(const Comment *C, const FullComment *FC) {
   dumpChild([=] {
 if (!C) {
@@ -2547,12 +2540,16 @@ LLVM_DUMP_METHOD void Comment::dump(cons
 void Comment::dump(raw_ostream , const CommandTraits *Traits,
const SourceManager *SM) const {
   const FullComment *FC = dyn_cast(this);
+  if (!FC)
+return;
   ASTDumper D(OS, Traits, SM);
-  D.dumpFullComment(FC);
+  D.dumpComment(FC, FC);
 }
 
 LLVM_DUMP_METHOD void Comment::dumpColor() const {
   const FullComment *FC = dyn_cast(this);
+  if (!FC)
+return;
   ASTDumper D(llvm::errs(), nullptr, nullptr, /*ShowColors*/true);
-  D.dumpFullComment(FC);
+  D.dumpComment(FC, FC);
 }


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

[PATCH] D55393: Re-order content of template parameter dumps

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 177412.
steveire added a comment.

New approach


Repository:
  rC Clang

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

https://reviews.llvm.org/D55393

Files:
  lib/AST/ASTDumper.cpp
  test/AST/ast-dump-decl.cpp

Index: test/AST/ast-dump-decl.cpp
===
--- test/AST/ast-dump-decl.cpp
+++ test/AST/ast-dump-decl.cpp
@@ -328,20 +328,20 @@
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} TestTemplateDefaultType
 // CHECK-NEXT:   TemplateTypeParmDecl
 // CHECK-NEXT: TemplateArgument type 'int'
-// CHECK-NEXT: inherited from TemplateTypeParm 0x{{[^ ]*}} 'T'
+// CHECK-NEXT:   inherited from TemplateTypeParm 0x{{[^ ]*}} 'T'
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} TestTemplateDefaultNonType
 // CHECK-NEXT:   NonTypeTemplateParmDecl
 // CHECK-NEXT: TemplateArgument expr
+// CHECK-NEXT:   inherited from NonTypeTemplateParm 0x{{[^ ]*}} 'I' 'int'
 // CHECK-NEXT:   ConstantExpr
 // CHECK-NEXT: IntegerLiteral
-// CHECK-NEXT: inherited from NonTypeTemplateParm 0x{{[^ ]*}} 'I' 'int'
 
 // CHECK:  ClassTemplateDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}} {{.*}} TestTemplateTemplateDefaultType
 // CHECK-NEXT:   TemplateTemplateParmDecl
 // CHECK-NEXT: TemplateTypeParmDecl
 // CHECK-NEXT: TemplateArgument
-// CHECK-NEXT: inherited from TemplateTemplateParm 0x{{[^ ]*}} 'TT'
+// CHECK-NEXT:   inherited from TemplateTemplateParm 0x{{[^ ]*}} 'TT'
 
 // PR15220 dump instantiation only once
 namespace testCanonicalTemplate {
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -101,10 +101,14 @@
 void dumpCXXCtorInitializer(const CXXCtorInitializer *Init);
 void dumpTemplateParameters(const TemplateParameterList *TPL);
 void dumpTemplateArgumentListInfo(const TemplateArgumentListInfo );
-void dumpTemplateArgumentLoc(const TemplateArgumentLoc );
+void dumpTemplateArgumentLoc(const TemplateArgumentLoc ,
+ const Decl *From = nullptr,
+ const char *label = nullptr);
 void dumpTemplateArgumentList(const TemplateArgumentList );
 void dumpTemplateArgument(const TemplateArgument ,
-  SourceRange R = SourceRange());
+  SourceRange R = SourceRange(),
+  const Decl *From = nullptr,
+  const char *label = nullptr);
 
 // Objective-C utilities.
 void dumpObjCTypeParamList(const ObjCTypeParamList *typeParams);
@@ -709,8 +713,9 @@
 dumpTemplateArgumentLoc(TALI[i]);
 }
 
-void ASTDumper::dumpTemplateArgumentLoc(const TemplateArgumentLoc ) {
-  dumpTemplateArgument(A.getArgument(), A.getSourceRange());
+void ASTDumper::dumpTemplateArgumentLoc(const TemplateArgumentLoc ,
+const Decl *From, const char *label) {
+  dumpTemplateArgument(A.getArgument(), A.getSourceRange(), From, label);
 }
 
 void ASTDumper::dumpTemplateArgumentList(const TemplateArgumentList ) {
@@ -718,12 +723,17 @@
 dumpTemplateArgument(TAL[i]);
 }
 
-void ASTDumper::dumpTemplateArgument(const TemplateArgument , SourceRange R) {
+void ASTDumper::dumpTemplateArgument(const TemplateArgument , SourceRange R,
+ const Decl *From, const char *label) {
   dumpChild([=] {
 OS << "TemplateArgument";
 if (R.isValid())
   NodeDumper.dumpSourceRange(R);
 
+if (From) {
+  dumpDeclRef(From, label);
+}
+
 switch (A.getKind()) {
 case TemplateArgument::Null:
   OS << " null";
@@ -1408,10 +1418,10 @@
 OS << " ...";
   NodeDumper.dumpName(D);
   if (D->hasDefaultArgument())
-dumpTemplateArgument(D->getDefaultArgument());
-  if (auto *From = D->getDefaultArgStorage().getInheritedFrom())
-dumpDeclRef(From, D->defaultArgumentWasInherited() ? "inherited from"
-   : "previous");
+dumpTemplateArgument(D->getDefaultArgument(), SourceRange(),
+ D->getDefaultArgStorage().getInheritedFrom(),
+ D->defaultArgumentWasInherited() ? "inherited from"
+  : "previous");
 }
 
 void ASTDumper::VisitNonTypeTemplateParmDecl(const NonTypeTemplateParmDecl *D) {
@@ -1421,10 +1431,10 @@
 OS << " ...";
   NodeDumper.dumpName(D);
   if (D->hasDefaultArgument())
-dumpTemplateArgument(D->getDefaultArgument());
-  if (auto *From = D->getDefaultArgStorage().getInheritedFrom())
-dumpDeclRef(From, D->defaultArgumentWasInherited() ? "inherited from"
-   : "previous");
+dumpTemplateArgument(D->getDefaultArgument(), SourceRange(),
+ 

[PATCH] D55398: Re-order content from InitListExpr

[Stephen Kelly via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348714: Re-order content from InitListExpr (authored by 
steveire, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55398

Files:
  cfe/trunk/lib/AST/ASTDumper.cpp


Index: cfe/trunk/lib/AST/ASTDumper.cpp
===
--- cfe/trunk/lib/AST/ASTDumper.cpp
+++ cfe/trunk/lib/AST/ASTDumper.cpp
@@ -1979,16 +1979,16 @@
 }
 
 void ASTDumper::VisitInitListExpr(const InitListExpr *ILE) {
+  if (auto *Field = ILE->getInitializedFieldInUnion()) {
+OS << " field ";
+NodeDumper.dumpBareDeclRef(Field);
+  }
   if (auto *Filler = ILE->getArrayFiller()) {
 dumpChild([=] {
   OS << "array filler";
   dumpStmt(Filler);
 });
   }
-  if (auto *Field = ILE->getInitializedFieldInUnion()) {
-OS << " field ";
-NodeDumper.dumpBareDeclRef(Field);
-  }
 }
 
 void ASTDumper::VisitUnaryOperator(const UnaryOperator *Node) {


Index: cfe/trunk/lib/AST/ASTDumper.cpp
===
--- cfe/trunk/lib/AST/ASTDumper.cpp
+++ cfe/trunk/lib/AST/ASTDumper.cpp
@@ -1979,16 +1979,16 @@
 }
 
 void ASTDumper::VisitInitListExpr(const InitListExpr *ILE) {
+  if (auto *Field = ILE->getInitializedFieldInUnion()) {
+OS << " field ";
+NodeDumper.dumpBareDeclRef(Field);
+  }
   if (auto *Filler = ILE->getArrayFiller()) {
 dumpChild([=] {
   OS << "array filler";
   dumpStmt(Filler);
 });
   }
-  if (auto *Field = ILE->getInitializedFieldInUnion()) {
-OS << " field ";
-NodeDumper.dumpBareDeclRef(Field);
-  }
 }
 
 void ASTDumper::VisitUnaryOperator(const UnaryOperator *Node) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348714 - Re-order content from InitListExpr

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:15:18 2018
New Revision: 348714

URL: http://llvm.org/viewvc/llvm-project?rev=348714=rev
Log:
Re-order content from InitListExpr

Summary:
This causes no change in the output of ast-dump-stmt.cpp due to the way
child nodes are printed with a delay.

Reviewers: aaron.ballman

Subscribers: cfe-commits

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

Modified:
cfe/trunk/lib/AST/ASTDumper.cpp

Modified: cfe/trunk/lib/AST/ASTDumper.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348714=348713=348714=diff
==
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)
+++ cfe/trunk/lib/AST/ASTDumper.cpp Sun Dec  9 05:15:18 2018
@@ -1979,16 +1979,16 @@ void ASTDumper::VisitStringLiteral(const
 }
 
 void ASTDumper::VisitInitListExpr(const InitListExpr *ILE) {
+  if (auto *Field = ILE->getInitializedFieldInUnion()) {
+OS << " field ";
+NodeDumper.dumpBareDeclRef(Field);
+  }
   if (auto *Filler = ILE->getArrayFiller()) {
 dumpChild([=] {
   OS << "array filler";
   dumpStmt(Filler);
 });
   }
-  if (auto *Field = ILE->getInitializedFieldInUnion()) {
-OS << " field ";
-NodeDumper.dumpBareDeclRef(Field);
-  }
 }
 
 void ASTDumper::VisitUnaryOperator(const UnaryOperator *Node) {


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

r348713 - Fix InitListExpr test

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Sun Dec  9 05:13:41 2018
New Revision: 348713

URL: http://llvm.org/viewvc/llvm-project?rev=348713=rev
Log:
Fix InitListExpr test

Wrong case of Check meant this has no effect.

Modified:
cfe/trunk/test/AST/ast-dump-stmt.cpp

Modified: cfe/trunk/test/AST/ast-dump-stmt.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-stmt.cpp?rev=348713=348712=348713=diff
==
--- cfe/trunk/test/AST/ast-dump-stmt.cpp (original)
+++ cfe/trunk/test/AST/ast-dump-stmt.cpp Sun Dec  9 05:13:41 2018
@@ -89,12 +89,12 @@ union U {
 void TestUnionInitList()
 {
   U us[3] = {1};
-// Check: VarDecl {{.+}}  col:5 us 'U [3]' cinit
-// Check-NEXT: `-InitListExpr {{.+}}  'U [3]'
-// Check-NEXT:   |-array filler
-// Check-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
-// Check-NEXT:   |-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
-// Check-NEXT:   | `-IntegerLiteral {{.+}}  'int' 1
+// CHECK: VarDecl {{.+}}  col:5 us 'U [3]' cinit
+// CHECK-NEXT: `-InitListExpr {{.+}}  'U [3]'
+// CHECK-NEXT:   |-array filler
+// CHECK-NEXT:   | `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
+// CHECK-NEXT:   `-InitListExpr {{.+}}  'U' field Field {{.+}} 'i' 
'int'
+// CHECK-NEXT: `-IntegerLiteral {{.+}}  'int' 1
 }
 
 void TestSwitch(int i) {


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

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

[Orivej Desh via Phabricator via cfe-commits]

orivej added a comment.

I have noticed that this change breaks seemingly valid code:

  class A { protected: ~A(); };
  struct B : A {};
  B f() { return B(); }
  B g() { return {}; }

`f` compiles, but `g` fails with `temporary of type 'A' has protected 
destructor`. (g++ 8.2 compiles this file.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D45898



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