[PATCH] D60674: [X86] Restore the pavg intrinsics.

[Craig Topper via Phabricator via cfe-commits]

craig.topper updated this revision to Diff 195090.
craig.topper added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add the clang changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60674

Files:
  clang/include/clang/Basic/BuiltinsX86.def
  clang/lib/Headers/avx2intrin.h
  clang/lib/Headers/avx512bwintrin.h
  clang/lib/Headers/emmintrin.h
  clang/test/CodeGen/avx2-builtins.c
  clang/test/CodeGen/avx512bw-builtins.c
  clang/test/CodeGen/avx512vlbw-builtins.c
  clang/test/CodeGen/sse2-builtins.c
  llvm/include/llvm/IR/IntrinsicsX86.td
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/lib/Target/X86/X86IntrinsicsInfo.h
  llvm/test/CodeGen/X86/avx2-intrinsics-fast-isel.ll
  llvm/test/CodeGen/X86/avx2-intrinsics-x86-upgrade.ll
  llvm/test/CodeGen/X86/avx2-intrinsics-x86.ll
  llvm/test/CodeGen/X86/avx512bw-intrinsics.ll
  llvm/test/CodeGen/X86/avx512bwvl-intrinsics.ll
  llvm/test/CodeGen/X86/sse2-intrinsics-fast-isel.ll
  llvm/test/CodeGen/X86/sse2-intrinsics-x86-upgrade.ll
  llvm/test/CodeGen/X86/sse2-intrinsics-x86.ll

Index: llvm/test/CodeGen/X86/sse2-intrinsics-x86.ll
===
--- llvm/test/CodeGen/X86/sse2-intrinsics-x86.ll
+++ llvm/test/CodeGen/X86/sse2-intrinsics-x86.ll
@@ -946,6 +946,48 @@
 }
 
 
+define <16 x i8> @test_x86_sse2_pavg_b(<16 x i8> %a0, <16 x i8> %a1) {
+; SSE-LABEL: test_x86_sse2_pavg_b:
+; SSE:   ## %bb.0:
+; SSE-NEXT:pavgb %xmm1, %xmm0 ## encoding: [0x66,0x0f,0xe0,0xc1]
+; SSE-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
+;
+; AVX1-LABEL: test_x86_sse2_pavg_b:
+; AVX1:   ## %bb.0:
+; AVX1-NEXT:vpavgb %xmm1, %xmm0, %xmm0 ## encoding: [0xc5,0xf9,0xe0,0xc1]
+; AVX1-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
+;
+; AVX512-LABEL: test_x86_sse2_pavg_b:
+; AVX512:   ## %bb.0:
+; AVX512-NEXT:vpavgb %xmm1, %xmm0, %xmm0 ## EVEX TO VEX Compression encoding: [0xc5,0xf9,0xe0,0xc1]
+; AVX512-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
+  %res = call <16 x i8> @llvm.x86.sse2.pavg.b(<16 x i8> %a0, <16 x i8> %a1) ; <<16 x i8>> [#uses=1]
+  ret <16 x i8> %res
+}
+declare <16 x i8> @llvm.x86.sse2.pavg.b(<16 x i8>, <16 x i8>) nounwind readnone
+
+
+define <8 x i16> @test_x86_sse2_pavg_w(<8 x i16> %a0, <8 x i16> %a1) {
+; SSE-LABEL: test_x86_sse2_pavg_w:
+; SSE:   ## %bb.0:
+; SSE-NEXT:pavgw %xmm1, %xmm0 ## encoding: [0x66,0x0f,0xe3,0xc1]
+; SSE-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
+;
+; AVX1-LABEL: test_x86_sse2_pavg_w:
+; AVX1:   ## %bb.0:
+; AVX1-NEXT:vpavgw %xmm1, %xmm0, %xmm0 ## encoding: [0xc5,0xf9,0xe3,0xc1]
+; AVX1-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
+;
+; AVX512-LABEL: test_x86_sse2_pavg_w:
+; AVX512:   ## %bb.0:
+; AVX512-NEXT:vpavgw %xmm1, %xmm0, %xmm0 ## EVEX TO VEX Compression encoding: [0xc5,0xf9,0xe3,0xc1]
+; AVX512-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
+  %res = call <8 x i16> @llvm.x86.sse2.pavg.w(<8 x i16> %a0, <8 x i16> %a1) ; <<8 x i16>> [#uses=1]
+  ret <8 x i16> %res
+}
+declare <8 x i16> @llvm.x86.sse2.pavg.w(<8 x i16>, <8 x i16>) nounwind readnone
+
+
 define <4 x i32> @test_x86_sse2_pmadd_wd(<8 x i16> %a0, <8 x i16> %a1) {
 ; SSE-LABEL: test_x86_sse2_pmadd_wd:
 ; SSE:   ## %bb.0:
Index: llvm/test/CodeGen/X86/sse2-intrinsics-x86-upgrade.ll
===
--- llvm/test/CodeGen/X86/sse2-intrinsics-x86-upgrade.ll
+++ llvm/test/CodeGen/X86/sse2-intrinsics-x86-upgrade.ll
@@ -630,46 +630,6 @@
 }
 declare <2 x double> @llvm.x86.sse2.div.sd(<2 x double>, <2 x double>) nounwind readnone
 
-define <16 x i8> @mm_avg_epu8(<16 x i8> %a0, <16 x i8> %a1) {
-; SSE-LABEL: mm_avg_epu8:
-; SSE:   ## %bb.0:
-; SSE-NEXT:pavgb %xmm1, %xmm0 ## encoding: [0x66,0x0f,0xe0,0xc1]
-; SSE-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
-;
-; AVX1-LABEL: mm_avg_epu8:
-; AVX1:   ## %bb.0:
-; AVX1-NEXT:vpavgb %xmm1, %xmm0, %xmm0 ## encoding: [0xc5,0xf9,0xe0,0xc1]
-; AVX1-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
-;
-; AVX512-LABEL: mm_avg_epu8:
-; AVX512:   ## %bb.0:
-; AVX512-NEXT:vpavgb %xmm1, %xmm0, %xmm0 ## EVEX TO VEX Compression encoding: [0xc5,0xf9,0xe0,0xc1]
-; AVX512-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
-  %res = call <16 x i8> @llvm.x86.sse2.pavg.b(<16 x i8> %a0, <16 x i8> %a1) ; <<16 x i8>> [#uses=1]
-  ret <16 x i8> %res
-}
-declare <16 x i8> @llvm.x86.sse2.pavg.b(<16 x i8>, <16 x i8>) nounwind readnone
-
-define <8 x i16> @mm_avg_epu16(<8 x i16> %a0, <8 x i16> %a1) {
-; SSE-LABEL: mm_avg_epu16:
-; SSE:   ## %bb.0:
-; SSE-NEXT:pavgw %xmm1, %xmm0 ## encoding: [0x66,0x0f,0xe3,0xc1]
-; SSE-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
-;
-; AVX1-LABEL: mm_avg_epu16:
-; AVX1:   ## %bb.0:
-; AVX1-NEXT:vpavgw %xmm1, %xmm0, %xmm0 ## encoding: [0xc5,0xf9,0xe3,0xc1]
-; AVX1-NEXT:ret{{[l|q]}} ## encoding: [0xc3]
-;
-; AVX512-LABEL: mm_avg_epu16:
-; AVX512:   ## %bb.0:
-; AVX512-NEXT:

[PATCH] D59673: [Driver] Allow setting the DWO name DWARF attribute separately

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert planned changes to this revision.
aaronpuchert added a comment.

In D59673#1461983 , @dblaikie wrote:

> Sure, I think the naming's a bit weird (but hard to come up with good names 
> for any of this)


Agreed, `-split-dwarf-output` is pretty clear, but `-split-dwarf-file` could be 
both the actual filename or the attribute.

There is `test/CodeGen/split-debug-filename.c` checking that we emit 
`!DICompileUnit({{.*}}, splitDebugFilename: "foo.dwo"` into the IR under some 
circumstances, but I'm not sure why. It seems to be ignored by `llc`. What's 
the intended use for this? Your commit message in rC301063 
 suggests that this is needed for implicit 
modules. How does this work? I'm asking of course because I'm not sure whether 
we might need to emit both file names there.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

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

Quuxplusone added inline comments.



Comment at: 
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:304
+static_assert(f(g3) == 4, "");// FIXME: Also well-formed from the 
union rule.
+  // expected-error@-1 {{use of 
undeclared}}
+  }

riccibruno wrote:
> Quuxplusone wrote:
> > riccibruno wrote:
> > > Quuxplusone wrote:
> > > > I see how `g3` matches the example in CWG997
> > > > http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#997
> > > > However, I don't see how CWG997's resolution actually affected this 
> > > > example in the slightest. The wording inserted for CWG997 was, 
> > > > "Additionally, if the aforementioned set of overloaded functions is 
> > > > named with a template-id, its associated classes and namespaces are 
> > > > those of its type template-arguments and its template 
> > > > template-arguments." That makes e.g.
> > > > 
> > > > f(g3)
> > > > 
> > > > consider `N::f`, because `N::S` is a "type template-argument" of the 
> > > > template-id `g3` which names the set of overloaded functions.  
> > > > But it doesn't do anything at all to `f(g3)` because `g3` is not a 
> > > > template-id and doesn't have any template-arguments.
> > > > 
> > > > This piece of ADL is implemented only by GCC (not EDG, Clang, or MSVC), 
> > > > and personally I would very much like to keep it that way. We know 
> > > > there's no real-world code that expects or relies on CWG997 — because 
> > > > such code would never work in practice except on GCC. Let's keep it 
> > > > that way!  As soon as you implement a crazy arcane rule, such that code 
> > > > _could_ portably rely on it, code _will start_ relying on it... and 
> > > > then we'll never be able to simplify the language.
> > > > Case in point: the piece of ADL described in this blog post --
> > > > https://quuxplusone.github.io/blog/2019/04/09/adl-insanity-round-2/
> > > > As soon as the above-described arcane ADL rule was implemented in GCC 
> > > > and Clang, Boost.Hana started relying on it; and now the rule is 
> > > > "locked in" to the paper standard because there's real-world code 
> > > > relying on it.
> > > > Personally I'd like to _keep_ real-world code from relying on CWG997, 
> > > > until someone figures out what CWG was thinking when they added it.
> > > I think that the relevant part of CWG 997 is the removal of the 
> > > restriction on non-dependent parameter types. Sure, `g3` is not a 
> > > `template-id`, but it refers to an overload set which contains the second 
> > > `g3`, and one of the parameter of this second `g3` is `N::Q`.
> > > 
> > > I don't think this is a surprising rule. It matches the general intuition 
> > > that for function types ADL is done based on the function parameter types 
> > > and return type. Not having this rule introduces a difference between 
> > > function templates and functions in overload sets. Consider 
> > > https://godbolt.org/z/UXHqm2 :
> > > ```
> > > namespace N {
> > > struct S1 {};
> > > template  struct S2 {};
> > > 
> > > void f(void (*g)());
> > > }
> > > 
> > > void g1();  // #1
> > > void g1(N::S1); // #2
> > > 
> > > void g2();  // #3
> > > template  void g2(N::S2);// #4
> > > 
> > > void test() {
> > > f(g1); // ok, g1 is #1
> > > f(g2); // should be ok, g2 is #3
> > > }
> > > ```
> > > I think that the relevant part of CWG 997 is the removal of the 
> > > restriction on non-dependent parameter types.
> > 
> > Ah, I had missed the removal of the word `(non-dependent)` in my reading of 
> > CWG997. So just that one-word removal is what fixed their example, and is 
> > what you're testing with `g3`.
> > 
> > I still object to `g2` — I would like that `FIXME` to say `PLEASEDONTFIXME` 
> > or something. :)
> I don't understand your objection to the template-id rule of CWG 997 (which 
> is what `f(g2)` tests). It seems to me that the following should be 
> well-formed (https://godbolt.org/z/J5uhQ-) :
> 
> 
> ```
> namespace N {
> template  void f(T &&);
> struct S {};
> }
> 
> template  struct C {};
> template  void g();
> 
> void test() {
> f(C{}); // ok
> f(g);   // should be ok (and is ok according to the spec)
> }
> ```
I admit that your comparison makes the rule look reasonable. :)  But here are 
some comparisons designed to make the rule look UNreasonable.
https://godbolt.org/z/n5kvUE

```
namespace N {
void foo(void(*)());

class T {};
using A = int;
template class TT {};
template using AT = int;
void F();
void (*V)();
}

template void ft_t();
template void (*vt_t)();
template void ft_nt();
template void (*vt_nt)();
template class> void ft_tt();
template class> void (*vt_tt)();

void test() {
foo(ft_t);  // OK: ADL considers N::foo (nobody but GCC implements 
this)
foo(vt_t);  // Error: ADL does not consider N::foo

[PATCH] D59639: [clangd] Print template arguments helper

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.



Comment at: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt:16
   Annotations.cpp
+  PrintASTTests.cpp
   BackgroundIndexTests.cpp

Keep alphabetized?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59639



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

[PATCH] D60672: [libclang] visit c++14 lambda capture init expressions

[Milian Wolff via Phabricator via cfe-commits]

milianw created this revision.
milianw added reviewers: clang-c, cfe-commits, hokein, nik, yvvan, marcobubke.
Herald added a subscriber: arphaman.
Herald added a project: clang.

Previously, libclang users could visit the non-init C++11 lambda
captures, e.g. `[foo]`. But C++14 init-captures such as `[foo=bar]`
were skipped, which is fixed by this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60672

Files:
  clang/test/Index/cxx14-lambdas.cpp
  clang/tools/libclang/CIndex.cpp


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3120,12 +3120,11 @@
   }
 
   case VisitorJob::LambdaExprPartsKind: {
-// Visit captures.
+// Visit non-init captures.
 const LambdaExpr *E = cast()->get();
 for (LambdaExpr::capture_iterator C = E->explicit_capture_begin(),
CEnd = E->explicit_capture_end();
  C != CEnd; ++C) {
-  // FIXME: Lambda init-captures.
   if (!C->capturesVariable())
 continue;
 
@@ -3134,6 +3133,11 @@
   TU)))
 return true;
 }
+// Visit init captures
+for (auto InitExpr : E->capture_inits()) {
+  if (Visit(InitExpr))
+return true;
+}
 
 TypeLoc TL = E->getCallOperator()->getTypeSourceInfo()->getTypeLoc();
 // Visit parameters and return type, if present.
Index: clang/test/Index/cxx14-lambdas.cpp
===
--- /dev/null
+++ clang/test/Index/cxx14-lambdas.cpp
@@ -0,0 +1,38 @@
+// Test is line- and column-sensitive; see below.
+
+typedef int Integer;
+struct X {
+  void f() {
+int localA, localB;
+auto lambda = [ptr = , copy = localB] (Integer x) -> Integer {
+  return *ptr + copy + x;
+};
+  }
+};
+
+// RUN: c-index-test -test-load-source all -std=c++14 %s | FileCheck 
-check-prefix=CHECK-LOAD %s
+// CHECK-LOAD: cxx14-lambdas.cpp:7:5: DeclStmt= Extent=[7:5 - 9:7]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:10: VarDecl=lambda:7:10 (Definition) 
Extent=[7:5 - 9:6]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:19: UnexposedExpr= Extent=[7:19 - 9:6]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:19: CallExpr= Extent=[7:19 - 9:6]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:19: UnexposedExpr= Extent=[7:19 - 9:6]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:19: LambdaExpr= Extent=[7:19 - 9:6]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:20: VariableRef=ptr:7:20 Extent=[7:20 - 
7:23]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:35: VariableRef=copy:7:35 Extent=[7:35 - 
7:39]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:27: DeclRefExpr=localA:6:9 Extent=[7:27 - 
7:33]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:42: DeclRefExpr=localB:6:17 Extent=[7:42 - 
7:48]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:59: ParmDecl=x:7:59 (Definition) 
Extent=[7:51 - 7:60]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:51: TypeRef=Integer:3:13 Extent=[7:51 - 
7:58]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:65: TypeRef=Integer:3:13 Extent=[7:65 - 
7:72]
+// CHECK-LOAD: cxx14-lambdas.cpp:7:73: CompoundStmt= Extent=[7:73 - 9:6]
+// CHECK-LOAD: cxx14-lambdas.cpp:8:7: ReturnStmt= Extent=[8:7 - 8:29]
+
+// RUN: env CINDEXTEST_INDEXLOCALSYMBOLS=1 c-index-test -index-file -std=c++14 
%s | FileCheck -check-prefix=CHECK-INDEX %s
+// CHECK-INDEX: [indexEntityReference]: kind: variable | name: ptr | USR: 
c:cxx14-lambdas.cpp@139@S@X@F@f#@Sa@F@operator()#I#1@ptr | lang: C | cursor: 
VariableRef=ptr:7:20 | loc: 7:20
+// CHECK-INDEX: [indexEntityReference]: kind: variable | name: copy | USR: 
c:cxx14-lambdas.cpp@154@S@X@F@f#@Sa@F@operator()#I#1@copy | lang: C | cursor: 
VariableRef=copy:7:35 | loc: 7:35
+// CHECK-INDEX: [indexDeclaration]: kind: variable | name: x | USR: 
c:cxx14-lambdas.cpp@170@S@X@F@f#@Sa@F@operator()#I#1@x | lang: C | cursor: 
ParmDecl=x:7:59 (Definition) | loc: 7:59
+// CHECK-INDEX: [indexEntityReference]: kind: typedef | name: Integer | USR: 
c:cxx14-lambdas.cpp@T@Integer | lang: C | cursor: TypeRef=Integer:3:13 | loc: 
7:51
+// CHECK-INDEX: [indexEntityReference]: kind: typedef | name: Integer | USR: 
c:cxx14-lambdas.cpp@T@Integer | lang: C | cursor: TypeRef=Integer:3:13 | loc: 
7:65
+// CHECK-INDEX: [indexEntityReference]: kind: variable | name: ptr | USR: 
c:cxx14-lambdas.cpp@139@S@X@F@f#@Sa@F@operator()#I#1@ptr | lang: C | cursor: 
DeclRefExpr=ptr:7:20 | loc: 8:15
+// CHECK-INDEX: [indexEntityReference]: kind: variable | name: copy | USR: 
c:cxx14-lambdas.cpp@154@S@X@F@f#@Sa@F@operator()#I#1@copy | lang: C | cursor: 
DeclRefExpr=copy:7:35 | loc: 8:21
+// CHECK-INDEX: [indexEntityReference]: kind: variable | name: x | USR: 
c:cxx14-lambdas.cpp@170@S@X@F@f#@Sa@F@operator()#I#1@x | lang: C | cursor: 
DeclRefExpr=x:7:59 | loc: 8:28


Index: clang/tools/libclang/CIndex.cpp
===
--- 

[PATCH] D60363: [clang-format] [PR41170] Break after return type ignored with certain comments positions

[Owen Pan via Phabricator via cfe-commits]

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

Looks good!


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

https://reviews.llvm.org/D60363



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

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 195076.
riccibruno added a comment.

Also test that typedef-names and using-declarations are not considered.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60570

Files:
  
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
  test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-inline-namespace.cpp
  test/CXX/basic/basic.lookup/basic.lookup.argdep/p2.cpp
  test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
  test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp

Index: test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
===
--- test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
+++ test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
@@ -67,3 +67,96 @@
 foo(a, 10); // expected-error {{no matching function for call to 'foo'}}
   }
 }
+
+
+// Check the rules described in p4:
+//  When considering an associated namespace, the lookup is the same as the lookup
+//  performed when the associated namespace is used as a qualifier (6.4.3.2) except that:
+
+//  - Any using-directives in the associated namespace are ignored.
+namespace test_using_directives {
+  namespace M { struct S; }
+  namespace N {
+void f(M::S); // expected-note {{declared here}}
+  }
+  namespace M {
+using namespace N;
+struct S {};
+  }
+  void test() {
+M::S s;
+f(s); // expected-error {{use of undeclared}}
+M::f(s); // ok
+  }
+}
+
+//  - Any namespace-scope friend functions or friend function templates declared in
+//associated classes are visible within their respective namespaces even if
+//they are not visible during an ordinary lookup
+// (Note: For the friend declaration to be visible, the corresponding class must be
+//  included in the set of associated classes. Merely including the namespace in
+//  the set of associated namespaces is not enough.)
+namespace test_friend1 {
+  namespace N {
+struct S;
+struct T {
+  friend void f(S); // #1
+};
+struct S { S(); S(T); };
+  }
+
+  void test() {
+N::S s;
+N::T t;
+f(s); // expected-error {{use of undeclared}}
+f(t); // ok, #1
+  }
+}
+
+// credit: Arthur O’Dwyer
+namespace test_friend2 {
+  struct A {
+struct B {
+struct C {};
+};
+friend void foo(...); // #1
+  };
+
+  struct D {
+friend void foo(...); // #2
+  };
+  template struct E {
+struct F {};
+  };
+
+  template struct G {};
+  template struct H {};
+  template struct I {};
+  struct J { friend void foo(...) {} }; // #3
+
+  void test() {
+A::B::C c;
+foo(c); // #1 is not visible since A is not an associated class
+// expected-error@-1 {{use of undeclared}}
+E::F f;
+foo(f); // #2 is not visible since D is not an associated class
+// expected-error@-1 {{use of undeclared}}
+G > > j;
+foo(j);  // ok, #3.
+  }
+}
+
+//  - All names except those of (possibly overloaded) functions and
+//function templates are ignored.
+namespace test_other_names {
+  namespace N {
+struct S {};
+struct Callable { void operator()(S); };
+static struct Callable Callable;
+  }
+
+  void test() {
+N::S s;
+Callable(s); // expected-error {{use of undeclared}}
+  }
+}
Index: test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
===
--- test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
+++ test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
@@ -18,3 +18,67 @@
 }
   };
 }
+
+// If X contains [...] then Y is empty.
+// - a declaration of a class member
+namespace test_adl_suppression_by_class_member {
+  namespace N {
+struct T {};
+void f(T); // expected-note {{declared here}}
+  }
+  struct S {
+void f();
+void test() {
+  N::T t;
+  f(t); // expected-error {{too many arguments}}
+}
+  };
+}
+
+// - a block-scope function declaration that is not a using-declaration
+namespace test_adl_suppression_by_block_scope {
+  namespace N {
+struct S {};
+void f(S);
+  }
+  namespace M { void f(int); } // expected-note 2{{candidate}}
+  void test1() {
+N::S s;
+using M::f;
+f(s); // ok
+  }
+
+  void test2() {
+N::S s;
+extern void f(char); // expected-note {{passing argument to parameter here}}
+f(s); // expected-error {{no viable conversion from 'N::S' to 'char'}}
+  }
+
+  void test3() {
+N::S s;
+extern void f(char); // expected-note {{candidate}}
+using M::f;
+f(s); // expected-error {{no matching function}}
+  }
+
+  void test4() {
+N::S s;
+using M::f;
+extern void f(char); // expected-note {{candidate}}
+f(s); // expected-error {{no matching function}}
+  }
+
+}
+
+// - a declaration that is neither a function nor a function template
+namespace test_adl_suppression_by_non_function {
+  namespace N {
+ 

[PATCH] D53076: [analyzer] ConditionBRVisitor: Enhance to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks you @NoQ for the great idea!

Without the duplicated `BugReport.cpp` reports it could be an on-by-default 
patch, see:

Before:
F8685549: before.html 

After:
F8685550: after.html 

It is not perfect, but I like that and I have already got ideas how to use 
these new pieces.




Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187-190
+  // The value may hard-coded.
+  SVal HardCodedSVal = State->getSVal(CondVarExpr, LCtx);
+  if (auto HardCodedCI = HardCodedSVal.getAs())
+return >getValue();

NoQ wrote:
> I don't see this triggered on tests. It looks to me that this function is for 
> now only applied to `DeclRefExpr`s that are always glvalues and therefore 
> should never be evaluated to a `nonloc` value.
Good catch!


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

https://reviews.llvm.org/D53076



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

[PATCH] D60663: Time profiler: small fixes and optimizations

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev updated this revision to Diff 195069.
anton-afanasyev added a comment.

Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -1,30 +1,35 @@
 //===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 //
-/// \file Hierarchical time profiler implementation.
+// This file implements hierarchical time profiler.
 //
 //===--===//
 
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include 
 #include 
 #include 
-#include 
 #include 
 
 using namespace std::chrono;
 
 namespace llvm {
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",
+cl::desc(
+"Minimum time granularity (in microsecons) traced by time profiler"),
+cl::init(500));
+
 TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 
 static std::string escapeString(StringRef Src) {
@@ -61,18 +66,21 @@
   DurationType Duration;
   std::string Name;
   std::string Detail;
+
+  Entry(time_point &, DurationType &, std::string &,
+std::string &)
+  : Start(std::move(S)), Duration(std::move(D)), Name(std::move(N)),
+Detail(std::move(Dt)){};
 };
 
 struct TimeTraceProfiler {
   TimeTraceProfiler() {
-Stack.reserve(8);
-Entries.reserve(128);
 StartTime = steady_clock::now();
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
-Stack.push_back(std::move(E));
+Stack.emplace_back(steady_clock::now(), DurationType{}, std::move(Name),
+   Detail());
   }
 
   void end() {
@@ -80,8 +88,8 @@
 auto  = Stack.back();
 E.Duration = steady_clock::now() - E.Start;
 
-// Only include sections longer than 500us.
-if (duration_cast(E.Duration).count() > 500)
+// Only include sections longer than TimeTraceGranularity.
+if (duration_cast(E.Duration).count() > TimeTraceGranularity)
   Entries.emplace_back(E);
 
 // Track total time taken by each "name", but only the topmost levels of
@@ -100,20 +108,20 @@
 Stack.pop_back();
   }
 
-  void Write(std::unique_ptr ) {
+  void Write(raw_pwrite_stream ) {
 assert(Stack.empty() &&
"All profiler sections should be ended when calling Write");
 
-*OS << "{ \"traceEvents\": [\n";
+OS << "{ \"traceEvents\": [\n";
 
 // Emit all events for the main flame graph.
 for (const auto  : Entries) {
   auto StartUs = duration_cast(E.Start - StartTime).count();
   auto DurUs = duration_cast(E.Duration).count();
-  *OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
-  << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
-  << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
-  << "\"} },\n";
+  OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
+ << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
+ << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
+ << "\"} },\n";
 }
 
 // Emit totals by section name as additional "thread" events, sorted from
@@ -121,32 +129,32 @@
 int Tid = 1;
 std::vector SortedTotals;
 SortedTotals.reserve(CountAndTotalPerName.size());
-for (const auto  : CountAndTotalPerName) {
+for (const auto  : CountAndTotalPerName)
   SortedTotals.emplace_back(E.getKey(), E.getValue());
-}
-std::sort(SortedTotals.begin(), SortedTotals.end(),
-  [](const NameAndCountAndDurationType ,
- const NameAndCountAndDurationType ) {
-return A.second.second > B.second.second;
-  });
+
+llvm::sort(SortedTotals.begin(), SortedTotals.end(),
+   [](const NameAndCountAndDurationType ,
+  const NameAndCountAndDurationType ) {
+ return A.second.second > B.second.second;
+   });
 for (const auto  : SortedTotals) {
   auto DurUs = 

[PATCH] D60663: Time profiler: small fixes and optimizations

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev updated this revision to Diff 195068.
anton-afanasyev marked 2 inline comments as done.
anton-afanasyev added a comment.

Updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -1,19 +1,19 @@
 //===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 //
-/// \file Hierarchical time profiler implementation.
+// This file implements hierarchical time profiler.
 //
 //===--===//
 
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include 
 #include 
@@ -25,6 +25,12 @@
 
 namespace llvm {
 
+static cl::opt TimeTraceGranularity(
+"time-trace-granularity",
+cl::desc(
+"Minimum time granularity (in microsecons) traced by time profiler"),
+cl::init(500));
+
 TimeTraceProfiler *TimeTraceProfilerInstance = nullptr;
 
 static std::string escapeString(StringRef Src) {
@@ -61,18 +67,21 @@
   DurationType Duration;
   std::string Name;
   std::string Detail;
+
+  Entry(time_point &, DurationType &, std::string &,
+std::string &)
+  : Start(std::move(S)), Duration(std::move(D)), Name(std::move(N)),
+Detail(std::move(Dt)){};
 };
 
 struct TimeTraceProfiler {
   TimeTraceProfiler() {
-Stack.reserve(8);
-Entries.reserve(128);
 StartTime = steady_clock::now();
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
-Stack.push_back(std::move(E));
+Stack.emplace_back(steady_clock::now(), DurationType{}, std::move(Name),
+   Detail());
   }
 
   void end() {
@@ -80,8 +89,8 @@
 auto  = Stack.back();
 E.Duration = steady_clock::now() - E.Start;
 
-// Only include sections longer than 500us.
-if (duration_cast(E.Duration).count() > 500)
+// Only include sections longer than TimeTraceGranularity.
+if (duration_cast(E.Duration).count() > TimeTraceGranularity)
   Entries.emplace_back(E);
 
 // Track total time taken by each "name", but only the topmost levels of
@@ -100,20 +109,20 @@
 Stack.pop_back();
   }
 
-  void Write(std::unique_ptr ) {
+  void Write(raw_pwrite_stream ) {
 assert(Stack.empty() &&
"All profiler sections should be ended when calling Write");
 
-*OS << "{ \"traceEvents\": [\n";
+OS << "{ \"traceEvents\": [\n";
 
 // Emit all events for the main flame graph.
 for (const auto  : Entries) {
   auto StartUs = duration_cast(E.Start - StartTime).count();
   auto DurUs = duration_cast(E.Duration).count();
-  *OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
-  << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
-  << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
-  << "\"} },\n";
+  OS << "{ \"pid\":1, \"tid\":0, \"ph\":\"X\", \"ts\":" << StartUs
+ << ", \"dur\":" << DurUs << ", \"name\":\"" << escapeString(E.Name)
+ << "\", \"args\":{ \"detail\":\"" << escapeString(E.Detail)
+ << "\"} },\n";
 }
 
 // Emit totals by section name as additional "thread" events, sorted from
@@ -121,32 +130,32 @@
 int Tid = 1;
 std::vector SortedTotals;
 SortedTotals.reserve(CountAndTotalPerName.size());
-for (const auto  : CountAndTotalPerName) {
+for (const auto  : CountAndTotalPerName)
   SortedTotals.emplace_back(E.getKey(), E.getValue());
-}
-std::sort(SortedTotals.begin(), SortedTotals.end(),
-  [](const NameAndCountAndDurationType ,
- const NameAndCountAndDurationType ) {
-return A.second.second > B.second.second;
-  });
+
+llvm::sort(SortedTotals.begin(), SortedTotals.end(),
+   [](const NameAndCountAndDurationType ,
+  const NameAndCountAndDurationType ) {
+ return A.second.second > B.second.second;
+   });
 for (const auto  : SortedTotals) {
   auto 

[PATCH] D60663: Time profiler: small fixes and optimizations

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 7 inline comments as done.
anton-afanasyev added a subscriber: aras-p.
anton-afanasyev added a comment.

In D60663#1465721 , @lebedev.ri wrote:

> Looks good ignoring the json bits.
>
> Re license:
>
> > https://reviews.llvm.org/D58675
> >  This is the first part of time tracing system, I have splitted them cause 
> > this part is mostly written by Aras Pranckevicius except of several minor 
> > fixes concerning formatting.
>
> So i can't and won't claim any legal knowledge, but it maybe would be good 
> for him to at least comment here, that he is ok with this?


Oh, sure! @aras-p -- I have changed license header to fit it with current LLVM 
Relicensing state. Are you ok with this? (see 
https://reviews.llvm.org/D60663#change-jr7Lagn5WNFy)




Comment at: llvm/lib/Support/TimeProfiler.cpp:20
 #include 
 #include 
 #include 

lebedev.ri wrote:
> Unused header?
Yes, thanks.



Comment at: llvm/lib/Support/TimeProfiler.cpp:47-48
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
+Entry E = {steady_clock::now(), {}, std::move(Name), Detail()};
 Stack.push_back(std::move(E));
   }

lebedev.ri wrote:
> Does
> ```
> Stack.emplace_back(steady_clock::now(), {}, std::move(Name), Detail());
> ```
> not work?
> (also, the `std::string` returned from `Detail` function invocation is moved?)
Ok, changed.



Comment at: llvm/lib/Support/TimeProfiler.cpp:57
 // Only include sections longer than 500us.
 if (duration_cast(E.Duration).count() > 500)
   Entries.emplace_back(E);

lebedev.ri wrote:
> I feel like this should be 
> ```
> static cl::opt TimeProfileGranularity(
> "time-profile-granularity",
> cl::desc(""),
> cl::init(500));
> ```
> ?
I planned to change this later together with unit tests (cause they need small 
time granularity), but can fix it now. Changed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663



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

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

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

xazax.hun added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:326
+
+// We should not catch move assignment operators.
+class MoveAssignOperator {

While I do agree move assignment operators should not be checked by default 
some would argue that move assignments should still be tolerant to self 
assignments as one could write something like:
```
x = std::move(aliasToX);
```
Having an option to also check move assignments in a follow-up patch would be 
also nice to have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60507



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

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:51
+  // Matcher for standard smart pointers.
+  const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(classTemplateSpecializationDecl(

xazax.hun wrote:
> JonasToth wrote:
> > what about `auto_ptr`? I am actually not sure if we should care, as its 
> > deprecated and removed already, on the other hand legacy code probably 
> > still has it.
> I am perfectly fine with addressing this in a follow-up patch or even not 
> addressing at all, but I think for some users it might be useful to be able 
> to specify a set of suspicious types as a configuration option such as custom 
> smart pointers, handles and so on. 
Seems a good idea for a follow-up change.
Actually, with an earlier version of this patch I found another vulnerable copy 
assignment operator in llvm code which uses a custom pointer type 
(PointerIntPair):
FunctionInfo =(const FunctionInfo )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60507



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

[PATCH] D60573: [Sema] ADL: Associated namespaces for class types and enumeration types (CWG 1691)

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D60573#1463777 , @rjmccall wrote:

> In D60573#1463641 , @riccibruno 
> wrote:
>
> > In D60573#1463569 , @rjmccall 
> > wrote:
> >
> > > Hmm.  Does this never impact code that's just using a locally-defined 
> > > type within its scope?  I guess if ADL is involved, unqualified lookup 
> > > must have reached the scope of the innermost namespace, and so it would 
> > > be searching that namespace anyway.
> > >
> > > In that case, I think I'm mollified that the source-compatibility risk is 
> > > low and we should just unconditionally apply the new rule.  LGTM.
> >
> >
> > I am not sure about what you mean. It is certainly possible to construct a 
> > piece of C++11 code which breaks with this patch.
>
>
> Yes, but these examples are contrived, so it's easy to rationalize that the 
> source impact is low.  The typical use-pattern of a local type is that you 
> only use it locally, so the most important question would be whether it is 
> possible to change the semantics of, say,
>
>   void test() {
> struct A { ... };
> foo(A{});
>   }
>   
>
> But I think the answer is "no", for the reasons I explained.


I can't imagine a way to change the result of the lookup for `foo` in an 
example like this without escaping the local type from the function.

Thanks for the review !


Repository:
  rC Clang

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

https://reviews.llvm.org/D60573



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

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

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

xazax.hun added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:51
+  // Matcher for standard smart pointers.
+  const auto SmartPointerType = qualType(hasUnqualifiedDesugaredType(
+  recordType(hasDeclaration(classTemplateSpecializationDecl(

JonasToth wrote:
> what about `auto_ptr`? I am actually not sure if we should care, as its 
> deprecated and removed already, on the other hand legacy code probably still 
> has it.
I am perfectly fine with addressing this in a follow-up patch or even not 
addressing at all, but I think for some users it might be useful to be able to 
specify a set of suspicious types as a configuration option such as custom 
smart pointers, handles and so on. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60507



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

[PATCH] D60665: [Sema] ADL: Template arguments in a template-id naming a set of overloaded functions (part of CWG 997)

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: Quuxplusone, rsmith, rjmccall.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.
riccibruno added a parent revision: D60573: [Sema] ADL: Associated namespaces 
for class types and enumeration types (CWG 1691).

CWG 997 added the following wording at the end of `[basic.lookup.argdep]p2`

> [...] Additionally, if the aforementioned set of overloaded functions is 
> named with a template-id, its associated classes and namespaces are those of 
> its type template-arguments and its template template-arguments.

This resolve CWG 1015 and part of CWG 997 (I will mark it as done when the rest 
of it is implemented).


Repository:
  rC Clang

https://reviews.llvm.org/D60665

Files:
  lib/Sema/SemaLookup.cpp
  
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
  test/CXX/drs/dr10xx.cpp

Index: test/CXX/drs/dr10xx.cpp
===
--- test/CXX/drs/dr10xx.cpp
+++ test/CXX/drs/dr10xx.cpp
@@ -35,6 +35,19 @@
   Third > t; // expected-note {{in instantiation of default argument}}
 }
 
+namespace dr1015 { // dr1015: 9
+  namespace N {
+struct S {};
+void f(void (*)(S));
+  }
+
+  template void g(T);
+
+  void h() {
+f(g); // ok, N::f
+  }
+}
+
 namespace dr1048 { // dr1048: 3.6
   struct A {};
   const A f();
Index: test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
===
--- test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
+++ test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
@@ -315,7 +315,7 @@
   namespace N {
 struct S {};
 constexpr int f(int (*g)()) { return g(); }
-// expected-note@-1 2{{'N::f' declared here}}
+// expected-note@-1 {{'N::f' declared here}}
 template  struct Q;
   }
 
@@ -328,11 +328,13 @@
   constexpr int g3() { return 4; }
   template  constexpr int g3(T, N::Q) { return 5; }
 
+  template  class Z> constexpr int g4() { return 6; }
+
   void test() {
-static_assert(f(g1) == 1, "");// Well-formed from the union rule above
-static_assert(f(g2) == 3, "");  // FIXME: Well-formed from the template-id rule above.
-  // expected-error@-1 {{use of undeclared}}
+static_assert(f(g1) == 1, "");// Well-formed from the union rule.
+static_assert(f(g2) == 3, "");  // Well-formed from the template-id rule.
 static_assert(f(g3) == 4, "");// FIXME: Also well-formed from the union rule.
   // expected-error@-1 {{use of undeclared}}
+static_assert(f(g4) == 6, "");  // Well-formed from the template-id rule.
   }
 }
Index: lib/Sema/SemaLookup.cpp
===
--- lib/Sema/SemaLookup.cpp
+++ lib/Sema/SemaLookup.cpp
@@ -2625,12 +2625,11 @@
   }
 }
 
-// Add the associated classes and namespaces for
-// argument-dependent lookup with an argument of type T
-// (C++ [basic.lookup.koenig]p2).
+// Add the associated classes and namespaces for argument-dependent
+// lookup with an argument of type T (C++ [basic.lookup.argdep]p2).
 static void
 addAssociatedClassesAndNamespaces(AssociatedLookup , QualType Ty) {
-  // C++ [basic.lookup.koenig]p2:
+  // C++ [basic.lookup.argdep]p2:
   //
   //   For each argument type T in the function call, there is a set
   //   of zero or more associated namespaces and a set of zero or more
@@ -2806,7 +2805,7 @@
   AssociatedLookup Result(*this, InstantiationLoc,
   AssociatedNamespaces, AssociatedClasses);
 
-  // C++ [basic.lookup.koenig]p2:
+  // C++ [basic.lookup.argdep]p2:
   //   For each argument type T in the function call, there is a set
   //   of zero or more associated namespaces and a set of zero or more
   //   associated classes to be considered. The sets of namespaces and
@@ -2821,15 +2820,16 @@
   continue;
 }
 
-// [...] In addition, if the argument is the name or address of a
-// set of overloaded functions and/or function templates, its
-// associated classes and namespaces are the union of those
-// associated with each of the members of the set: the namespace
-// in which the function or function template is defined and the
-// classes and namespaces associated with its (non-dependent)
-// parameter types and return type.
 OverloadExpr *OE = OverloadExpr::find(Arg).Expression;
 
+// C++ [basic.lookup.argdep] end of p2:
+//   [...] In addition, if the argument is the name or address of a set of
+//   overloaded functions and/or function templates, its associated classes
+//   and namespaces are the union of those associated with each of the
+//   members of the set, i.e., the classes and namespaces associated with
+//   its parameter types 

[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas updated this revision to Diff 195061.
ztamas added a comment.

Missed to syncronize ReleasNotes with the corresponding doc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60507

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-self-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp
@@ -0,0 +1,434 @@
+// RUN: %check_clang_tidy %s bugprone-unhandled-self-assignment %t
+
+namespace std {
+
+template 
+void swap(T x, T y) {
+}
+
+template 
+T &(T x) {
+}
+
+template 
+class unique_ptr {
+};
+
+template 
+class shared_ptr {
+};
+
+template 
+class weak_ptr {
+};
+
+template 
+class auto_ptr {
+};
+
+} // namespace std
+
+void assert(int expression){};
+
+///
+/// Test cases correctly caught by the check.
+
+class PtrField {
+public:
+  PtrField =(const PtrField );
+
+private:
+  int *p;
+};
+
+PtrField ::operator=(const PtrField ) {
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+  // ...
+  return *this;
+}
+
+// Class with an inline operator definition.
+class InlineDefinition {
+public:
+  InlineDefinition =(const InlineDefinition ) {
+// CHECK-MESSAGES: [[@LINE-1]]:21: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class UniquePtrField {
+public:
+  UniquePtrField =(const UniquePtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::unique_ptr p;
+};
+
+class SharedPtrField {
+public:
+  SharedPtrField =(const SharedPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::shared_ptr p;
+};
+
+class WeakPtrField {
+public:
+  WeakPtrField =(const WeakPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::weak_ptr p;
+};
+
+class AutoPtrField {
+public:
+  AutoPtrField =(const AutoPtrField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:17: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  std::auto_ptr p;
+};
+
+// Class with C array field.
+class CArrayField {
+public:
+  CArrayField =(const CArrayField ) {
+// CHECK-MESSAGES: [[@LINE-1]]:16: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+// ...
+return *this;
+  }
+
+private:
+  int array[256];
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy constructor of another class.
+class CopyConstruct {
+public:
+  CopyConstruct =(const CopyConstruct ) {
+// CHECK-MESSAGES: [[@LINE-1]]:18: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+WeakPtrField a;
+WeakPtrField b(a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+// Make sure to not ignore cases when the operator definition calls
+// a copy assignment operator of another class.
+class AssignOperator {
+public:
+  AssignOperator =(const AssignOperator ) {
+// CHECK-MESSAGES: [[@LINE-1]]:19: warning: operator=() might not handle self-assignment properly [bugprone-unhandled-self-assignment]
+a.operator=(object.a);
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+  WeakPtrField a;
+};
+
+///
+/// Test cases correctly ignored by the check.
+
+// Self-assignment is checked using the equality operator.
+class SelfCheck1 {
+public:
+  SelfCheck1 =(const SelfCheck1 ) {
+if (this == )
+  return *this;
+// ...
+return *this;
+  }
+
+private:
+  int *p;
+};
+
+class SelfCheck2 {
+public:
+  SelfCheck2 =(const SelfCheck2 ) {
+if 

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 195060.
riccibruno marked 3 inline comments as done.

Repository:
  rC Clang

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

https://reviews.llvm.org/D60570

Files:
  
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp
  test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-inline-namespace.cpp
  test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
  test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp

Index: test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
===
--- test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
+++ test/CXX/basic/basic.lookup/basic.lookup.argdep/p4.cpp
@@ -67,3 +67,96 @@
 foo(a, 10); // expected-error {{no matching function for call to 'foo'}}
   }
 }
+
+
+// Check the rules described in p4:
+//  When considering an associated namespace, the lookup is the same as the lookup
+//  performed when the associated namespace is used as a qualifier (6.4.3.2) except that:
+
+//  - Any using-directives in the associated namespace are ignored.
+namespace test_using_directives {
+  namespace M { struct S; }
+  namespace N {
+void f(M::S); // expected-note {{declared here}}
+  }
+  namespace M {
+using namespace N;
+struct S {};
+  }
+  void test() {
+M::S s;
+f(s); // expected-error {{use of undeclared}}
+M::f(s); // ok
+  }
+}
+
+//  - Any namespace-scope friend functions or friend function templates declared in
+//associated classes are visible within their respective namespaces even if
+//they are not visible during an ordinary lookup
+// (Note: For the friend declaration to be visible, the corresponding class must be
+//  included in the set of associated classes. Merely including the namespace in
+//  the set of associated namespaces is not enough.)
+namespace test_friend1 {
+  namespace N {
+struct S;
+struct T {
+  friend void f(S); // #1
+};
+struct S { S(); S(T); };
+  }
+
+  void test() {
+N::S s;
+N::T t;
+f(s); // expected-error {{use of undeclared}}
+f(t); // ok, #1
+  }
+}
+
+// credit: Arthur O’Dwyer
+namespace test_friend2 {
+  struct A {
+struct B {
+struct C {};
+};
+friend void foo(...); // #1
+  };
+
+  struct D {
+friend void foo(...); // #2
+  };
+  template struct E {
+struct F {};
+  };
+
+  template struct G {};
+  template struct H {};
+  template struct I {};
+  struct J { friend void foo(...) {} }; // #3
+
+  void test() {
+A::B::C c;
+foo(c); // #1 is not visible since A is not an associated class
+// expected-error@-1 {{use of undeclared}}
+E::F f;
+foo(f); // #2 is not visible since D is not an associated class
+// expected-error@-1 {{use of undeclared}}
+G > > j;
+foo(j);  // ok, #3.
+  }
+}
+
+//  - All names except those of (possibly overloaded) functions and
+//function templates are ignored.
+namespace test_other_names {
+  namespace N {
+struct S {};
+struct Callable { void operator()(S); };
+static struct Callable Callable;
+  }
+
+  void test() {
+N::S s;
+Callable(s); // expected-error {{use of undeclared}}
+  }
+}
Index: test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
===
--- test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
+++ test/CXX/basic/basic.lookup/basic.lookup.argdep/p3.cpp
@@ -18,3 +18,67 @@
 }
   };
 }
+
+// If X contains [...] then Y is empty.
+// - a declaration of a class member
+namespace test_adl_suppression_by_class_member {
+  namespace N {
+struct T {};
+void f(T); // expected-note {{declared here}}
+  }
+  struct S {
+void f();
+void test() {
+  N::T t;
+  f(t); // expected-error {{too many arguments}}
+}
+  };
+}
+
+// - a block-scope function declaration that is not a using-declaration
+namespace test_adl_suppression_by_block_scope {
+  namespace N {
+struct S {};
+void f(S);
+  }
+  namespace M { void f(int); } // expected-note 2{{candidate}}
+  void test1() {
+N::S s;
+using M::f;
+f(s); // ok
+  }
+
+  void test2() {
+N::S s;
+extern void f(char); // expected-note {{passing argument to parameter here}}
+f(s); // expected-error {{no viable conversion from 'N::S' to 'char'}}
+  }
+
+  void test3() {
+N::S s;
+extern void f(char); // expected-note {{candidate}}
+using M::f;
+f(s); // expected-error {{no matching function}}
+  }
+
+  void test4() {
+N::S s;
+using M::f;
+extern void f(char); // expected-note {{candidate}}
+f(s); // expected-error {{no matching function}}
+  }
+
+}
+
+// - a declaration that is neither a function nor a function template
+namespace test_adl_suppression_by_non_function {
+  namespace N {
+struct S {};
+void f(S);
+  }
+  void test() {
+extern void (*f)();
+N::S s;
+f(s); // 

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked 11 inline comments as done.
riccibruno added a comment.

In D60570#1465478 , @Quuxplusone wrote:

> As you're making tests for ADL corner cases, you might also consider testing 
> the interactions between ADL and defaulted function parameters, e.g. 
> https://godbolt.org/z/vHnyFl
>  It looks like everyone (except MSVC) already gets that stuff right (or at 
> least portable-between-the-big-three). I bet the behavior naturally falls out 
> of some other rules; you might say "there's no way that could possibly break, 
> so we don't need to test it," and I'd accept that.


I think that as you say this falls out of the other rules.




Comment at: 
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:77
+
+  // associated class: itself, lambda
+  namespace X5 {

Quuxplusone wrote:
> Do you also have a test somewhere to verify that the parameter and return 
> types of a lambda's `operator()` do not contribute to the associated types of 
> the lambda type itself? That is,
> ```
> // https://godbolt.org/z/g_oMOA
> namespace N {
> struct A {};
> template constexpr int f(T) { return 1; }
> }
> 
> constexpr int f(N::A (*)()) { return 2; }
> constexpr int f(void (*)(N::A)) { return 3; }
> 
> void test() {
> constexpr auto lambda = []() -> N::A { return {}; };
> static_assert(f(lambda) == 2);
> 
> constexpr auto lambda2 = [](N::A) {};
> static_assert(f(lambda2) == 3);
> }
> ```
> Clang does handle this correctly; I'm just asking for it to be tested, if 
> it's not already. (I might have overlooked an existing test.)
I am not sure, but I see no harm in adding it here.



Comment at: 
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:144
+
+  // template template argument
+  namespace X3 {

Quuxplusone wrote:
> I think for completeness there should be a "negative test" for non-type 
> template arguments:
> ```
>   namespace X4 {
> template  struct C {};
> namespace N {
>   struct Z {
>   enum E { E0 };
>   void X4_f(C);
>   };
>   enum E { E0 };
>   void X4_g(C);
> }
>   }
>   void test4() {
> X4::C c1;
> X4::C c2;
> X4_f(c1); // expected-error{{undeclared identifier 'X4_f'}}
> X4_g(c2); // expected-error{{undeclared identifier 'X4_g'}}
>   }
> ```
> In C++2a, user-defined NTTPs will become possible, so we'll want another test 
> for something like
> ```
>   // https://godbolt.org/z/MfWG8C
>   namespace X4 {
> template struct C {};
> namespace N {
>   struct Z {
> int i;
> constexpr Z(int i): i(i) {}
> auto operator<=>(const Z&) const = default;
>   };
>   void X4_f(C);
> }
>   }
>   void test4() {
> X4::C c1;
> X4_f(c1); // expected-error{{undeclared identifier 'X4_f'}}
>   }
> ```
Isn't that already covered by the test in 
`adl_class_template_specialization_type::X1` ? I agree that a test for class 
type NTTP will be needed in C++2a, but for now this feature is not yet 
implemented.



Comment at: 
test/CXX/basic/basic.lookup/basic.lookup.argdep/p2-associated-namespaces-classes.cpp:304
+static_assert(f(g3) == 4, "");// FIXME: Also well-formed from the 
union rule.
+  // expected-error@-1 {{use of 
undeclared}}
+  }

Quuxplusone wrote:
> riccibruno wrote:
> > Quuxplusone wrote:
> > > I see how `g3` matches the example in CWG997
> > > http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#997
> > > However, I don't see how CWG997's resolution actually affected this 
> > > example in the slightest. The wording inserted for CWG997 was, 
> > > "Additionally, if the aforementioned set of overloaded functions is named 
> > > with a template-id, its associated classes and namespaces are those of 
> > > its type template-arguments and its template template-arguments." That 
> > > makes e.g.
> > > 
> > > f(g3)
> > > 
> > > consider `N::f`, because `N::S` is a "type template-argument" of the 
> > > template-id `g3` which names the set of overloaded functions.  But 
> > > it doesn't do anything at all to `f(g3)` because `g3` is not a 
> > > template-id and doesn't have any template-arguments.
> > > 
> > > This piece of ADL is implemented only by GCC (not EDG, Clang, or MSVC), 
> > > and personally I would very much like to keep it that way. We know 
> > > there's no real-world code that expects or relies on CWG997 — because 
> > > such code would never work in practice except on GCC. Let's keep it that 
> > > way!  As soon as you implement a crazy arcane rule, such that code 
> > > _could_ portably rely on it, code _will start_ relying on it... and then 
> > > we'll never be able to simplify the language.
> > > Case in point: the piece of ADL described in this blog post --
> > > 

[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

[Tamás Zolnai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358356: [clang-tidy] Add MagnitudeBitsUpperLimit option to  
bugprone-too-small-loop… (authored by ztamas, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59870?vs=194261=195058#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59870

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
  
clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable-magniute-bits-upper-limit.cpp
  clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable-magniute-bits-upper-limit.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable-magniute-bits-upper-limit.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable-magniute-bits-upper-limit.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t -- -- --target=x86_64-linux
+
+// MagnitudeBitsUpperLimit = 16 (default value)
+
+unsigned long size() { return 294967296l; }
+
+void voidFilteredOutForLoop1() {
+  for (long i = 0; i < size(); ++i) {
+// no warning
+  }
+}
+
+void voidCaughtForLoop1() {
+  for (int i = 0; i < size(); ++i) {
+// no warning
+  }
+}
+
+void voidCaughtForLoop2() {
+  for (short i = 0; i < size(); ++i) {
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: loop variable has narrower type 'short' than iteration's upper bound 'unsigned long' [bugprone-too-small-loop-variable]
+  }
+}
Index: clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable.cpp
@@ -1,4 +1,8 @@
-// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t -- -- --target=x86_64-linux
+// RUN: %check_clang_tidy %s bugprone-too-small-loop-variable %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: bugprone-too-small-loop-variable.MagnitudeBitsUpperLimit, \
+// RUN:   value: 1024}]}" \
+// RUN:   -- --target=x86_64-linux
 
 long size() { return 294967296l; }
 
Index: clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
@@ -27,6 +27,17 @@
 static constexpr llvm::StringLiteral LoopIncrementName =
 llvm::StringLiteral("loopIncrement");
 
+TooSmallLoopVariableCheck::TooSmallLoopVariableCheck(StringRef Name,
+ ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  MagnitudeBitsUpperLimit(Options.get(
+  "MagnitudeBitsUpperLimit", 16)) {}
+
+void TooSmallLoopVariableCheck::storeOptions(
+ClangTidyOptions::OptionMap ) {
+  Options.store(Opts, "MagnitudeBitsUpperLimit", MagnitudeBitsUpperLimit);
+}
+
 /// \brief The matcher for loops with suspicious integer loop variable.
 ///
 /// In this general example, assuming 'j' and 'k' are of integral type:
@@ -84,9 +95,9 @@
   this);
 }
 
-/// Returns the positive part of the integer width for an integer type.
-static unsigned calcPositiveBits(const ASTContext ,
- const QualType ) {
+/// Returns the magnitude bits of an integer type.
+static unsigned calcMagnitudeBits(const ASTContext ,
+  const QualType ) {
   assert(IntExprType->isIntegerType());
 
   return IntExprType->isUnsignedIntegerType()
@@ -94,13 +105,13 @@
  : Context.getIntWidth(IntExprType) - 1;
 }
 
-/// \brief Calculate the upper bound expression's positive bits, but ignore
+/// \brief Calculate the upper bound expression's magnitude bits, but ignore
 /// constant like values to reduce false positives.
-static unsigned calcUpperBoundPositiveBits(const ASTContext ,
-   const Expr *UpperBound,
-   const QualType ) {
+static unsigned calcUpperBoundMagnitudeBits(const ASTContext ,
+const Expr *UpperBound,
+const QualType ) {
   // Ignore casting caused by constant values inside a binary operator.
-  

[clang-tools-extra] r358356 - [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

[Tamas Zolnai via cfe-commits]

Author: ztamas
Date: Sun Apr 14 05:47:48 2019
New Revision: 358356

URL: http://llvm.org/viewvc/llvm-project?rev=358356=rev
Log:
[clang-tidy] Add MagnitudeBitsUpperLimit option to  
bugprone-too-small-loop-variable

Summary:
The bugprone-too-small-loop-variable check often catches loop variables which 
can represent "big enough" values, so we don't actually need to worry about 
that this variable will overflow in a loop when the code iterates through a 
container. For example a 32 bit signed integer type's maximum value is 2 147 
483 647 and a container's size won't reach this maximum value in most of the 
cases.
So the idea of this option to allow the user to specify an upper limit (using 
magnitude bit of the integer type) to filter out those catches which are not 
interesting for the user, so he/she can focus on the more risky integer 
incompatibilities.
Next to the option I replaced the term "positive bits" to "magnitude bits" 
which seems a better naming both in the code and in the name of the new option.

Reviewers: JonasToth, alexfh, aaron.ballman, hokein

Reviewed By: JonasToth

Subscribers: Eugene.Zelenko, xazax.hun, jdoerfert, cfe-commits

Tags: #clang-tools-extra, #clang

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

Added:

clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable-magniute-bits-upper-limit.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst

clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
clang-tools-extra/trunk/test/clang-tidy/bugprone-too-small-loop-variable.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp?rev=358356=358355=358356=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp 
Sun Apr 14 05:47:48 2019
@@ -27,6 +27,17 @@ static constexpr llvm::StringLiteral Loo
 static constexpr llvm::StringLiteral LoopIncrementName =
 llvm::StringLiteral("loopIncrement");
 
+TooSmallLoopVariableCheck::TooSmallLoopVariableCheck(StringRef Name,
+ ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  MagnitudeBitsUpperLimit(Options.get(
+  "MagnitudeBitsUpperLimit", 16)) {}
+
+void TooSmallLoopVariableCheck::storeOptions(
+ClangTidyOptions::OptionMap ) {
+  Options.store(Opts, "MagnitudeBitsUpperLimit", MagnitudeBitsUpperLimit);
+}
+
 /// \brief The matcher for loops with suspicious integer loop variable.
 ///
 /// In this general example, assuming 'j' and 'k' are of integral type:
@@ -84,9 +95,9 @@ void TooSmallLoopVariableCheck::register
   this);
 }
 
-/// Returns the positive part of the integer width for an integer type.
-static unsigned calcPositiveBits(const ASTContext ,
- const QualType ) {
+/// Returns the magnitude bits of an integer type.
+static unsigned calcMagnitudeBits(const ASTContext ,
+  const QualType ) {
   assert(IntExprType->isIntegerType());
 
   return IntExprType->isUnsignedIntegerType()
@@ -94,13 +105,13 @@ static unsigned calcPositiveBits(const A
  : Context.getIntWidth(IntExprType) - 1;
 }
 
-/// \brief Calculate the upper bound expression's positive bits, but ignore
+/// \brief Calculate the upper bound expression's magnitude bits, but ignore
 /// constant like values to reduce false positives.
-static unsigned calcUpperBoundPositiveBits(const ASTContext ,
-   const Expr *UpperBound,
-   const QualType ) {
+static unsigned calcUpperBoundMagnitudeBits(const ASTContext ,
+const Expr *UpperBound,
+const QualType ) {
   // Ignore casting caused by constant values inside a binary operator.
-  // We are interested in variable values' positive bits.
+  // We are interested in variable values' magnitude bits.
   if (const auto *BinOperator = dyn_cast(UpperBound)) {
 const Expr *RHSE = BinOperator->getRHS()->IgnoreParenImpCasts();
 const Expr *LHSE = BinOperator->getLHS()->IgnoreParenImpCasts();
@@ -122,15 +133,15 @@ static unsigned calcUpperBoundPositiveBi
 if (RHSEIsConstantValue && LHSEIsConstantValue)
   return 0;
 if (RHSEIsConstantValue)
-  return calcPositiveBits(Context, LHSEType);
+  return calcMagnitudeBits(Context, LHSEType);
 if (LHSEIsConstantValue)
-  return calcPositiveBits(Context, RHSEType);
+  return 

[PATCH] D60363: [clang-format] [PR41170] Break after return type ignored with certain comments positions

[MyDeveloperDay via Phabricator via cfe-commits]

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

use endsWith() as it ignored comments


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

https://reviews.llvm.org/D60363

Files:
  clang/lib/Format/TokenAnnotator.h
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5768,6 +5768,26 @@
"  return a;\n"
"}\n",
Style);
+
+  Style = getGNUStyle();
+
+  // Test for comments at the end of function declarations.
+  verifyFormat("void\n"
+   "foo (int a, /*abc*/ int b) // def\n"
+   "{\n"
+   "}\n",
+   Style);
+
+  verifyFormat("void\n"
+   "foo (int a, /* abc */ int b) /* def */\n"
+   "{\n"
+   "}\n",
+   Style);
+
+  // Definitions that should not break after return type
+  verifyFormat("void foo (int a, int b); // def\n", Style);
+  verifyFormat("void foo (int a, int b); /* def */\n", Style);
+  verifyFormat("void foo (int a, int b);\n", Style);
 }
 
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
Index: clang/lib/Format/TokenAnnotator.h
===
--- clang/lib/Format/TokenAnnotator.h
+++ clang/lib/Format/TokenAnnotator.h
@@ -99,9 +99,17 @@
   /// function declaration. Asserts MightBeFunctionDecl.
   bool mightBeFunctionDefinition() const {
 assert(MightBeFunctionDecl);
-// FIXME: Line.Last points to other characters than tok::semi
-// and tok::lbrace.
-return !Last->isOneOf(tok::semi, tok::comment);
+// Try to determine if the end of a stream of tokens is either the
+// Definition or the Declaration for a function. It does this by looking 
for
+// the ';' in foo(); and using that it ends with a ; to know this is the
+// Definition, however the line could end with
+//foo(); /* comment */
+// or
+//foo(); // comment
+// or
+//foo() // comment
+// endsWith() ignores the comment.
+return !endsWith(tok::semi);
   }
 
   /// \c true if this line starts a namespace definition.


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -5768,6 +5768,26 @@
"  return a;\n"
"}\n",
Style);
+
+  Style = getGNUStyle();
+
+  // Test for comments at the end of function declarations.
+  verifyFormat("void\n"
+   "foo (int a, /*abc*/ int b) // def\n"
+   "{\n"
+   "}\n",
+   Style);
+
+  verifyFormat("void\n"
+   "foo (int a, /* abc */ int b) /* def */\n"
+   "{\n"
+   "}\n",
+   Style);
+
+  // Definitions that should not break after return type
+  verifyFormat("void foo (int a, int b); // def\n", Style);
+  verifyFormat("void foo (int a, int b); /* def */\n", Style);
+  verifyFormat("void foo (int a, int b);\n", Style);
 }
 
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
Index: clang/lib/Format/TokenAnnotator.h
===
--- clang/lib/Format/TokenAnnotator.h
+++ clang/lib/Format/TokenAnnotator.h
@@ -99,9 +99,17 @@
   /// function declaration. Asserts MightBeFunctionDecl.
   bool mightBeFunctionDefinition() const {
 assert(MightBeFunctionDecl);
-// FIXME: Line.Last points to other characters than tok::semi
-// and tok::lbrace.
-return !Last->isOneOf(tok::semi, tok::comment);
+// Try to determine if the end of a stream of tokens is either the
+// Definition or the Declaration for a function. It does this by looking for
+// the ';' in foo(); and using that it ends with a ; to know this is the
+// Definition, however the line could end with
+//foo(); /* comment */
+// or
+//foo(); // comment
+// or
+//foo() // comment
+// endsWith() ignores the comment.
+return !endsWith(tok::semi);
   }
 
   /// \c true if this line starts a namespace definition.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60561: [clang] fixing diagnostics of constexpr callstack

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 195056.

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

https://reviews.llvm.org/D60561

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1159,3 +1159,32 @@
 enum class InEnum3 {
   THREE = indirect_builtin_constant_p("abc")
 };
+
+constexpr int f1(int i, int b) {
+  i += 4;
+  b -= 4;
+  // expected-note@+1 {{negative shift count -7}}
+  return i << b;
+}
+
+constexpr int f2(int i) {
+  int b = 0;
+  i += 1;
+  b -= 1;
+  // expected-note@+1 {{in call to 'f1(3, -3)'}}
+  return f1(i + 2, b -= 2);
+}
+
+constexpr int f(int i) {
+  i -= 1;
+  //expected-note@+1 {{negative shift count -1}}
+  return 1 << i;
+}
+
+// expected-error@+2 {{constant expression}}
+// expected-note@+1 {{in call to 'f2(0)'}}
+constexpr int val = f2(0);
+
+// expected-error@+2 {{constant expression}}
+// expected-note@+1 {{in call to 'f(0)'}}
+static_assert(f(0), "");
\ No newline at end of file
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -661,6 +661,10 @@
 /// CurrentCall - The top of the constexpr call stack.
 CallStackFrame *CurrentCall;
 
+/// ArgumentCallStack - Used to store Arguments to function calls
+/// only required if diagnostics should produce a callstack
+SmallVectorImpl *ArgumentCallStack;
+
 /// CallStackDepth - The number of calls in the call stack right now.
 unsigned CallStackDepth;
 
@@ -789,14 +793,14 @@
 bool checkingForOverflow() { return EvalMode == EM_EvaluateForOverflow; }
 
 EvalInfo(const ASTContext , Expr::EvalStatus , EvaluationMode Mode)
-  : Ctx(const_cast(C)), EvalStatus(S), CurrentCall(nullptr),
-CallStackDepth(0), NextCallIndex(1),
-StepsLeft(getLangOpts().ConstexprStepLimit),
-BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
-EvaluatingDecl((const ValueDecl *)nullptr),
-EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
-HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
-InConstantContext(false), EvalMode(Mode) {}
+: Ctx(const_cast(C)), EvalStatus(S), CurrentCall(nullptr),
+  ArgumentCallStack(nullptr), CallStackDepth(0), NextCallIndex(1),
+  StepsLeft(getLangOpts().ConstexprStepLimit),
+  BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
+  EvaluatingDecl((const ValueDecl *)nullptr),
+  EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
+  HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
+  InConstantContext(false), EvalMode(Mode) {}
 
 void setEvaluatingDecl(APValue::LValueBase Base, APValue ) {
   EvaluatingDecl = Base;
@@ -1240,10 +1244,20 @@
   Arguments(Arguments), CallLoc(CallLoc), Index(Info.NextCallIndex++) {
   Info.CurrentCall = this;
   ++Info.CallStackDepth;
+  if (Info.ArgumentCallStack) {
+Info.ArgumentCallStack->reserve(Info.ArgumentCallStack->size() +
+Callee->param_size());
+std::copy(Arguments, Arguments + Callee->param_size(),
+  std::back_inserter(*Info.ArgumentCallStack));
+  }
 }
 
 CallStackFrame::~CallStackFrame() {
   assert(Info.CurrentCall == this && "calls retired out of order");
+  if (Info.ArgumentCallStack && Caller) {
+Info.ArgumentCallStack->resize(Info.ArgumentCallStack->size() -
+   Callee->param_size());
+  }
   --Info.CallStackDepth;
   Info.CurrentCall = Caller;
 }
@@ -1257,9 +1271,12 @@
   return Result;
 }
 
-static void describeCall(CallStackFrame *Frame, raw_ostream );
+static void describeCall(CallStackFrame *Frame, raw_ostream ,
+ SmallVectorImpl *ArgumentCallStack,
+ unsigned Pos);
 
 void EvalInfo::addCallStack(unsigned Limit) {
+  assert(ArgumentCallStack && "needed to produce a call stack");
   // Determine which calls to skip, if any.
   unsigned ActiveCalls = CallStackDepth - 1;
   unsigned SkipStart = ActiveCalls, SkipEnd = SkipStart;
@@ -1270,8 +1287,10 @@
 
   // Walk the call stack and add the diagnostics.
   unsigned CallIdx = 0;
+  unsigned Pos = ArgumentCallStack->size();
   for (CallStackFrame *Frame = CurrentCall; Frame != 
Frame = Frame->Caller, ++CallIdx) {
+Pos -= Frame->Callee->param_size();
 // Skip this call?
 if (CallIdx >= SkipStart && CallIdx < SkipEnd) {
   if (CallIdx == SkipStart) {
@@ -1294,7 +1313,7 @@
 
 SmallVector Buffer;
 llvm::raw_svector_ostream Out(Buffer);
-describeCall(Frame, Out);
+describeCall(Frame, Out, ArgumentCallStack, Pos);
   

[PATCH] D60561: [clang] fixing diagnostics of constexpr callstack

[Gauthier via Phabricator via cfe-commits]

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

i changed the way arguments are stored. to make it more controllable.
added an argument call stack where it is needed.
this version slows down compilation by around 0.5% in average over 200 run for 
SemaDecl -fsyntax-only


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

https://reviews.llvm.org/D60561

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1159,3 +1159,32 @@
 enum class InEnum3 {
   THREE = indirect_builtin_constant_p("abc")
 };
+
+constexpr int f1(int i, int b) {
+  i += 4;
+  b -= 4;
+  // expected-note@+1 {{negative shift count -7}}
+  return i << b;
+}
+
+constexpr int f2(int i) {
+  int b = 0;
+  i += 1;
+  b -= 1;
+  // expected-note@+1 {{in call to 'f1(3, -3)'}}
+  return f1(i + 2, b -= 2);
+}
+
+constexpr int f(int i) {
+  i -= 1;
+  //expected-note@+1 {{negative shift count -1}}
+  return 1 << i;
+}
+
+// expected-error@+2 {{constant expression}}
+// expected-note@+1 {{in call to 'f2(0)'}}
+constexpr int val = f2(0);
+
+// expected-error@+2 {{constant expression}}
+// expected-note@+1 {{in call to 'f(0)'}}
+static_assert(f(0), "");
\ No newline at end of file
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -661,6 +661,10 @@
 /// CurrentCall - The top of the constexpr call stack.
 CallStackFrame *CurrentCall;
 
+/// ArgumentCallStack - Used to store Arguments to function calls
+/// only required if diagnostics should produce a callstack
+SmallVectorImpl *ArgumentCallStack;
+
 /// CallStackDepth - The number of calls in the call stack right now.
 unsigned CallStackDepth;
 
@@ -789,14 +793,14 @@
 bool checkingForOverflow() { return EvalMode == EM_EvaluateForOverflow; }
 
 EvalInfo(const ASTContext , Expr::EvalStatus , EvaluationMode Mode)
-  : Ctx(const_cast(C)), EvalStatus(S), CurrentCall(nullptr),
-CallStackDepth(0), NextCallIndex(1),
-StepsLeft(getLangOpts().ConstexprStepLimit),
-BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
-EvaluatingDecl((const ValueDecl *)nullptr),
-EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
-HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
-InConstantContext(false), EvalMode(Mode) {}
+: Ctx(const_cast(C)), EvalStatus(S), CurrentCall(nullptr),
+  ArgumentCallStack(nullptr), CallStackDepth(0), NextCallIndex(1),
+  StepsLeft(getLangOpts().ConstexprStepLimit),
+  BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
+  EvaluatingDecl((const ValueDecl *)nullptr),
+  EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
+  HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
+  InConstantContext(false), EvalMode(Mode) {}
 
 void setEvaluatingDecl(APValue::LValueBase Base, APValue ) {
   EvaluatingDecl = Base;
@@ -1240,10 +1244,20 @@
   Arguments(Arguments), CallLoc(CallLoc), Index(Info.NextCallIndex++) {
   Info.CurrentCall = this;
   ++Info.CallStackDepth;
+  if (Info.ArgumentCallStack) {
+Info.ArgumentCallStack->reserve(Info.ArgumentCallStack->size() +
+Callee->param_size());
+std::copy(Arguments, Arguments + Callee->param_size(),
+  std::back_inserter(*Info.ArgumentCallStack));
+  }
 }
 
 CallStackFrame::~CallStackFrame() {
   assert(Info.CurrentCall == this && "calls retired out of order");
+  if (Info.ArgumentCallStack && Caller) {
+Info.ArgumentCallStack->resize(Info.ArgumentCallStack->size() -
+   Callee->param_size());
+  }
   --Info.CallStackDepth;
   Info.CurrentCall = Caller;
 }
@@ -1257,9 +1271,12 @@
   return Result;
 }
 
-static void describeCall(CallStackFrame *Frame, raw_ostream );
+static void describeCall(CallStackFrame *Frame, raw_ostream ,
+ SmallVectorImpl *ArgumentCallStack,
+ unsigned );
 
 void EvalInfo::addCallStack(unsigned Limit) {
+  assert(ArgumentCallStack && "needed to produce a call stack");
   // Determine which calls to skip, if any.
   unsigned ActiveCalls = CallStackDepth - 1;
   unsigned SkipStart = ActiveCalls, SkipEnd = SkipStart;
@@ -1270,8 +1287,10 @@
 
   // Walk the call stack and add the diagnostics.
   unsigned CallIdx = 0;
+  unsigned Pos = ArgumentCallStack->size();
   for (CallStackFrame *Frame = CurrentCall; Frame != 
Frame = Frame->Caller, ++CallIdx) {
+Pos -= Frame->Callee->param_size();
 // Skip this call?
 if (CallIdx >= 

[PATCH] D60663: Time profiler: small fixes and optimizations

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Looks good ignoring the json bits.

Re license:

> https://reviews.llvm.org/D58675
>  This is the first part of time tracing system, I have splitted them cause 
> this part is mostly written by Aras Pranckevicius except of several minor 
> fixes concerning formatting.

So i can't and won't claim any legal knowledge, but it maybe would be good for 
him to at least comment here, that he is ok with this?




Comment at: llvm/lib/Support/TimeProfiler.cpp:20
 #include 
 #include 
 #include 

Unused header?



Comment at: llvm/lib/Support/TimeProfiler.cpp:37-38
   DurationType Duration;
   std::string Name;
   std::string Detail;
 };

Ah yes, they are allocated when created. Hmm.



Comment at: llvm/lib/Support/TimeProfiler.cpp:47-48
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
+Entry E = {steady_clock::now(), {}, std::move(Name), Detail()};
 Stack.push_back(std::move(E));
   }

Does
```
Stack.emplace_back(steady_clock::now(), {}, std::move(Name), Detail());
```
not work?
(also, the `std::string` returned from `Detail` function invocation is moved?)



Comment at: llvm/lib/Support/TimeProfiler.cpp:57
 // Only include sections longer than 500us.
 if (duration_cast(E.Duration).count() > 500)
   Entries.emplace_back(E);

I feel like this should be 
```
static cl::opt TimeProfileGranularity(
"time-profile-granularity",
cl::desc(""),
cl::init(500));
```
?



Comment at: llvm/lib/Support/TimeProfiler.cpp:65
 // itself.
 if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry ) {
   return Val.Name == E.Name;

Yes, good point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60663



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D58675#1465706 , @anton-afanasyev 
wrote:

> In D58675#1465336 , @lebedev.ri 
> wrote:
>
> > Some post-commit review (please submit a new review, don't replace this 
> > diff)
> >  As usual, the incorrect license headers keep slipping through.
>
>
> Ok, I've made a separate review for this: https://reviews.llvm.org/D60663


Thank you.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58675



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

[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev marked 23 inline comments as done.
anton-afanasyev added a comment.

In D58675#1465336 , @lebedev.ri wrote:

> Some post-commit review (please submit a new review, don't replace this diff)
>  As usual, the incorrect license headers keep slipping through.


Ok, I've made a separate review for this: https://reviews.llvm.org/D60663




Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

lebedev.ri wrote:
> OOOPS, wrong license.
Yes, thanks.



Comment at: llvm/trunk/include/llvm/Support/TimeProfiler.h:53
+/// is not initialized, the overhead is a single branch.
+struct TimeTraceScope {
+  TimeTraceScope(StringRef Name, StringRef Detail) {

lebedev.ri wrote:
> Did you mean to explicitly prohibit all the default constructors / 
> `operator=`?
Ok, I'm to add lines like `TimeTraceScope() = delete;` here.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:5-6
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

lebedev.ri wrote:
> Wrong license.
Yes, I'm to fix it.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:29-30
+
+static std::string escapeString(StringRef Src) {
+  std::string OS;
+  for (const unsigned char  : Src) {

lebedev.ri wrote:
> lebedev.ri wrote:
> > `SmallString<32>` ?
> > Also, it is safe to `OS.reserve(Src.size())`
> Also, you probably want to add `raw_svector_ostream` ontop, and `<<` into it.
This function has been already dropped here https://reviews.llvm.org/D60609 . 
I'm to submit code without it if json library using are ok for performance.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:59-60
+  DurationType Duration;
+  std::string Name;
+  std::string Detail;
+};

lebedev.ri wrote:
> `SmallString<32>`?
Hmm, would it make a sense for `Detail`? `Entry`s are heap-allocated, the 
actual size is ranging from several bytes to several hundreds. Also, 
getQualifiedNameAsString() which is usually used for `Detail` returns 
std::string.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:70-71
+
+  void begin(std::string Name, llvm::function_ref Detail) {
+Entry E = {steady_clock::now(), {}, Name, Detail()};
+Stack.push_back(std::move(E));

lebedev.ri wrote:
> Why not either take `StringRef` arg, or at least `std::move()` it when 
> creating `Entry`?
Do you mean `std::move(Name)`? Ok.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:89
+// itself.
+if (std::find_if(++Stack.rbegin(), Stack.rend(), [&](const Entry ) {
+  return Val.Name == E.Name;

lebedev.ri wrote:
> llvm::find_if(llvm::reverse(), ) 
Hmm, one need not `[rbegin(), rend()]`, but `[rbegin()++,rend()]`, so have to 
explicitly specify begin and end, `llvm::reverse(Stack)` is inappropriate here.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:99
+
+  void Write(std::unique_ptr ) {
+assert(Stack.empty() &&

lebedev.ri wrote:
> Why pass `std::unique_ptr` ?
> Just `raw_pwrite_stream `
Yes, thanks! 
This was blindly copied from `CompilerInstance::createOutputFile()` return type.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:122
+  SortedTotals.push_back(E);
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),

lebedev.ri wrote:
> Elide `{}`
Ok, thanks.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:123
+}
+std::sort(SortedTotals.begin(), SortedTotals.end(),
+  [](const NameAndDuration , const NameAndDuration ) {

lebedev.ri wrote:
> llvm::sort <- this is a correctness issue.
Yes, thanks!



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:144-145
+
+  std::vector Stack;
+  std::vector Entries;
+  std::unordered_map TotalPerName;

lebedev.ri wrote:
> Would it make sense to make these `SmallVector`?
Ok, I'm to change it to
```
  SmallVector Stack;  
  SmallVector Entries;
```
`Stack` size may be enough for small sources while `Entries` usually amounts 
many thousands of `Entry`s.



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:146-147
+  std::vector Entries;
+  std::unordered_map TotalPerName;
+  std::unordered_map CountPerName;
+  time_point StartTime;

lebedev.ri wrote:
> 1. Eww, `std::unordered_map`, that will have *horrible* perf.
> 2. Eww, map with key = string. Use `llvm::StringMap`
You are right, but this is already fixed and submitted in this follow-up: 
https://reviews.llvm.org/D60404



Comment at: llvm/trunk/lib/Support/TimeProfiler.cpp:162
+
+void 

[PATCH] D60663: Time profiler: small fixes and optimizations

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev created this revision.
anton-afanasyev added a reviewer: lebedev.ri.
Herald added subscribers: llvm-commits, cfe-commits, mgrang, hiraditya.
Herald added projects: clang, LLVM.

Fixes from Roman's review here: https://reviews.llvm.org/D58675#1465336


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60663

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/include/llvm/Support/TimeProfiler.h
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -1,13 +1,12 @@
 //===-- TimeProfiler.cpp - Hierarchical Time Profiler -===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 //
-/// \file Hierarchical time profiler implementation.
+// This file implements hierarchical time profiler.
 //
 //===--===//
 
@@ -41,13 +40,11 @@
 
 struct TimeTraceProfiler {
   TimeTraceProfiler() {
-Stack.reserve(8);
-Entries.reserve(128);
 StartTime = steady_clock::now();
   }
 
   void begin(std::string Name, llvm::function_ref Detail) {
-Entry E = {steady_clock::now(), {}, Name, Detail()};
+Entry E = {steady_clock::now(), {}, std::move(Name), Detail()};
 Stack.push_back(std::move(E));
   }
 
@@ -76,7 +73,7 @@
 Stack.pop_back();
   }
 
-  void Write(std::unique_ptr ) {
+  void Write(raw_pwrite_stream ) {
 assert(Stack.empty() &&
"All profiler sections should be ended when calling Write");
 
@@ -103,14 +100,14 @@
 int Tid = 1;
 std::vector SortedTotals;
 SortedTotals.reserve(CountAndTotalPerName.size());
-for (const auto  : CountAndTotalPerName) {
+for (const auto  : CountAndTotalPerName)
   SortedTotals.emplace_back(E.getKey(), E.getValue());
-}
-std::sort(SortedTotals.begin(), SortedTotals.end(),
-  [](const NameAndCountAndDurationType ,
- const NameAndCountAndDurationType ) {
-return A.second.second > B.second.second;
-  });
+
+llvm::sort(SortedTotals.begin(), SortedTotals.end(),
+   [](const NameAndCountAndDurationType ,
+  const NameAndCountAndDurationType ) {
+ return A.second.second > B.second.second;
+   });
 for (const auto  : SortedTotals) {
   auto DurUs = duration_cast(E.second.second).count();
   auto Count = CountAndTotalPerName[E.first].first;
@@ -140,12 +137,12 @@
 {"args", json::Object{{"name", "clang"}}},
 });
 
-*OS << formatv("{0:2}", json::Value(json::Object(
-{{"traceEvents", std::move(Events)}})));
+OS << formatv("{0:2}", json::Value(json::Object(
+   {{"traceEvents", std::move(Events)}})));
   }
 
-  std::vector Stack;
-  std::vector Entries;
+  SmallVector Stack;
+  SmallVector Entries;
   StringMap CountAndTotalPerName;
   time_point StartTime;
 };
@@ -161,7 +158,7 @@
   TimeTraceProfilerInstance = nullptr;
 }
 
-void timeTraceProfilerWrite(std::unique_ptr ) {
+void timeTraceProfilerWrite(raw_pwrite_stream ) {
   assert(TimeTraceProfilerInstance != nullptr &&
  "Profiler object can't be null");
   TimeTraceProfilerInstance->Write(OS);
Index: llvm/include/llvm/Support/TimeProfiler.h
===
--- llvm/include/llvm/Support/TimeProfiler.h
+++ llvm/include/llvm/Support/TimeProfiler.h
@@ -1,9 +1,8 @@
 //===- llvm/Support/TimeProfiler.h - Hierarchical Time Profiler -*- C++ -*-===//
 //
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
 
@@ -33,7 +32,7 @@
 /// Write profiling data to output file.
 /// Data produced is JSON, in Chrome "Trace Event" format, see
 /// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview
-void timeTraceProfilerWrite(std::unique_ptr );
+void timeTraceProfilerWrite(raw_pwrite_stream );
 
 /// Manually begin a time section, with the given \p Name and \p Detail.
 /// Profiler copies the string data, so the 

r358355 - [c++20] Enable driver and frontend support for building and using

[Richard Smith via cfe-commits]

Author: rsmith
Date: Sun Apr 14 04:11:37 2019
New Revision: 358355

URL: http://llvm.org/viewvc/llvm-project?rev=358355=rev
Log:
[c++20] Enable driver and frontend support for building and using
modules when -std=c++2a is specified.

Added:
cfe/trunk/test/CXX/module/module.unit/p8.cpp
cfe/trunk/test/Driver/modules.cpp
Modified:
cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Frontend/FrontendActions.cpp
cfe/trunk/lib/Parse/Parser.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td?rev=358355=358354=358355=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td Sun Apr 14 
04:11:37 2019
@@ -173,10 +173,11 @@ def note_incompatible_analyzer_plugin_ap
 
 def err_module_build_requires_fmodules : Error<
   "module compilation requires '-fmodules'">;
-def err_module_interface_requires_modules_ts : Error<
-  "module interface compilation requires '-fmodules-ts'">;
+def err_module_interface_requires_cpp_modules : Error<
+  "module interface compilation requires '-std=c++2a' or '-fmodules-ts'">;
 def err_header_module_requires_modules : Error<
-  "header module compilation requires '-fmodules' or '-fmodules-ts'">;
+  "header module compilation requires '-fmodules', '-std=c++2a', or "
+  "'-fmodules-ts'">;
 def warn_module_config_mismatch : Warning<
   "module file %0 cannot be loaded due to a configuration mismatch with the 
current "
   "compilation">, InGroup>, 
DefaultError;

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=358355=358354=358355=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Sun Apr 14 04:11:37 2019
@@ -2721,7 +2721,7 @@ static void RenderModulesOptions(Compila
 }
   }
 
-  HaveModules = HaveClangModules;
+  HaveModules |= HaveClangModules;
   if (Args.hasArg(options::OPT_fmodules_ts)) {
 CmdArgs.push_back("-fmodules-ts");
 HaveModules = true;
@@ -4259,7 +4259,8 @@ void Clang::ConstructJob(Compilation ,
   // If a std is supplied, only add -trigraphs if it follows the
   // option.
   bool ImplyVCPPCXXVer = false;
-  if (Arg *Std = Args.getLastArg(options::OPT_std_EQ, options::OPT_ansi)) {
+  const Arg *Std = Args.getLastArg(options::OPT_std_EQ, options::OPT_ansi);
+  if (Std) {
 if (Std->getOption().matches(options::OPT_ansi))
   if (types::isCXX(InputType))
 CmdArgs.push_back("-std=c++98");
@@ -4696,9 +4697,6 @@ void Clang::ConstructJob(Compilation ,
   Args.AddLastArg(CmdArgs, options::OPT_fdouble_square_bracket_attributes,
   options::OPT_fno_double_square_bracket_attributes);
 
-  bool HaveModules = false;
-  RenderModulesOptions(C, D, Args, Input, Output, CmdArgs, HaveModules);
-
   // -faccess-control is default.
   if (Args.hasFlag(options::OPT_fno_access_control,
options::OPT_faccess_control, false))
@@ -4765,6 +4763,7 @@ void Clang::ConstructJob(Compilation ,
   if (ImplyVCPPCXXVer) {
 StringRef LanguageStandard;
 if (const Arg *StdArg = Args.getLastArg(options::OPT__SLASH_std)) {
+  Std = StdArg;
   LanguageStandard = llvm::StringSwitch(StdArg->getValue())
  .Case("c++14", "-std=c++14")
  .Case("c++17", "-std=c++17")
@@ -4830,6 +4829,12 @@ void Clang::ConstructJob(Compilation ,
options::OPT_fno_inline_functions))
 InlineArg->render(Args, CmdArgs);
 
+  // FIXME: Find a better way to determine whether the language has modules
+  // support by default, or just assume that all languages do.
+  bool HaveModules =
+  Std && (Std->containsValue("c++2a") || Std->containsValue("c++latest"));
+  RenderModulesOptions(C, D, Args, Input, Output, CmdArgs, HaveModules);
+
   Args.AddLastArg(CmdArgs, options::OPT_fexperimental_new_pass_manager,
   options::OPT_fno_experimental_new_pass_manager);
 

Modified: cfe/trunk/lib/Frontend/FrontendActions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/FrontendActions.cpp?rev=358355=358354=358355=diff
==
--- cfe/trunk/lib/Frontend/FrontendActions.cpp (original)
+++ cfe/trunk/lib/Frontend/FrontendActions.cpp Sun Apr 14 04:11:37 2019
@@ -221,8 +221,8 @@ GenerateModuleFromModuleMapAction::Creat
 
 bool GenerateModuleInterfaceAction::BeginSourceFileAction(
 CompilerInstance ) {
-  if (!CI.getLangOpts().ModulesTS) {
-

[PATCH] D43763: libclang: Visit class template instantiations

[Milian Wolff via Phabricator via cfe-commits]

milianw added a comment.
Herald added a subscriber: arphaman.
Herald added a project: clang.

looks good, but this needs a test, could you add one please?


Repository:
  rC Clang

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

https://reviews.llvm.org/D43763



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

r358353 - [c++20] Parsing support for module-declarations, import-declarations,

[Richard Smith via cfe-commits]

Author: rsmith
Date: Sun Apr 14 01:06:59 2019
New Revision: 358353

URL: http://llvm.org/viewvc/llvm-project?rev=358353=rev
Log:
[c++20] Parsing support for module-declarations, import-declarations,
and the global and private module fragment.

For now, the private module fragment introducer is ignored, but use of
the global module fragment introducer should be properly enforced.

Added:
cfe/trunk/test/CXX/basic/basic.link/p1.cpp
cfe/trunk/test/CXX/basic/basic.link/p3.cpp
cfe/trunk/test/CXX/module/
cfe/trunk/test/CXX/module/module.unit/
cfe/trunk/test/CXX/module/module.unit/p3.cpp
Modified:
cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Lex/Preprocessor.h
cfe/trunk/include/clang/Parse/Parser.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Lex/PPDirectives.cpp
cfe/trunk/lib/Lex/Preprocessor.cpp
cfe/trunk/lib/Parse/Parser.cpp
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/test/CXX/modules-ts/basic/basic.link/module-declaration.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=358353=358352=358353=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Sun Apr 14 01:06:59 
2019
@@ -1223,15 +1223,22 @@ def err_unexpected_module_decl : Error<
   "module declaration can only appear at the top level">;
 def err_module_expected_ident : Error<
   "expected a module name after '%select{module|import}0'">;
-def err_module_implementation_partition : Error<
-  "module partition must be declared 'export'">;
 def err_attribute_not_module_attr : Error<
   "%0 attribute cannot be applied to a module">;
 def err_attribute_not_import_attr : Error<
   "%0 attribute cannot be applied to a module import">;
 def err_module_expected_semi : Error<
   "expected ';' after module name">;
+def err_global_module_introducer_not_at_start : Error<
+  "'module;' introducing a global module fragment can appear only "
+  "at the start of the translation unit">;
+def err_module_fragment_exported : Error<
+  "%select{global|private}0 module fragment cannot be exported">;
+def err_private_module_fragment_expected_semi : Error<
+  "expected ';' after private module fragment declaration">;
 def err_missing_before_module_end : Error<"expected %0 at end of module">;
+def err_unsupported_module_partition : Error<
+  "sorry, module partitions are not yet supported">;
 
 def err_export_empty : Error<"export declaration cannot be empty">;
 }

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=358353=358352=358353=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Sun Apr 14 01:06:59 
2019
@@ -9199,7 +9199,7 @@ let CategoryName = "Modules Issue" in {
 def err_module_decl_in_module_map_module : Error<
   "'module' declaration found while building module from module map">;
 def err_module_decl_in_header_module : Error<
-  "'module' declaration found while building header module">;
+  "'module' declaration found while building header unit">;
 def err_module_interface_implementation_mismatch : Error<
   "missing 'export' specifier in module declaration while "
   "building module interface">;
@@ -9217,6 +9217,9 @@ def err_module_redeclaration : Error<
 def note_prev_module_declaration : Note<"previous module declaration is here">;
 def err_module_declaration_missing : Error<
   "missing 'export module' declaration in module interface unit">;
+def err_module_declaration_missing_after_global_module_introducer : Error<
+  "missing 'module' declaration at end of global module fragment "
+  "introduced here">;
 def err_module_private_specialization : Error<
   "%select{template|partial|member}0 specialization cannot be "
   "declared __module_private__">;
@@ -9254,7 +9257,12 @@ def err_module_self_import : Error<
 def err_module_import_in_implementation : Error<
   "@import of module '%0' in implementation of '%1'; use #import">;
 
-// C++ Modules TS
+// C++ Modules
+def err_module_decl_not_at_start : Error<
+  "module declaration must occur at the start of the translation unit">;
+def note_global_module_introducer_missing : Note<
+  "add 'module;' to the start of the file to introduce a "
+  "global module fragment">;
 def err_export_within_export : Error<
   "export declaration appears within another export declaration">;
 def err_export_not_in_module_interface : Error<

Modified: cfe/trunk/include/clang/Lex/Preprocessor.h
URL: