[PATCH] D159453: [Matrix] Fix test on SystemZ

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar abandoned this revision.
kuhar added a comment.

Gah, disregard


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159453

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

[PATCH] D159453: [Matrix] Fix test on SystemZ

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
Herald added subscribers: hanchung, tschuett.
Herald added a project: All.
kuhar requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As reported by @uweigand in https://reviews.llvm.org/D158883:

  The newly added test cases in ffp-model.c fail on SystemZ, making CI red:
  https://lab.llvm.org/buildbot/#/builders/94/builds/16280
  
  The root cause seems to be that by default, the SystemZ back-end targets
  a machine without SIMD support, and therefore vector return types are
  passed via implicit reference according to the ABI

this uses manual stores instead of vector returns.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159453

Files:
  clang/test/CodeGen/ffp-model.c


Index: clang/test/CodeGen/ffp-model.c
===
--- clang/test/CodeGen/ffp-model.c
+++ clang/test/CodeGen/ffp-model.c
@@ -49,9 +49,9 @@
 
 typedef float __attribute__((ext_vector_type(2))) v2f;
 
-v2f my_vec_muladd(v2f x, float y, v2f z) {
-  // CHECK: define{{.*}} @my_vec_muladd
-  return x * y + z;
+void my_vec_muladd(v2f x, float y, v2f z, v2f *res) {
+  // CHECK: define{{.*}}@my_vec_muladd
+  *res = x * y + z;
 
   // CHECK-FAST: fmul fast <2 x float>
   // CHECK-FAST: load <2 x float>, ptr
@@ -83,9 +83,9 @@
 
 typedef float __attribute__((matrix_type(2, 1))) m21f;
 
-m21f my_m21_muladd(m21f x, float y, m21f z) {
-  // CHECK: define{{.*}} <2 x float> @my_m21_muladd
-  return x * y + z;
+void my_m21_muladd(m21f x, float y, m21f z, m21f *res) {
+  // CHECK: define{{.*}}@my_m21_muladd
+  *res = x * y + z;
 
   // CHECK-FAST: fmul fast <2 x float>
   // CHECK-FAST: load <2 x float>, ptr
@@ -117,9 +117,9 @@
 
 typedef float __attribute__((matrix_type(2, 2))) m22f;
 
-m22f my_m22_muladd(m22f x, float y, m22f z) {
-  // CHECK: define{{.*}} <4 x float> @my_m22_muladd
-  return x * y + z;
+void my_m22_muladd(m22f x, float y, m22f z, m22f *res) {
+  // CHECK: define{{.*}}@my_m22_muladd
+  *res = x * y + z;
 
   // CHECK-FAST: fmul fast <4 x float>
   // CHECK-FAST: load <4 x float>, ptr


Index: clang/test/CodeGen/ffp-model.c
===
--- clang/test/CodeGen/ffp-model.c
+++ clang/test/CodeGen/ffp-model.c
@@ -49,9 +49,9 @@
 
 typedef float __attribute__((ext_vector_type(2))) v2f;
 
-v2f my_vec_muladd(v2f x, float y, v2f z) {
-  // CHECK: define{{.*}} @my_vec_muladd
-  return x * y + z;
+void my_vec_muladd(v2f x, float y, v2f z, v2f *res) {
+  // CHECK: define{{.*}}@my_vec_muladd
+  *res = x * y + z;
 
   // CHECK-FAST: fmul fast <2 x float>
   // CHECK-FAST: load <2 x float>, ptr
@@ -83,9 +83,9 @@
 
 typedef float __attribute__((matrix_type(2, 1))) m21f;
 
-m21f my_m21_muladd(m21f x, float y, m21f z) {
-  // CHECK: define{{.*}} <2 x float> @my_m21_muladd
-  return x * y + z;
+void my_m21_muladd(m21f x, float y, m21f z, m21f *res) {
+  // CHECK: define{{.*}}@my_m21_muladd
+  *res = x * y + z;
 
   // CHECK-FAST: fmul fast <2 x float>
   // CHECK-FAST: load <2 x float>, ptr
@@ -117,9 +117,9 @@
 
 typedef float __attribute__((matrix_type(2, 2))) m22f;
 
-m22f my_m22_muladd(m22f x, float y, m22f z) {
-  // CHECK: define{{.*}} <4 x float> @my_m22_muladd
-  return x * y + z;
+void my_m22_muladd(m22f x, float y, m22f z, m22f *res) {
+  // CHECK: define{{.*}}@my_m22_muladd
+  *res = x * y + z;
 
   // CHECK-FAST: fmul fast <4 x float>
   // CHECK-FAST: load <4 x float>, ptr
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146104: Use *{Map,Set}::contains (NFC)

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar accepted this revision.
kuhar added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146104

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

[PATCH] D145987: [ADT][NFCI] Do not use non-const lvalue-refs with enumerate in llvm/

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb9db89fbcfda: [ADT][NFCI] Do not use non-const lvalue-refs 
with enumerate in llvm/ (authored by kuhar).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145987

Files:
  clang/tools/clang-refactor/TestSupport.cpp
  llvm/lib/ObjectYAML/DWARFYAML.cpp
  llvm/lib/ObjectYAML/MinidumpEmitter.cpp
  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
  llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
  llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp

Index: llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
===
--- llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
+++ llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
@@ -89,20 +89,20 @@
   TraversableEdges(MatchDag.getNumEdges()),
   TestablePredicates(MatchDag.getNumPredicates()) {
   // Number all the predicates in this DAG
-  for (auto &P : enumerate(MatchDag.predicates())) {
-PredicateIDs.insert(std::make_pair(P.value(), P.index()));
+  for (const auto &[Idx, P] : enumerate(MatchDag.predicates())) {
+PredicateIDs.insert(std::make_pair(P, Idx));
   }
 
   // Number all the predicate dependencies in this DAG and set up a bitvector
   // for each predicate indicating the unsatisfied dependencies.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
-PredicateDepIDs.insert(std::make_pair(Dep.value(), Dep.index()));
+  for (const auto &[Idx, Dep] : enumerate(MatchDag.predicate_edges())) {
+PredicateDepIDs.insert(std::make_pair(Dep, Idx));
   }
   UnsatisfiedPredDepsForPred.resize(MatchDag.getNumPredicates(),
 BitVector(PredicateDepIDs.size()));
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
-unsigned ID = PredicateIDs.lookup(Dep.value()->getPredicate());
-UnsatisfiedPredDepsForPred[ID].set(Dep.index());
+  for (const auto &[Idx, Dep] : enumerate(MatchDag.predicate_edges())) {
+unsigned ID = PredicateIDs.lookup(Dep->getPredicate());
+UnsatisfiedPredDepsForPred[ID].set(Idx);
   }
 }
 
@@ -134,10 +134,10 @@
   // Mark the dependencies that are now satisfied as a result of this
   // instruction and mark any predicates whose dependencies are fully
   // satisfied.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
+  for (const auto &Dep : enumerate(MatchDag.predicate_edges())) {
 if (Dep.value()->getRequiredMI() == Instr &&
 Dep.value()->getRequiredMO() == nullptr) {
-  for (auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
+  for (const auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
 DepsFor.value().reset(Dep.index());
 if (DepsFor.value().none())
   TestablePredicates.set(DepsFor.index());
@@ -157,10 +157,9 @@
   // When an operand becomes reachable, we potentially activate some traversals.
   // Record the edges that can now be followed as a result of this
   // instruction.
-  for (auto &E : enumerate(MatchDag.edges())) {
-if (E.value()->getFromMI() == Instr &&
-E.value()->getFromMO()->getIdx() == OpIdx) {
-  TraversableEdges.set(E.index());
+  for (const auto &[Idx, E] : enumerate(MatchDag.edges())) {
+if (E->getFromMI() == Instr && E->getFromMO()->getIdx() == OpIdx) {
+  TraversableEdges.set(Idx);
 }
   }
 
@@ -168,10 +167,10 @@
   // Clear the dependencies that are now satisfied as a result of this
   // operand and activate any predicates whose dependencies are fully
   // satisfied.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
+  for (const auto &Dep : enumerate(MatchDag.predicate_edges())) {
 if (Dep.value()->getRequiredMI() == Instr && Dep.value()->getRequiredMO() &&
 Dep.value()->getRequiredMO()->getIdx() == OpIdx) {
-  for (auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
+  for (const auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
 DepsFor.value().reset(Dep.index());
 if (DepsFor.value().none())
   TestablePredicates.set(DepsFor.index());
@@ -536,22 +535,22 @@
 
   BitVector PossibleLeaves = getPossibleLeavesForPartition(PartitionIdx);
   // Consume any predicates we handled.
-  for (auto &EnumeratedLeaf : enumerate(Builder.getPossibleLeaves())) {
-if (!PossibleLeaves[EnumeratedLeaf.index()])
+  for (const auto &[Index, EnumeratedLeaf] :
+   enumerate(Builder.getPossibleLeaves())) {
+if (!PossibleLeaves[Index])
   continue;
 
-auto &Leaf = EnumeratedLeaf.value();
-const auto &TestedPredicatesForLeaf =
-TestedPredicates[EnumeratedLeaf.index()];
+const auto &TestedPredicatesForLeaf = TestedPredicates[Index];
 
 for (unsigned PredIdx : TestedPredicatesForLeaf.set_bits()) {
-  LLVM_DEBUG(dbgs() << ""

[PATCH] D145987: [ADT][NFCI] Do not use non-const lvalue-refs with enumerate in llvm/

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 504901.
kuhar marked an inline comment as done.
kuhar added a comment.

Update lambda signature


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145987

Files:
  clang/tools/clang-refactor/TestSupport.cpp
  llvm/lib/ObjectYAML/DWARFYAML.cpp
  llvm/lib/ObjectYAML/MinidumpEmitter.cpp
  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
  llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
  llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp

Index: llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
===
--- llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
+++ llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
@@ -89,20 +89,20 @@
   TraversableEdges(MatchDag.getNumEdges()),
   TestablePredicates(MatchDag.getNumPredicates()) {
   // Number all the predicates in this DAG
-  for (auto &P : enumerate(MatchDag.predicates())) {
-PredicateIDs.insert(std::make_pair(P.value(), P.index()));
+  for (const auto &[Idx, P] : enumerate(MatchDag.predicates())) {
+PredicateIDs.insert(std::make_pair(P, Idx));
   }
 
   // Number all the predicate dependencies in this DAG and set up a bitvector
   // for each predicate indicating the unsatisfied dependencies.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
-PredicateDepIDs.insert(std::make_pair(Dep.value(), Dep.index()));
+  for (const auto &[Idx, Dep] : enumerate(MatchDag.predicate_edges())) {
+PredicateDepIDs.insert(std::make_pair(Dep, Idx));
   }
   UnsatisfiedPredDepsForPred.resize(MatchDag.getNumPredicates(),
 BitVector(PredicateDepIDs.size()));
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
-unsigned ID = PredicateIDs.lookup(Dep.value()->getPredicate());
-UnsatisfiedPredDepsForPred[ID].set(Dep.index());
+  for (const auto &[Idx, Dep] : enumerate(MatchDag.predicate_edges())) {
+unsigned ID = PredicateIDs.lookup(Dep->getPredicate());
+UnsatisfiedPredDepsForPred[ID].set(Idx);
   }
 }
 
@@ -134,10 +134,10 @@
   // Mark the dependencies that are now satisfied as a result of this
   // instruction and mark any predicates whose dependencies are fully
   // satisfied.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
+  for (const auto &Dep : enumerate(MatchDag.predicate_edges())) {
 if (Dep.value()->getRequiredMI() == Instr &&
 Dep.value()->getRequiredMO() == nullptr) {
-  for (auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
+  for (const auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
 DepsFor.value().reset(Dep.index());
 if (DepsFor.value().none())
   TestablePredicates.set(DepsFor.index());
@@ -157,10 +157,9 @@
   // When an operand becomes reachable, we potentially activate some traversals.
   // Record the edges that can now be followed as a result of this
   // instruction.
-  for (auto &E : enumerate(MatchDag.edges())) {
-if (E.value()->getFromMI() == Instr &&
-E.value()->getFromMO()->getIdx() == OpIdx) {
-  TraversableEdges.set(E.index());
+  for (const auto &[Idx, E] : enumerate(MatchDag.edges())) {
+if (E->getFromMI() == Instr && E->getFromMO()->getIdx() == OpIdx) {
+  TraversableEdges.set(Idx);
 }
   }
 
@@ -168,10 +167,10 @@
   // Clear the dependencies that are now satisfied as a result of this
   // operand and activate any predicates whose dependencies are fully
   // satisfied.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
+  for (const auto &Dep : enumerate(MatchDag.predicate_edges())) {
 if (Dep.value()->getRequiredMI() == Instr && Dep.value()->getRequiredMO() &&
 Dep.value()->getRequiredMO()->getIdx() == OpIdx) {
-  for (auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
+  for (const auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
 DepsFor.value().reset(Dep.index());
 if (DepsFor.value().none())
   TestablePredicates.set(DepsFor.index());
@@ -536,22 +535,22 @@
 
   BitVector PossibleLeaves = getPossibleLeavesForPartition(PartitionIdx);
   // Consume any predicates we handled.
-  for (auto &EnumeratedLeaf : enumerate(Builder.getPossibleLeaves())) {
-if (!PossibleLeaves[EnumeratedLeaf.index()])
+  for (const auto &[Index, EnumeratedLeaf] :
+   enumerate(Builder.getPossibleLeaves())) {
+if (!PossibleLeaves[Index])
   continue;
 
-auto &Leaf = EnumeratedLeaf.value();
-const auto &TestedPredicatesForLeaf =
-TestedPredicates[EnumeratedLeaf.index()];
+const auto &TestedPredicatesForLeaf = TestedPredicates[Index];
 
 for (unsigned PredIdx : TestedPredicatesForLeaf.set_bits()) {
-  LLVM_DEBUG(dbgs() << "" << Leaf.getName() << " tested predicate #"
-<< PredIdx << " of " << TestedPredicatesForLeaf.size

[PATCH] D145987: [ADT][NFCI] Do not use non-const lvalue-refs with enumerate in llvm/

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 504837.
kuhar added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Also include clang/ since there's only one function to update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145987

Files:
  clang/tools/clang-refactor/TestSupport.cpp
  llvm/lib/ObjectYAML/DWARFYAML.cpp
  llvm/lib/ObjectYAML/MinidumpEmitter.cpp
  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
  llvm/tools/llvm-reduce/deltas/ReduceArguments.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
  llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp

Index: llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
===
--- llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
+++ llvm/utils/TableGen/GlobalISel/GIMatchTree.cpp
@@ -89,20 +89,20 @@
   TraversableEdges(MatchDag.getNumEdges()),
   TestablePredicates(MatchDag.getNumPredicates()) {
   // Number all the predicates in this DAG
-  for (auto &P : enumerate(MatchDag.predicates())) {
-PredicateIDs.insert(std::make_pair(P.value(), P.index()));
+  for (const auto &[Idx, P] : enumerate(MatchDag.predicates())) {
+PredicateIDs.insert(std::make_pair(P, Idx));
   }
 
   // Number all the predicate dependencies in this DAG and set up a bitvector
   // for each predicate indicating the unsatisfied dependencies.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
-PredicateDepIDs.insert(std::make_pair(Dep.value(), Dep.index()));
+  for (const auto &[Idx, Dep] : enumerate(MatchDag.predicate_edges())) {
+PredicateDepIDs.insert(std::make_pair(Dep, Idx));
   }
   UnsatisfiedPredDepsForPred.resize(MatchDag.getNumPredicates(),
 BitVector(PredicateDepIDs.size()));
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
-unsigned ID = PredicateIDs.lookup(Dep.value()->getPredicate());
-UnsatisfiedPredDepsForPred[ID].set(Dep.index());
+  for (const auto &[Idx, Dep] : enumerate(MatchDag.predicate_edges())) {
+unsigned ID = PredicateIDs.lookup(Dep->getPredicate());
+UnsatisfiedPredDepsForPred[ID].set(Idx);
   }
 }
 
@@ -134,10 +134,10 @@
   // Mark the dependencies that are now satisfied as a result of this
   // instruction and mark any predicates whose dependencies are fully
   // satisfied.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
+  for (const auto &Dep : enumerate(MatchDag.predicate_edges())) {
 if (Dep.value()->getRequiredMI() == Instr &&
 Dep.value()->getRequiredMO() == nullptr) {
-  for (auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
+  for (const auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
 DepsFor.value().reset(Dep.index());
 if (DepsFor.value().none())
   TestablePredicates.set(DepsFor.index());
@@ -157,10 +157,9 @@
   // When an operand becomes reachable, we potentially activate some traversals.
   // Record the edges that can now be followed as a result of this
   // instruction.
-  for (auto &E : enumerate(MatchDag.edges())) {
-if (E.value()->getFromMI() == Instr &&
-E.value()->getFromMO()->getIdx() == OpIdx) {
-  TraversableEdges.set(E.index());
+  for (const auto &[Idx, E] : enumerate(MatchDag.edges())) {
+if (E->getFromMI() == Instr && E->getFromMO()->getIdx() == OpIdx) {
+  TraversableEdges.set(Idx);
 }
   }
 
@@ -168,10 +167,10 @@
   // Clear the dependencies that are now satisfied as a result of this
   // operand and activate any predicates whose dependencies are fully
   // satisfied.
-  for (auto &Dep : enumerate(MatchDag.predicate_edges())) {
+  for (const auto &Dep : enumerate(MatchDag.predicate_edges())) {
 if (Dep.value()->getRequiredMI() == Instr && Dep.value()->getRequiredMO() &&
 Dep.value()->getRequiredMO()->getIdx() == OpIdx) {
-  for (auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
+  for (const auto &DepsFor : enumerate(UnsatisfiedPredDepsForPred)) {
 DepsFor.value().reset(Dep.index());
 if (DepsFor.value().none())
   TestablePredicates.set(DepsFor.index());
@@ -536,22 +535,22 @@
 
   BitVector PossibleLeaves = getPossibleLeavesForPartition(PartitionIdx);
   // Consume any predicates we handled.
-  for (auto &EnumeratedLeaf : enumerate(Builder.getPossibleLeaves())) {
-if (!PossibleLeaves[EnumeratedLeaf.index()])
+  for (const auto &[Index, EnumeratedLeaf] :
+   enumerate(Builder.getPossibleLeaves())) {
+if (!PossibleLeaves[Index])
   continue;
 
-auto &Leaf = EnumeratedLeaf.value();
-const auto &TestedPredicatesForLeaf =
-TestedPredicates[EnumeratedLeaf.index()];
+const auto &TestedPredicatesForLeaf = TestedPredicates[Index];
 
 for (unsigned PredIdx : TestedPredicatesForLeaf.set_bits()) {
-  LLVM_DEBUG(dbgs() << "" << Leaf.getName() << " tested predicate #"
-   

[PATCH] D132001: [clang-format] Fix regressions in WhitespaceSensitiveMacros

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

This seems to have caused a test failure:
https://lab.llvm.org/buildbot/#/builders/109/builds/45138

  Failed Tests (1):
Clang-Unit :: 
Format/./FormatTests/TokenAnnotatorTest/UnderstandsLBracesInMacroDefinition
  Testing Time: 92.30s
Skipped  :38
Unsupported  :   767
Passed   : 82570
Expectedly Failed:   194
Failed   : 1
  
  TokenAnnotatorTest.cpp:292: Failure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132001

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

[PATCH] D101471: [clang-tidy] Add proper emplace checks to modernize-use-emplace

[Jakub Kuderski via Phabricator via cfe-commits]

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

LGTM but please get a second approval before submitting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101471

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

[PATCH] D101471: [clang-tidy] Add proper emplace checks to modernize-use-emplace

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar resigned from this revision.
kuhar added inline comments.
Herald added a subscriber: carlosgalvezp.
Herald added a project: All.



Comment at: clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp:44
+  // FullNameTrimmed matches any of the given Names.
+  const StringRef FullNameTrimmedRef = StringRef(FullNameTrimmed);
+  for (const StringRef Pattern : Names) {

nit: `StringRef(` seems unnecessary on the rhs of `=`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101471

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

[PATCH] D123278: [Clang] [Docs] Add HLSLSupport page

[Jakub Kuderski via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/docs/HLSLSupport.rst:223
+* References
+* goto or labels
+* Variable Length Arrays

nit, for consistency with the other list items: make goto verbatim



Comment at: clang/docs/HLSLSupport.rst:225
+* Variable Length Arrays
+* _Complex and _Imaginary
+* C Threads or Atomics (or Obj-C blocks)

nit: also verbatim?



Comment at: clang/docs/HLSLSupport.rst:237
+* Anonymous or inline namespaces
+* ``new`` & ``delete`` operators in all of their forms (array, placement, etc)
+* Constructors & destructors

ubernit: & -> and, for consistency with the other list items


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123278

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

[PATCH] D123278: [Clang] [Docs] Add HLSLSupport page

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/docs/HLSLSupport.rst:190
+* RTTI
+* Exceptions
+* Multiple inheritance

How about goto and labels? Irreducible control flow? Are infinite loops valid?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123278

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

[PATCH] D123278: [Clang] [Docs] Add HLSLSupport page

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Looks good to me overall, I just left some local comments. Please take my 
writing suggestions with a pinch of salt, English is my second language.




Comment at: clang/docs/HLSLSupport.rst:13
+describes the high level goals of the project, the guiding principals, as well
+as some of the idiosyncrasies of the HLSL language and how we intend to support
+them in Clang.

ubernit: some of the idiosyncrasies -> some idiosyncrasies



Comment at: clang/docs/HLSLSupport.rst:22-23
+`_. in all its supported
+use cases. Accomplishing that goal will require a high level of source
+compatibility.
+

What do you mean by source compatibility in this case? In terms of the input 
source language (HLSL), or the source code of both implementations?

Perhaps we could make it more direct? E.g., 'This requires us to add the HLSL 
language support to Clang.'



Comment at: clang/docs/HLSLSupport.rst:28-31
+HLSL ASTs do not need to be compatible between DXC and Clang. We do not expect
+identical code generation or that features will resemble DXC's implementation 
or
+architecture. In fact, we explicitly expect to deviate from DXC's 
implementation
+in key ways.

It's great you made this so explicit!



Comment at: clang/docs/HLSLSupport.rst:36
+
+This document lacks precise details for architectural decisions that are not 
yet
+finalized. Our top priorities are quality, maintainability, and flexibility. In

ubernit: how about just 'details'?



Comment at: clang/docs/HLSLSupport.rst:50-52
+HLSL is not a formally or fully specified language, and while our goals require
+a high level of source compatibility, implementations can vary and we have some
+flexibility to be more or less permissive in some cases.

Is DXC the reference implementation?



Comment at: clang/docs/HLSLSupport.rst:55-56
+The HLSL effort prioritizes following similar patterns for other languages,
+drivers, runtimes and targets. Specifically, HLSL will create separate
+implementation files to contain the HLSL-specific behavior wherever it makes
+sense (i.e. ParseHLSL.cpp, SemaHLSL.cpp, CGHLSL*.cpp...). We will use inline

This is a bit awkward, as if the language HSLS created files on disk on 
something :P. 

Maybe something like: We will maintain separation between HSLS-specific code 
and the rest of Clang as much as possible (e.g., ParseHSLS.cpp, ...)



Comment at: clang/docs/HLSLSupport.rst:90-91
+metadata. *hand wave* *hand wave* *hand wave* As a design principle here we 
want
+our IR to be idiomatic Clang IR as much as possible. We will use IR attributes
+wherever we can, and use metadata as sparingly as possible. One example of a
+difference from DXC already implemented in Clang is the use of target triples 
to

Are all attributes guaranteed to be preserved? I thought some might get dropped 
by opt.



Comment at: clang/docs/HLSLSupport.rst:128
+HLSL does support member functions, and (in HLSL 2021) limited operator
+overloading. With member function support HLSL also has a ``this`` keyword. The
+``this`` keyword is an example of one of the places where HLSL relies on

nit: With member function support*,* HLSL also



Comment at: clang/docs/HLSLSupport.rst:135
+
+This is a simple one, but it deviates from C so is worth mentioning. HLSL
+bitshifts are defined to mask the shift count by the size of the type. In DXC,

nit: so *it* is worth?



Comment at: clang/docs/HLSLSupport.rst:137
+bitshifts are defined to mask the shift count by the size of the type. In DXC,
+the semantics of LLVM IR were altered to accomodate this, in Clang we intend to
+generate the mask explicitly in the IR.

accommodate



Comment at: clang/docs/HLSLSupport.rst:187
+
+HLSL does not support the following C++ features:
+

These are C++ language features. I assume that all library features are also 
out of the window?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123278

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

[PATCH] D119061: [Clang] noinline call site attribute

[Jakub Kuderski via Phabricator via cfe-commits]

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

LGTM put please get at least one additional approval before submitting


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

https://reviews.llvm.org/D119061

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

[PATCH] D119061: [Clang] noinline call site attribute

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/test/CodeGen/attr-noinline.cpp:11
+
+void foo(int i) {
+  [[clang::noinline]] bar();

Could you also add a test function that is already marked as noinline at the 
declaration?


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

https://reviews.llvm.org/D119061

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

[PATCH] D119061: [Clang] noinline call site attribute

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

In D119061#3312571 , @xbolva00 wrote:

>>> Do you plan to also add inline and flatten?
>
> You mean always_inline? Yes, after noinline. The flatten call site attribute 
> - theoretically why not, but it needs to be reworked in LLVM (like 
> always_inline_recursively) before any patch like this one.

SGTM.

>>> When I worked on my implementation, I remember that I also ran into the 
>>> issue of conflicting attributes. I defaulted to whatever the inliner 
>>> behavior was at the time, but a few folks pointed out to me that this can 
>>> be very confusing. I think this needs thorough documentation / testing.
>
> Yes, as we mentioned it already, for example always_inline on decl, and 
> noinline on callsite. We should diagnose these cases and always prefer call 
> site attribute. (and as I said, some fixes for inliner are needed).

It would be good to check with a language expert if this can break some code. 
I'm thinking of situations when someone relies on a function being emitted (or 
not) and ending up with linking issues. I'm not an expert here, but this is a 
past concern I vaguely remember.


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

https://reviews.llvm.org/D119061

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

[PATCH] D119061: [Clang] noinline call site attribute

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Thanks for working on this, @xbolva00! I don't know this part of the codebase, 
so won't comment on the patch itself. Just a few questions/suggestions:

1. Do you plan to also add `inline` and `flatten`?

2. When I worked on my implementation, I remember that I also ran into the 
issue of conflicting attributes. I default to whatever the inliner behavior was 
at the time, but a few folks pointed out to me that this can be very confusing. 
I think this needs thorough documentation / testing.


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

https://reviews.llvm.org/D119061

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

[PATCH] D118386: [Support][NFC] Fix generic `ChildrenGetterTy` of `IDFCalculatorBase`

[Jakub Kuderski via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118386

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

[PATCH] D112926: run-clang-tidy.py analyze unique files only

[Jakub Kuderski via Phabricator via cfe-commits]

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

I think this change is fine.

One could argue that having multiple files is an issue with tooling/build 
system in the first place, and we could consider printing a warning instead of 
silently fixing it up, but I don't think there's that much to gain from being 
pedantic here.


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

https://reviews.llvm.org/D112926

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

[PATCH] D101471: [clang-tidy] Add proper emplace checks to modernize-use-emplace

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp:28
+  std::string FullNameTrimmed;
+  int Count = 0;
+  for (const auto &Character : FullName) {

Can you add a comment explaining what this loops tries to do? Ideally, with a 
short example like `some_func --> ::some_func`



Comment at: clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp:29
+  int Count = 0;
+  for (const auto &Character : FullName) {
+if (Character == '<') {

Eugene.Zelenko wrote:
> I'm not sure, but probably braces could be elided in `for` and `if else`.
-1 for removing braces in multi-statement loops



Comment at: clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp:46
+} else if (FullNameTrimmedRef.endswith(Pattern) &&
+   FullNameTrimmedRef.drop_back(Pattern.size()).endswith("::")) {
+  return true;

Eugene.Zelenko wrote:
> I'm not sure, but probably braces could be elided.
-1



Comment at: clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp:294
+  } else {
+if ((MakeCall ? MakeCall->getNumArgs() : CtorCall->getNumArgs()) == 0) {
+  Diag << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(

Can you pull this ternary expression into a variable so that it does not have 
to repeated when the diagnostic is emitted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101471

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

[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Just two nits from me. I think it looks fine, but I'm not familiar with the new 
pass manager and don't feel confident enough to approve it.




Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:134
 
 public:
   static cl::opt VerifyPreservedCFG;

yrouban wrote:
> kuhar wrote:
> > yrouban wrote:
> > > kuhar wrote:
> > > > not necessary anymore
> > > there can bee a need to disabled/enable (e.g. for some tests or for 
> > > debugging).
> > I meant the 'public:'. You made everything public at the very top of the 
> > class.
> sure
I don't think this got fixed.



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:1096
+  checkCFG(P, F->getName(), *GraphBefore,
+   CFG(F, false /* TrackBBLifetime */));
   });

nit: can you put the commented argument name before the argument?


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

https://reviews.llvm.org/D91327

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

[PATCH] D94827: [SimplifyCFG] If provided, preserve Dominator Tree

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar accepted this revision.
kuhar added a comment.

Choo choo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827

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

[PATCH] D94827: [SimplifyCFG] Require and preserve dominator tree

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Wow, this is fantastic. When I first started working on the domtree updater 
back in 2017, SimplifyGFG seemed like one of the most difficult passes to 
handle, and I wasn't sure if we ever get there. Very impressive work, 
@lebedev.ri!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94827

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

[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:134
 
 public:
   static cl::opt VerifyPreservedCFG;

yrouban wrote:
> kuhar wrote:
> > not necessary anymore
> there can bee a need to disabled/enable (e.g. for some tests or for 
> debugging).
I meant the 'public:'. You made everything public at the very top of the class.


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

https://reviews.llvm.org/D91327

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

[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Looks fine to me, but I'm not confident enough to give an approval.




Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:733-735
+  static AnalysisKey Key;
+
+public:

This is already public


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

https://reviews.llvm.org/D91327

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

[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Found some cosmetics, but I'm not familiar enough with the NPM to do a full 
review.




Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:99
   struct CFG {
 struct BBGuard final : public CallbackVH {
   BBGuard(const BasicBlock *BB) : CallbackVH(BB) {}

Consider pulling this out of CFG. I don't see many reasons for having 3 levels 
of class nesting.



Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:134
 
 public:
   static cl::opt VerifyPreservedCFG;

not necessary anymore



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:728
+  Result run(Function &F, FunctionAnalysisManager &AM) {
+return Result(&F, true /* TrackBBLifetime */);
+  }

nit: move parameter name to the left?



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:759
+CFG::printDiff(dbgs(), GraphBefore, GraphAfter);
+report_fatal_error(Twine("CFG unexpectedly changed by ", Pass));
+  };

I think this will print with `errs()`. Would it make sense to flush `dbgs()` 
ahead of printing with `errs()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91327

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

[PATCH] D83088: Introduce CfgTraits abstraction

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Hi Nicoali,

In D83088#2227151 , @nhaehnle wrote:

> ...
> Take a look here for example: 
> https://github.com/nhaehnle/llvm-project/blob/715450fa7f968ceefaf9c3b04b47066866c97206/llvm/lib/Analysis/GenericConvergenceUtils.cpp#L499
>  -- this is obviously still fairly simple, but it's an example of printing 
> out the results of an analysis in a way that's generic over the underlying 
> CFG and SSA form. A statically polymorphic wrapper is here: 
> https://github.com/nhaehnle/llvm-project/blob/715450fa7f968ceefaf9c3b04b47066866c97206/llvm/include/llvm/Analysis/GenericConvergenceUtils.h#L569
>
> The simple example might be bearable writing as a template, precisely because 
> it's simple -- so only looking at simple examples is unlikely to really 
> capture the motivation. Really what the motivation boils down to is stuff 
> like this: 
> https://github.com/nhaehnle/llvm-project/blob/controlflow-wip-v7/llvm/lib/Analysis/GenericUniformAnalysis.cpp
>  -- I don't fancy writing all this as a template.
>
> Thid motivation would essentially go away if C++ could type-check against 
> traits in the way that Rust and other languages like it can -- but it can't, 
> so here we are.

Based on your description and the DomTree patches, if I understand correctly, 
the primary motivation is to facilitate writing CFG-representation-agnostic 
algorithms/analyses (e.g., dominators, divergence, convergence analyses), such 
that you can later lift the results back to the representation-aware types? If 
that's correct, I support the overall goal. Having spent probably ~weeks 
wrangling with domtree templates, this sounds like something that could 
simplify life a lot and potentially cut down on compilation times & sizes of 
llvm binaries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83088

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

[PATCH] D84713: [DominatorTree] Simplify ChildrenGetter.

[Jakub Kuderski via Phabricator via cfe-commits]

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

LGTM.

One tiny nit: the function name `ChildrenGet` sounds kind of backwards to me, 
but it seems like the other direction is already taken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84713



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

[PATCH] D83087: DomTree: remove explicit use of DomTreeNodeBase::iterator

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

In D83087#2139211 , @nhaehnle wrote:

> In D83087#2134881 , @kuhar wrote:
>
> > modulo accidental formatting changes.
>
>
> I'm not aware of any. Some line breaks changed because "const_iterator" is 
> longer than "iterator".


Ack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83087



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

[PATCH] D83087: DomTree: remove explicit use of DomTreeNodeBase::iterator

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar accepted this revision.
kuhar added a comment.

LGTM modulo accidental formatting changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83087



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

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

[Jakub Kuderski via Phabricator via cfe-commits]

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

Looks good to me overall. I don't want to block it over the cosmetic issues 
like allocating the empty GD object.




Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:325
+auto &GD = BUI ? BUI->PreViewCFG : EmptyGD;
+return !empty(children({&GD, N}));
   }

asbirlea wrote:
> kuhar wrote:
> > This pattern repeats a few times. Could it be pushed into a helper function 
> > to get something like this?
> > 
> > ```
> > return !empty(children(GetGraphDiffNodePair(BUI, N)));
> > ```
> > 
> My dilemma here is how to not allocate a GraphDiffT instance. There are 4 
> cases where the pattern is inside a static method and once in a class method. 
> I used a stack var for the 4 cases and a unique_ptr for the class method.
> 
> Sure, I can do:
> ```
> static auto getGDNPair(BUI, EmptyGD, N) {
> return std::make_pair<>(BUI ? BUI->PreViewCFG : EmptyGD, N);
> }
> {
> ...
> GraphDiffT EmptyGD;
> return !empty(children(getGDNPair(BUI, &EmptyGD, N)));
> }
> ```
> But it felt like moving one line at the callsite to a oneline helper is not 
> making things much better.
> 
> 
> I was hoping for something more along the lines of:
> ```
> return !empty(children({getGD(BUI), N});
> ```
> Or, ensure BUI is always non-null, and simplify to:
> ```
> return !empty(children({BUI->PreViewCFG, N});
> ```
> 
> 
> Thoughts?
Does allocating a GD have a big cost?

I think you could get away with returning a temporary GD object that would die 
as soon as the expression evaluation ends -- should be no different than 
placing it on the stack just before the function call.

Overall, I still don't understand why you try to avoid creating a wrapper 
function / struct that returns children, and call `childredn<...>(...)` 
directly. Either way, I being able to write:
```
return !empty(children({getGD(BUI), N});
```
or 
```
return !empty(getChildren(BUI, N));
```
would definitely be concise and readable enough IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341



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

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: llvm/include/llvm/IR/CFGDiff.h:198
 
+namespace {
+template 

kuhar wrote:
> What benefit does an anonymous namespace in a header have over a named one, 
> e.g., `detail`/`impl`? Doesn't it make it more difficult to deduplicate 
> symbols across different translation units? Not sure what the best practices 
> are in C++ now in this area.
Found an article on this matter: 
https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL59-CPP.+Do+not+define+an+unnamed+namespace+in+a+header+file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341



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

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

In D77341#1960854 , @asbirlea wrote:

> Address comments.


Thanks for the changes and explanations. It think with a few more tweaks this 
will be a good refactoring step towards phasing out BUI.




Comment at: llvm/include/llvm/IR/CFGDiff.h:198
 
+namespace {
+template 

What benefit does an anonymous namespace in a header have over a named one, 
e.g., `detail`/`impl`? Doesn't it make it more difficult to deduplicate symbols 
across different translation units? Not sure what the best practices are in C++ 
now in this area.



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:325
+auto &GD = BUI ? BUI->PreViewCFG : EmptyGD;
+return !empty(children({&GD, N}));
   }

This pattern repeats a few times. Could it be pushed into a helper function to 
get something like this?

```
return !empty(children(GetGraphDiffNodePair(BUI, N)));
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341



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

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:186
+  for (auto &Pair : children(GDNodePair)) {
+const NodePtr Succ = Pair.second;
 const auto SIT = NodeToInfo.find(Succ);

kuhar wrote:
> Could this new code be a hoisted into a helper function?
Or alternatively, could the old `ChildrenGetter` be implemented with these 5 
magic lines?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341



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

[PATCH] D77341: [DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff.

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: llvm/include/llvm/IR/CFGDiff.h:199
+namespace {
+template  struct reverse_if_helper;
+template <> struct reverse_if_helper {

You can use two helper functions taking `integral_constant` 
tags -- should be less verbose



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:89
 bool IsRecalculated = false;
+GraphDiffT *PreViewOfCFG;
+const size_t NumLegalized;

nit: Why pointer instead of keeping a reference?



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:177
   constexpr bool Direction = IsReverse != IsPostDom;  // XOR.
-  for (const NodePtr Succ :
-   ChildrenGetter::Get(BB, BatchUpdates)) {
+  typedef
+  typename std::conditional, NodePtr>::type

nit: use `using` instead of `typefef`.



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:178
+  typedef
+  typename std::conditional, NodePtr>::type
+  IsRevType;

nit: can we use `std::conditional_t<...>` in c++14 to get rid of `typename` and 
`::type`? Or does it make some buildbots unhappy?



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:179
+  typename std::conditional, NodePtr>::type
+  IsRevType;
+  using GraphDiffBBPair = std::pair;

nit: I don't find this name very informative. Maybe something like `DirectedBB`?



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:183
+  BatchUpdates ? BatchUpdates->PreViewOfCFG : EmptyGD.get();
+  std::pair GDNodePair =
+  std::make_pair(GDTmp, BB);

`std::pair GDNodePair(GDTmp, BB)`



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:185
+  std::make_pair(GDTmp, BB);
+  for (auto &Pair : children(GDNodePair)) {
+const NodePtr Succ = Pair.second;

Won't children infer the template parameter based on the passes value? I don't 
get why the template argument type is a pair where the second argument is a 
directed nodeptr, but the runtime value is always a plain nodeptr. Couldn't the 
second pair type also be directed for `GDNodePair`?



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:186
+  for (auto &Pair : children(GDNodePair)) {
+const NodePtr Succ = Pair.second;
 const auto SIT = NodeToInfo.find(Succ);

Could this new code be a hoisted into a helper function?



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:1146
   if (Update.getKind() == UpdateKind::Insert)
-DT.insertEdge(Update.getFrom(), Update.getTo());
+InsertEdge(DT, nullptr, Update.getFrom(), Update.getTo());
   else

Could you add a comment next to the nullptr with the argument name?



Comment at: llvm/include/llvm/Support/GenericDomTreeConstruction.h:1543
+  // FIXME: Updated to use the PreViewCFG and behave the same as until now.
+  // This behavior is however incorrect; this actually needs the PostViewCFG.
+  GraphDiff PreViewCFG(

Does this care about the direction (pre- or post-) at all, or does it need some 
CFG view? Why is pre-view incorrect?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77341



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

The non-static-analyzer bits look good to me, I added a few nits.




Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:230
+
+  virtual void releaseMemory() {
+PostDomTree.releaseMemory();

If the virtual function is introduced by ManagesAnalysis, isn't it better to 
use override here?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:235
+  // Lazily retrieves the set of control dependencies to \p A.
+  const CFGBlockVector &getControlDependencies(CFGBlock *A) {
+auto It = ControlDepenencyMap.find(A);

 Can it be const?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:252
+  /// Whether \p A is control dependent on \p B.
+  bool isControlDependent(CFGBlock *A, CFGBlock *B) {
+return llvm::is_contained(getControlDependencies(A), B);

Can it be const?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+  // Dumps immediate control dependencies for each block.
+  void dump() {
+CFG *cfg = PostDomTree.getCFG();

Can `dump` be const? 



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:257
+  // Dumps immediate control dependencies for each block.
+  void dump() {
+CFG *cfg = PostDomTree.getCFG();

kuhar wrote:
> Can `dump` be const? 
In LLVM, `dump`s are usually annotated with the `LLVM_DUMP_METHOD` attribute 
and not compiled in release builds. Is the convention different in the static 
analyzer?


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][IDF] Add a control dependency calculator + a new debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:51
+  CFGDominatorTreeImpl(CFG *cfg) {
+buildDominatorTree(cfg);
+  }

DomTree has a constructor that runs the builder -- why not use it directly?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:160
   /// changes.
   void changeImmediateDominator(CFGBlock *N, CFGBlock *NewIDom) {
 DT.changeImmediateDominator(N, NewIDom);

Do you need it at all? I understand it's a wrapper around dominators, but this 
API is virtually impossible to use safely.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:225
+
+  /// Whether \p A is control dependent of \p B.
+  bool isControlDependent(CFGBlock *A, CFGBlock *B) {

I thinks it should be `A is control dependent on B`


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

https://reviews.llvm.org/D62619



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

[PATCH] D62619: [analyzer][Dominators] Add a control dependency tree builder + a new debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

You can easily get CD by calculating the PostDominanceFrontier. LLVM implements 
a templated IDF (Iterated Dominance Frontier) construction.
A native implementation for llvm ir for reference, if you need:

- 
https://github.com/seahorn/seahorn/blob/deep-dev-5.0/include/seahorn/Analysis/ControlDependenceAnalysis.hh
- 
https://github.com/seahorn/seahorn/blob/deep-dev-5.0/lib/Analysis/ControlDependenceAnalysis.cc

Paper: R. Cytron, J. Ferrante, B. K. Rosen, M. N. Wegman, and F. K. Zadeck. 
Efficiently
computing static single assignment form and the control dependence graph. ACM
Trans. Program. Lang. Syst., 13(4):451–490, Oct. 1991.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62619



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

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:95
+  CFGPostDomTree PostDom;
+  PostDom.buildDominatorTree(cfg);
+

Why not have a constructor that takes the cfg and constructs a domtree straight 
away? But this should probably go into a separate patch.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:113
+  EXPECT_TRUE(PostDom.dominates(ExitBlock, ExitBlock));
+  EXPECT_FALSE(Dom.properlyDominates(ExitBlock, ExitBlock));
+}

A tastcase with a virtual root postdominating other nodes would be welcome.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62611



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

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:37
+TEST(CFGDominatorTree, DomTree) {
+  const char *Code = "enum Kind {\n"
+ "  A\n"

You can use a raw string literal to make it more readable: 
https://en.cppreference.com/w/cpp/language/string_literal


Repository:
  rC Clang

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

https://reviews.llvm.org/D62611



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

[PATCH] D62551: [analyzer][Dominator] Add post dominator tree builder for the CFG + a debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:111
+  assert(
+  (IDom && !IDom->getBlock() && *I == &(*I)->getParent()->getExit() &&
+   IsPostDom) ||

kuhar wrote:
> Szelethus wrote:
> > kuhar wrote:
> > > Assertions with multiple conditions conjoined are difficult to debug -- 
> > > perhaps it'd be better to split them and assign to separate booleans for 
> > > each condition, with descriptive names? This code can be hidden behind an 
> > > ifdef for debug. 
> > Do you like this better? I prefer running static analysis on large 
> > codebases with a Release+Asserts build, and I fear that this check will be 
> > lost if I use `#ifdef DEBUG`.
> I think it's much more readable now, thanks! Release+Asserts defines NDEBUG 
> and the assertion will work if you put it into an ifdef.
*does not define NDEBUG


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

https://reviews.llvm.org/D62551



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

[PATCH] D62551: [analyzer][Dominator] Add post dominator tree builder for the CFG + a debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:111
+  assert(
+  (IDom && !IDom->getBlock() && *I == &(*I)->getParent()->getExit() &&
+   IsPostDom) ||

Szelethus wrote:
> kuhar wrote:
> > Assertions with multiple conditions conjoined are difficult to debug -- 
> > perhaps it'd be better to split them and assign to separate booleans for 
> > each condition, with descriptive names? This code can be hidden behind an 
> > ifdef for debug. 
> Do you like this better? I prefer running static analysis on large codebases 
> with a Release+Asserts build, and I fear that this check will be lost if I 
> use `#ifdef DEBUG`.
I think it's much more readable now, thanks! Release+Asserts defines NDEBUG and 
the assertion will work if you put it into an ifdef.


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

https://reviews.llvm.org/D62551



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

[PATCH] D62551: [analyzer][Dominator] Add post dominator tree builder for the CFG + a debug checker

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:39
 
 /// Concrete subclass of DominatorTreeBase for Clang
 /// This class implements the dominators tree functionality given a Clang CFG.

Seems outdated?



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:47
 public:
-  llvm::DomTreeBase *DT;
+  using DomTreeBase = llvm::DominatorTreeBase;
 

nit: I think this can name can be somewhat confusing as it's not a base class 
of CFGDomTree.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:49
 
-  DominatorTree() {
-DT = new llvm::DomTreeBase();
+  DomTreeBase *DT;
+

Why not  have it as a value member? Or a unique_ptr, if pointer semantics are 
really desired.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:111
+  assert(
+  (IDom && !IDom->getBlock() && *I == &(*I)->getParent()->getExit() &&
+   IsPostDom) ||

Assertions with multiple conditions conjoined are difficult to debug -- perhaps 
it'd be better to split them and assign to separate booleans for each 
condition, with descriptive names? This code can be hidden behind an ifdef for 
debug. 



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:174
 
+using DominatorTree = CFGDominatorTree;
+using PostDominatorTree = CFGDominatorTree;

nit: How about having the parent class template called CFGDominatorTreeImpl and 
these two as CFGDomTree and CFGPostdomTree?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62551



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

[PATCH] D62507: [Dominators] PR42041: Skip nullpointer successors

[Jakub Kuderski via Phabricator via cfe-commits]

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

Thanks, I think this is a much better solution than directly hacking on 
DomTreeBuilder.

Note that every time you ask DomTree about a `nullptr` CFG node, it understands 
it as a request for a virtual root. While now this should crash on forward 
dominators, you will get an actual tree node for a nullptr in postdominators. 
In the future, it may happen that forward dominators will also have a virtual 
root (for more efficient updates) and the users of clang CFG and dominators 
should be aware that not every CFG node (i.e., an unreachable successor) has a 
corresponding DomTreeNode.


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

https://reviews.llvm.org/D62507



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

[PATCH] D62507: [Dominators] PR42041: Skip nullpointer successors

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/Dominators.h:190
+clang::CFGBlock *N, std::integral_constant) {
+  auto RChildren = reverse(children(N));
+  ResultTy Ret(RChildren.begin(), RChildren.end());

Shouldn't one of the cases use `inverse_children`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62507



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

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Thank you for the comments, Richard!

In https://reviews.llvm.org/D51200#1223745, @rsmith wrote:

> Have you considered whether the builtin should apply to `new` expressions? 
> (There are potentially three different top-level calls implied by a `new` 
> expression -- an `operator new`, a constructor, and an `operator delete` -- 
> so it's not completely obvious what effect this would have there.) And 
> likewise for `delete`.


I haven't thought about it, but to be honest I'm not sure what should happen. 
Intuitively, I think these 3 options would make most sense, listed from the 
most to the least reasonable in my opionion:

  1. apply the corresponding attribute to all of the implied calls
  2. apply to constructor/destructor only
1. don't handle it at all -- emit an error

How would you suggest to handle it?


https://reviews.llvm.org/D51200



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

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

In https://reviews.llvm.org/D51200#1223752, @rsmith wrote:

> +rjmccall for CodeGen changes


@rsmith @rjmccall 
I have one high-level question about the CodeGen part that I wasn't able to 
figure out: is it possible to bail out of custom CodeGen in CGBuiltin and 
somehow say: emit whatever is inside (IE treat the builtin call node as a 
no-op)?


https://reviews.llvm.org/D51200



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

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 162520.

https://reviews.llvm.org/D51200

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/CodeGen/CGFunctionInfo.h
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprCXX.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenTypes.h
  lib/Sema/SemaChecking.cpp
  test/CodeGenCXX/inline-builtin-call.cpp
  test/Sema/inline-builtins.c
  test/SemaCXX/inline-builtins.cpp

Index: test/SemaCXX/inline-builtins.cpp
===
--- /dev/null
+++ test/SemaCXX/inline-builtins.cpp
@@ -0,0 +1,128 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+
+struct S {
+  S();
+  void foo();
+  void bar(int);
+  int baz(float, char);
+  static void lol();
+
+  virtual void virt();
+  void operator++();
+  friend S operator+(const S &, const S &);
+};
+
+S operator"" _s(unsigned long long);
+
+S &getS();
+
+void test_positive() {
+  S &s = getS();
+  __builtin_always_inline(s.foo());
+  __builtin_no_inline(s.foo());
+
+  __builtin_always_inline(getS().foo());
+  __builtin_no_inline(getS().foo());
+
+  __builtin_always_inline(getS()).foo();
+  __builtin_no_inline(getS()).foo();
+
+  __builtin_always_inline(s.bar(1));
+  __builtin_no_inline(s.bar(1));
+
+  int a = __builtin_always_inline(s.baz(3.14, 'a'));
+  int b = __builtin_no_inline(s.baz(3.14, 'a'));
+
+  __builtin_always_inline(s.bar(s.baz(3.14, 'a')));
+  __builtin_no_inline(s.bar(s.baz(3.14, 'a')));
+
+  s.bar(__builtin_always_inline(s.baz(3.14, 'a')));
+  s.bar(__builtin_no_inline(s.baz(3.14, 'a')));
+
+  __builtin_always_inline(s.lol());
+  __builtin_no_inline(s.lol());
+
+  __builtin_always_inline(S::lol());
+  __builtin_no_inline(S::lol());
+
+  __builtin_always_inline(s.virt());
+  __builtin_no_inline(s.virt());
+
+  __builtin_always_inline(++s);
+  __builtin_no_inline(++s);
+
+  __builtin_always_inline(s.operator++());
+  __builtin_no_inline(s.operator++());
+
+  S s1;
+  __builtin_always_inline(s + s1);
+  __builtin_no_inline(s + s1);
+
+  auto mptr = &S::foo;
+  __builtin_always_inline((s.*mptr)());
+  __builtin_no_inline((s.*mptr)());
+
+  __builtin_always_inline(s.~S());
+  __builtin_no_inline(s.~S());
+
+  __builtin_always_inline([] {}());
+  __builtin_no_inline([] {}());
+
+  auto lam = [&a, b](S &ss) { return ++a + b; };
+  int c = __builtin_always_inline(lam(s1));
+  int d = __builtin_no_inline(lam(s1));
+}
+
+template 
+void callDtorAlways(T &t) {
+  __builtin_always_inline(t.~T()); // expected-error {{argument to __builtin_always_inline must not be a pseudo-destructor call}}
+}
+template 
+void callDtorNo(T &t) {
+  __builtin_no_inline(t.~T()); // expected-error {{argument to __builtin_no_inline must not be a pseudo-destructor call}}
+}
+
+void test_errors() {
+  using I = int;
+  I e = 1;
+  __builtin_always_inline(e.~I()); // expected-error {{argument to __builtin_always_inline must not be a pseudo-destructor call}}
+  __builtin_no_inline(e.~I()); // expected-error {{argument to __builtin_no_inline must not be a pseudo-destructor call}}
+
+  S s;
+  callDtorAlways(s);
+  callDtorNo(s);
+
+  callDtorAlways(e); // expected-note {{in instantiation of function template specialization 'callDtorAlways' requested here}}
+  callDtorNo(e); // expected-note {{in instantiation of function template specialization 'callDtorNo' requested here}}
+
+  // TODO: Support User Defined Literals.
+  S s2 = __builtin_always_inline(1_s); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  s2 = __builtin_no_inline(1_s);   // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+
+  // TODO: This should be handled as well.
+  S s3 = __builtin_always_inline(S()); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  S s4 = __builtin_no_inline(S()); // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+  S s5 = __builtin_always_inline(S{}); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  S s6 = __builtin_no_inline(S{}); // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+}
+
+constexpr int f1() { return 1; }
+
+// TODO: It should be possible to make the inline intrinsics constexpr-friendly.
+constexpr int f2(int x) {   // expected-error {{constexpr function never produces a constant expression}}
+  return x + __builtin_always_inline(f1()); // expected-note {{subexpression not valid in a constant expression}}
+}
+
+template 
+struct IsSame { static constexpr bool value = false; };
+
+template 
+struct IsSame { static constexpr bool value = true; };
+
+void test_unevaluated_context() {
+  static_assert(

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 162519.
kuhar added a comment.

Rename missed CallInlineKind parameters to CIK


https://reviews.llvm.org/D51200

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/CodeGen/CGFunctionInfo.h
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprCXX.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenTypes.h
  lib/Sema/SemaChecking.cpp
  test/CodeGenCXX/inline-builtin-call.cpp
  test/Sema/inline-builtins.c
  test/SemaCXX/inline-builtins.cpp

Index: test/SemaCXX/inline-builtins.cpp
===
--- /dev/null
+++ test/SemaCXX/inline-builtins.cpp
@@ -0,0 +1,128 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+
+struct S {
+  S();
+  void foo();
+  void bar(int);
+  int baz(float, char);
+  static void lol();
+
+  virtual void virt();
+  void operator++();
+  friend S operator+(const S &, const S &);
+};
+
+S operator"" _s(unsigned long long);
+
+S &getS();
+
+void test_positive() {
+  S &s = getS();
+  __builtin_always_inline(s.foo());
+  __builtin_no_inline(s.foo());
+
+  __builtin_always_inline(getS().foo());
+  __builtin_no_inline(getS().foo());
+
+  __builtin_always_inline(getS()).foo();
+  __builtin_no_inline(getS()).foo();
+
+  __builtin_always_inline(s.bar(1));
+  __builtin_no_inline(s.bar(1));
+
+  int a = __builtin_always_inline(s.baz(3.14, 'a'));
+  int b = __builtin_no_inline(s.baz(3.14, 'a'));
+
+  __builtin_always_inline(s.bar(s.baz(3.14, 'a')));
+  __builtin_no_inline(s.bar(s.baz(3.14, 'a')));
+
+  s.bar(__builtin_always_inline(s.baz(3.14, 'a')));
+  s.bar(__builtin_no_inline(s.baz(3.14, 'a')));
+
+  __builtin_always_inline(s.lol());
+  __builtin_no_inline(s.lol());
+
+  __builtin_always_inline(S::lol());
+  __builtin_no_inline(S::lol());
+
+  __builtin_always_inline(s.virt());
+  __builtin_no_inline(s.virt());
+
+  __builtin_always_inline(++s);
+  __builtin_no_inline(++s);
+
+  __builtin_always_inline(s.operator++());
+  __builtin_no_inline(s.operator++());
+
+  S s1;
+  __builtin_always_inline(s + s1);
+  __builtin_no_inline(s + s1);
+
+  auto mptr = &S::foo;
+  __builtin_always_inline((s.*mptr)());
+  __builtin_no_inline((s.*mptr)());
+
+  __builtin_always_inline(s.~S());
+  __builtin_no_inline(s.~S());
+
+  __builtin_always_inline([] {}());
+  __builtin_no_inline([] {}());
+
+  auto lam = [&a, b](S &ss) { return ++a + b; };
+  int c = __builtin_always_inline(lam(s1));
+  int d = __builtin_no_inline(lam(s1));
+}
+
+template 
+void callDtorAlways(T &t) {
+  __builtin_always_inline(t.~T()); // expected-error {{argument to __builtin_always_inline must not be a pseudo-destructor call}}
+}
+template 
+void callDtorNo(T &t) {
+  __builtin_no_inline(t.~T()); // expected-error {{argument to __builtin_no_inline must not be a pseudo-destructor call}}
+}
+
+void test_errors() {
+  using I = int;
+  I e = 1;
+  __builtin_always_inline(e.~I()); // expected-error {{argument to __builtin_always_inline must not be a pseudo-destructor call}}
+  __builtin_no_inline(e.~I()); // expected-error {{argument to __builtin_no_inline must not be a pseudo-destructor call}}
+
+  S s;
+  callDtorAlways(s);
+  callDtorNo(s);
+
+  callDtorAlways(e); // expected-note {{in instantiation of function template specialization 'callDtorAlways' requested here}}
+  callDtorNo(e); // expected-note {{in instantiation of function template specialization 'callDtorNo' requested here}}
+
+  // TODO: Support User Defined Literals.
+  S s2 = __builtin_always_inline(1_s); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  s2 = __builtin_no_inline(1_s);   // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+
+  // TODO: This should be handled as well.
+  S s3 = __builtin_always_inline(S()); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  S s4 = __builtin_no_inline(S()); // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+  S s5 = __builtin_always_inline(S{}); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  S s6 = __builtin_no_inline(S{}); // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+}
+
+constexpr int f1() { return 1; }
+
+// TODO: It should be possible to make the inline intrinsics constexpr-friendly.
+constexpr int f2(int x) {   // expected-error {{constexpr function never produces a constant expression}}
+  return x + __builtin_always_inline(f1()); // expected-note {{subexpression not valid in a constant expression}}
+}
+
+template 
+struct IsSame { static constexpr bool value = false; };
+
+template 
+struct IsSame { static constexpr bool

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8236
+def err_argument_to_inline_intrinsic_builtin_call : Error<
+  "argument to %0 must not be a builtin call">;
+def err_argument_to_inline_intrinsic_pdotr_call : Error<

Quuxplusone wrote:
> I'd expect to be able to write
> 
> __builtin_always_inline(sqrt(x))
> __builtin_no_inline(sqrt(x))
> 
> without caring whether `sqrt` was a real function or just a macro around 
> `__builtin_sqrt`. How important is it that calls to builtin functions be 
> errors, instead of just being ignored for this purpose?
Very good point. Initially I thought this should be an error, but I think that 
since compiler can reason about intrinsics  anyway, it makes sense to treat 
inline intrinsics as NoOps in this case.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8238
+def err_argument_to_inline_intrinsic_pdotr_call : Error<
+  "argument to %0 must not be a pseudo-destructor call">;
+def err_argument_to_inline_intrinsic_block_call : Error<

Quuxplusone wrote:
> Spelling: `s/dotr/dtor/`
> Semantics: Does this trigger in generic contexts? I feel like I'd want to be 
> able to write
> 
> __builtin_always_inline(p->~T());
> 
> without caring whether `T` was a primitive type or not. How important is it 
> that pseudo-destructor calls be errors, instead of just being ignored for 
> this purpose?
I thought about it an initially disliked the idea of of silently accepting code 
that cannot be inlined (because there's no function call).

This case is more general, because you can have the same problem with operator 
calls.
Consider a function template that does `__builtin_always_inline(t + t)`. It 
makes sense to inline it for some custom type that implements the + operator, 
but it could be also used when t is e.g. an int. 

Given that I the intrinsics already works on the best-effort basis (and doesn't 
not strictly guarantee that inlining/no inlining will happen; e.g., indirect 
calls), and that in the future I also want to introduce a third intrinsic for 
transitively inlining everything, I think it makes sense to treat it as as NoOp 
in generic context.

When the context is not generic, maybe it still would make sense to emit an 
error/warning? I don't like the idea of being able to write 
`__builtin_always_inline(2 + 3)` and the compiler not complaining at all...  
What do you think?



Comment at: include/clang/CodeGen/CGFunctionInfo.h:713
+  CanQualType resultType, ArrayRef argTypes,
+  CallInlineKind inlineKind = CallInlineKind::DefaultInline) {
 ID.AddInteger(info.getCC());

Quuxplusone wrote:
> Here and throughout, wouldn't it be more traditional to name the parameter 
> variable `CIK`? (You have sometimes `CIK`, sometimes `inlineKind`, sometimes 
> `InlineKind`.)
> 
> It also feels needlessly convoluted to have a value of type CallInlineKind 
> stored in a member named InlineCall (note the reversal of the words), but I'm 
> not sure how that's generally dealt with.
Good suggestion, thanks!



Comment at: test/SemaCXX/inline-builtins.cpp:12
+  void operator++();
+  friend S operator+(const S &, const S &);
+};

Quuxplusone wrote:
> This raises a question for me about the semantics of "always inlining" a 
> "call":
> 
> struct A { A(); A(A&&); };
> struct B { B(A); }
> void foo(B);
> 
> __builtin_always_inline(foo(A{}));
> 
> Does `__builtin_always_inline` apply to `foo`, `B(A)` (used to create the 
> parameter to `foo`), `A()` (used to create the argument to `A(A&&)`), or all 
> of the above?
> You should add a test case something like this, maybe with multiple function 
> arguments.
It should apply to the outermost call, e.g.:
`__builtin_always_inline(A(B(C(` -- applies to `A(...)`
`A(__builtin_always_inline(B(C(` --  applies to `B(...)`
`__builtin_always_inline(a.foo().bar())` -- applies to `.bar()`
`__builtin_always_inline(a.foo()).bar()` -- applies to `.foo()`

I added extra testcases for this. Ultimately, I believe that these intrinsic 
deserve a solid documentation on the clang www for builtins.



Comment at: test/SemaCXX/inline-builtins.cpp:12
+  void operator++();
+  friend S operator+(const S &, const S &);
+};

kuhar wrote:
> Quuxplusone wrote:
> > This raises a question for me about the semantics of "always inlining" a 
> > "call":
> > 
> > struct A { A(); A(A&&); };
> > struct B { B(A); }
> > void foo(B);
> > 
> > __builtin_always_inline(foo(A{}));
> > 
> > Does `__builtin_always_inline` apply to `foo`, `B(A)` (used to create the 
> > parameter to `foo`), `A()` (used to create the argument to `A(A&&)`), or 
> > all of the above?
> > You should add a test case something like this, maybe with multiple 
> > function arguments.
> It should apply to the outermost call, e.g.:
> `__builtin_always_

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 162495.
kuhar marked an inline comment as done.
kuhar added a comment.

Fix naming and add more tests.


Repository:
  rC Clang

https://reviews.llvm.org/D51200

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/CodeGen/CGFunctionInfo.h
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprCXX.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenTypes.h
  lib/Sema/SemaChecking.cpp
  test/CodeGenCXX/inline-builtin-call.cpp
  test/Sema/inline-builtins.c
  test/SemaCXX/inline-builtins.cpp

Index: test/SemaCXX/inline-builtins.cpp
===
--- /dev/null
+++ test/SemaCXX/inline-builtins.cpp
@@ -0,0 +1,128 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+
+struct S {
+  S();
+  void foo();
+  void bar(int);
+  int baz(float, char);
+  static void lol();
+
+  virtual void virt();
+  void operator++();
+  friend S operator+(const S &, const S &);
+};
+
+S operator"" _s(unsigned long long);
+
+S &getS();
+
+void test_positive() {
+  S &s = getS();
+  __builtin_always_inline(s.foo());
+  __builtin_no_inline(s.foo());
+
+  __builtin_always_inline(getS().foo());
+  __builtin_no_inline(getS().foo());
+
+  __builtin_always_inline(getS()).foo();
+  __builtin_no_inline(getS()).foo();
+
+  __builtin_always_inline(s.bar(1));
+  __builtin_no_inline(s.bar(1));
+
+  int a = __builtin_always_inline(s.baz(3.14, 'a'));
+  int b = __builtin_no_inline(s.baz(3.14, 'a'));
+
+  __builtin_always_inline(s.bar(s.baz(3.14, 'a')));
+  __builtin_no_inline(s.bar(s.baz(3.14, 'a')));
+
+  s.bar(__builtin_always_inline(s.baz(3.14, 'a')));
+  s.bar(__builtin_no_inline(s.baz(3.14, 'a')));
+
+  __builtin_always_inline(s.lol());
+  __builtin_no_inline(s.lol());
+
+  __builtin_always_inline(S::lol());
+  __builtin_no_inline(S::lol());
+
+  __builtin_always_inline(s.virt());
+  __builtin_no_inline(s.virt());
+
+  __builtin_always_inline(++s);
+  __builtin_no_inline(++s);
+
+  __builtin_always_inline(s.operator++());
+  __builtin_no_inline(s.operator++());
+
+  S s1;
+  __builtin_always_inline(s + s1);
+  __builtin_no_inline(s + s1);
+
+  auto mptr = &S::foo;
+  __builtin_always_inline((s.*mptr)());
+  __builtin_no_inline((s.*mptr)());
+
+  __builtin_always_inline(s.~S());
+  __builtin_no_inline(s.~S());
+
+  __builtin_always_inline([] {}());
+  __builtin_no_inline([] {}());
+
+  auto lam = [&a, b](S &ss) { return ++a + b; };
+  int c = __builtin_always_inline(lam(s1));
+  int d = __builtin_no_inline(lam(s1));
+}
+
+template 
+void callDtorAlways(T &t) {
+  __builtin_always_inline(t.~T()); // expected-error {{argument to __builtin_always_inline must not be a pseudo-destructor call}}
+}
+template 
+void callDtorNo(T &t) {
+  __builtin_no_inline(t.~T()); // expected-error {{argument to __builtin_no_inline must not be a pseudo-destructor call}}
+}
+
+void test_errors() {
+  using I = int;
+  I e = 1;
+  __builtin_always_inline(e.~I()); // expected-error {{argument to __builtin_always_inline must not be a pseudo-destructor call}}
+  __builtin_no_inline(e.~I()); // expected-error {{argument to __builtin_no_inline must not be a pseudo-destructor call}}
+
+  S s;
+  callDtorAlways(s);
+  callDtorNo(s);
+
+  callDtorAlways(e); // expected-note {{in instantiation of function template specialization 'callDtorAlways' requested here}}
+  callDtorNo(e); // expected-note {{in instantiation of function template specialization 'callDtorNo' requested here}}
+
+  // TODO: Support User Defined Literals.
+  S s2 = __builtin_always_inline(1_s); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  s2 = __builtin_no_inline(1_s);   // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+
+  // TODO: This should be handled as well.
+  S s3 = __builtin_always_inline(S()); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  S s4 = __builtin_no_inline(S()); // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+  S s5 = __builtin_always_inline(S{}); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  S s6 = __builtin_no_inline(S{}); // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+}
+
+constexpr int f1() { return 1; }
+
+// TODO: It should be possible to make the inline intrinsics constexpr-friendly.
+constexpr int f2(int x) {   // expected-error {{constexpr function never produces a constant expression}}
+  return x + __builtin_always_inline(f1()); // expected-note {{subexpression not valid in a constant expression}}
+}
+
+template 
+struct IsSame { static constexpr bool value = false; };
+
+

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

One extra question: do you think this deserves a separate RFC?


Repository:
  rC Clang

https://reviews.llvm.org/D51200



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

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Disclaimer: I'm a Clang newbie and I'm not sure if that's a good way to 
implement these intrinsics. I'm not sure about the following things:

- The new enum CallInlineKind may not be in the right place
- Not sure if adding the extra parameter to EmitSomething methods is the 
cleanest thing possible -- some functions have it now, some don't, and it makes 
argument lists longer
- Not sure if just adding an extra attribute at each callsite is enough -- I'm 
not sure what is supposed to happen when there are conflicting inline 
attributes at call sites and function declarations (e.g. 
`__builtin_always_inline` and `__attribute__ ((always_inline))`)
- I don't know how to handle constexpr functions
- I don't know how to implement it for constructor calls

I would really appreciate any tips here :).


Repository:
  rC Clang

https://reviews.llvm.org/D51200



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

[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added reviewers: rsmith, pcc, Prazek, sanjoy.
kuhar added a project: clang.
Herald added a subscriber: eraman.
Herald added a reviewer: grosser.

Traditionally, to force some inlining decisions one has to annotate function 
declarations with `__attribute__((always_inline))` or 
`__attribute__((noinline))`. One problem with these attributes is that they 
affect every call site. Always inlining or forbidding inlining may not be 
desirable in every context, and a workaround for that is creating a few copies 
of functions, each with a different inline attribute. Furthermore, it's not 
always feasible (in every project) to modify library code and introduce new 
function attributes there.

This patch introduces a new way of forcing inlining decisions on a per-callsite 
basis. This allows for more fine-grained control over inlining, without 
creating any duplicate functions. The two new intrinsics for controlling 
inlining are:

- `__builtin_always_inline(Foo())` -- inlines the function `Foo` at the 
callsite, if possible. Internally, this applies the `alwaysinline` attribute to 
the generated call instruction.
- `__builtin_no_inline(Foo())` -- forbids the function `Foo` to be inlined at 
the callsite. Internally, this applies the `noinline` attribute to the 
generated call instruction.

The inline intrinsics support function, function pointer, member function, 
member function pointer, virutal function, and operator calls. Support for 
constructor calls (`CXXTemporaryExpr`) should also be possible, but is not the 
part of this patch.


Repository:
  rC Clang

https://reviews.llvm.org/D51200

Files:
  include/clang/Basic/Builtins.def
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/CodeGen/CGFunctionInfo.h
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprCXX.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenTypes.h
  lib/Sema/SemaChecking.cpp
  test/CodeGenCXX/inline_builtin_call.cpp
  test/Sema/inline-builtins.c
  test/SemaCXX/inline-builtins.cpp

Index: test/SemaCXX/inline-builtins.cpp
===
--- /dev/null
+++ test/SemaCXX/inline-builtins.cpp
@@ -0,0 +1,83 @@
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s
+
+struct S {
+  S();
+  void foo();
+  void bar(int);
+  int baz(float, char);
+  static void lol();
+
+  virtual void virt();
+  void operator++();
+  friend S operator+(const S &, const S &);
+};
+
+S operator"" _s(unsigned long long);
+
+S &getS();
+
+void test_positive() {
+  S &s = getS();
+  __builtin_always_inline(s.foo());
+  __builtin_no_inline(s.foo());
+
+  __builtin_always_inline(s.bar(1));
+  __builtin_no_inline(s.bar(1));
+
+  int a = __builtin_always_inline(s.baz(3.14, 'a'));
+  int b = __builtin_no_inline(s.baz(3.14, 'a'));
+
+  __builtin_always_inline(s.lol());
+  __builtin_no_inline(s.lol());
+
+  __builtin_always_inline(S::lol());
+  __builtin_no_inline(S::lol());
+
+  __builtin_always_inline(s.virt());
+  __builtin_no_inline(s.virt());
+
+  __builtin_always_inline(++s);
+  __builtin_no_inline(++s);
+
+  __builtin_always_inline(s.operator++());
+  __builtin_no_inline(s.operator++());
+
+  S s1;
+  __builtin_always_inline(s + s1);
+  __builtin_no_inline(s + s1);
+
+  auto mptr = &S::foo;
+  __builtin_always_inline((s.*mptr)());
+  __builtin_no_inline((s.*mptr)());
+
+  __builtin_always_inline(s.~S());
+  __builtin_no_inline(s.~S());
+
+  __builtin_always_inline([] {}());
+  __builtin_no_inline([] {}());
+
+  auto lam = [&a, b](S &ss) { return ++a + b; };
+  int c = __builtin_always_inline(lam(s1));
+  int d = __builtin_no_inline(lam(s1));
+}
+
+void test_errors() {
+  using I = int;
+  I e = 1;
+  __builtin_always_inline(e.~I()); // expected-error {{argument to __builtin_always_inline must not be a pseudo-destructor call}}
+  __builtin_no_inline(e.~I()); // expected-error {{argument to __builtin_no_inline must not be a pseudo-destructor call}}
+
+  S s2 = __builtin_always_inline(1_s); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  s2 = __builtin_no_inline(1_s);   // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+
+  // TODO: This should be handled as well.
+  S s3 = __builtin_always_inline(S()); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  S s4 = __builtin_no_inline(S()); // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+}
+
+constexpr int f1() { return 1; }
+
+// TODO: It should be possible to make the inline intrinsics constexpr-friendly.
+constexpr int f2(int x) {   // expected-error {{constexpr function never produces a constant expression}}
+  return x + __builtin_always_inline(f1()); // expected-note {{subexpression not valid in a consta

[PATCH] D32690: [clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 99103.
kuhar added a comment.

The patch broke the spinx docs build and I had to revert it in r303140.
I fixed the docs file problems.


Repository:
  rL LLVM

https://reviews.llvm.org/D32690

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -1,7 +1,12 @@
 // RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN: [{key: modernize-use-emplace.ContainersWithPushBack, \
-// RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
+// RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}, \
+// RUN:  {key: modernize-use-emplace.TupleTypes, \
+// RUN:   value: '::std::pair; std::tuple; ::test::Single'}, \
+// RUN:  {key: modernize-use-emplace.TupleMakeFunctions, \
+// RUN:   value: '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}] \
+// RUN: }" -- -std=c++11
 
 namespace std {
 template 
@@ -46,27 +51,54 @@
   ~deque();
 };
 
-template 
-class pair {
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+
+template  class pair {
 public:
   pair() = default;
   pair(const pair &) = default;
   pair(pair &&) = default;
 
   pair(const T1 &, const T2 &) {}
   pair(T1 &&, T2 &&) {}
 
-  template 
-  pair(const pair &p){};
-  template 
-  pair(pair &&p){};
+  template  pair(const pair &){};
+  template  pair(pair &&){};
 };
 
 template 
-pair make_pair(T1&&, T2&&) {
+pair::type, typename remove_reference::type>
+make_pair(T1 &&, T2 &&) {
   return {};
 };
 
+template  class tuple {
+public:
+  tuple() = default;
+  tuple(const tuple &) = default;
+  tuple(tuple &&) = default;
+
+  tuple(const Ts &...) {}
+  tuple(Ts &&...) {}
+
+  template  tuple(const tuple &){};
+  template  tuple(tuple &&) {}
+
+  template  tuple(const pair &) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+  template  tuple(pair &&) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+};
+
+template 
+tuple::type...> make_tuple(Ts &&...) {
+  return {};
+}
+
 template 
 class unique_ptr {
 public:
@@ -207,6 +239,30 @@
   // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37)));
 }
 
+void testTuple() {
+  std::vector> v;
+  v.push_back(std::tuple(false, 'x', 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'x', 1);
+
+  v.push_back(std::tuple{false, 'y', 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'y', 2);
+
+  v.push_back({true, 'z', 3});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(true, 'z', 3);
+
+  std::vector> x;
+  x.push_back(std::make_pair(1, false));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(1, false);
+
+  x.push_back(std::make_pair(2LL, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(2LL, 1);
+}
+
 struct Base {
   Base(int, int *, int = 42);
 };
@@ -328,6 +384,77 @@
   // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
+void testMakeTuple() {
+  std::vector> v;
+  v.push_back(std::make_tuple(1, true, 'v'));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, true, 'v');
+
+  v.push_back(std::make_tuple(2ULL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(2ULL, 1, 0);
+
+  v.push_back(std::make_tuple(3LL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_tuple(3LL, 1, 0));
+  // make_tuple is not removed when there are explicit template
+  // arguments provided.
+}
+
+namespace test {
+template  struct Single {
+  Single() = default;
+  Single(const Single &) = default;
+  Single(Single &&) = default;
+
+  Single(const T &) {}
+  Single(T &&) {}
+
+  template  Single(const Single &) {}
+  template  Single(Single &&) {}
+
+  template  Single(const std::tuple &) {}
+  template  Single(std::tuple &&) {}
+};
+
+template 
+Single::type> MakeSingle(T &&) {
+  return {};
+}
+} // namespace test
+
+void testOtherTuples() {
+  std::vector> v;
+  v.push_back(test::Single(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_b

[PATCH] D32690: [clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL303139: [clang-tidy] modernize-use-emplace: Remove 
unnecessary make_tuple calls (authored by kuhar).

Changed prior to commit:
  https://reviews.llvm.org/D32690?vs=98073&id=99100#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32690

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp

Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -24,14 +24,20 @@
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
 "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
+const auto DefaultTupleTypes = "::std::pair; ::std::tuple";
+const auto DefaultTupleMakeFunctions = "::std::make_pair; ::std::make_tuple";
 } // namespace
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   ContainersWithPushBack(utils::options::parseStringList(Options.get(
   "ContainersWithPushBack", DefaultContainersWithPushBack))),
   SmartPointers(utils::options::parseStringList(
-  Options.get("SmartPointers", DefaultSmartPointers))) {}
+  Options.get("SmartPointers", DefaultSmartPointers))),
+  TupleTypes(utils::options::parseStringList(
+  Options.get("TupleTypes", DefaultTupleTypes))),
+  TupleMakeFunctions(utils::options::parseStringList(
+  Options.get("TupleMakeFunctions", DefaultTupleMakeFunctions))) {}
 
 void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
@@ -87,20 +93,23 @@
   .bind("ctor");
   auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
 
-  auto MakePair = ignoringImplicit(
-  callExpr(callee(expr(ignoringImplicit(
-  declRefExpr(unless(hasExplicitTemplateArgs()),
-  to(functionDecl(hasName("::std::make_pair"
-  .bind("make_pair"));
+  auto MakeTuple = ignoringImplicit(
+  callExpr(
+  callee(expr(ignoringImplicit(declRefExpr(
+  unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasAnyName(SmallVector(
+  TupleMakeFunctions.begin(), TupleMakeFunctions.end())
+  .bind("make"));
 
-  // make_pair can return type convertible to container's element type.
+  // make_something can return type convertible to container's element type.
   // Allow the conversion only on containers of pairs.
-  auto MakePairCtor = ignoringImplicit(cxxConstructExpr(
-  has(materializeTemporaryExpr(MakePair)),
-  hasDeclaration(cxxConstructorDecl(ofClass(hasName("::std::pair"));
+  auto MakeTupleCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(MakeTuple)),
+  hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
+  SmallVector(TupleTypes.begin(), TupleTypes.end(;
 
   auto SoughtParam = materializeTemporaryExpr(
-  anyOf(has(MakePair), has(MakePairCtor),
+  anyOf(has(MakeTuple), has(MakeTupleCtor),
 HasConstructExpr, has(cxxFunctionalCastExpr(HasConstructExpr;
 
   Finder->addMatcher(cxxMemberCallExpr(CallPushBack, has(SoughtParam),
@@ -112,8 +121,8 @@
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Call = Result.Nodes.getNodeAs("call");
   const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor");
-  const auto *MakePairCall = Result.Nodes.getNodeAs("make_pair");
-  assert((InnerCtorCall || MakePairCall) && "No push_back parameter matched");
+  const auto *MakeCall = Result.Nodes.getNodeAs("make");
+  assert((InnerCtorCall || MakeCall) && "No push_back parameter matched");
 
   const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
   Call->getExprLoc(), Call->getArg(0)->getExprLoc());
@@ -123,20 +132,20 @@
   if (FunctionNameSourceRange.getBegin().isMacroID())
 return;
 
-  const auto *EmplacePrefix = MakePairCall ? "emplace_back" : "emplace_back(";
+  const auto *EmplacePrefix = MakeCall ? "emplace_back" : "emplace_back(";
   Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, EmplacePrefix);
 
   const SourceRange CallParensRange =
-  MakePairCall ? SourceRange(MakePairCall->getCallee()->getLocEnd(),
- MakePairCall->getRParenLoc())
-   : InnerCtorCall->getParenOrBraceRange();
+  MakeCall ? SourceRange(MakeCall->getCallee()->getLocEnd(),
+ MakeCall->getRParenLoc()

[PATCH] D32690: [clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 98073.
kuhar added a comment.
Herald added a subscriber: xazax.hun.

I updated the patch against the current trunk.

I also run the modernize-use-emplace check on llvm and verified that there are 
no new regressions after applying fixits.


https://reviews.llvm.org/D32690

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -1,7 +1,12 @@
 // RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN: [{key: modernize-use-emplace.ContainersWithPushBack, \
-// RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
+// RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}, \
+// RUN:  {key: modernize-use-emplace.TupleTypes, \
+// RUN:   value: '::std::pair; std::tuple; ::test::Single'}, \
+// RUN:  {key: modernize-use-emplace.TupleMakeFunctions, \
+// RUN:   value: '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}] \
+// RUN: }" -- -std=c++11
 
 namespace std {
 template 
@@ -46,27 +51,54 @@
   ~deque();
 };
 
-template 
-class pair {
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+
+template  class pair {
 public:
   pair() = default;
   pair(const pair &) = default;
   pair(pair &&) = default;
 
   pair(const T1 &, const T2 &) {}
   pair(T1 &&, T2 &&) {}
 
-  template 
-  pair(const pair &p){};
-  template 
-  pair(pair &&p){};
+  template  pair(const pair &){};
+  template  pair(pair &&){};
 };
 
 template 
-pair make_pair(T1&&, T2&&) {
+pair::type, typename remove_reference::type>
+make_pair(T1 &&, T2 &&) {
   return {};
 };
 
+template  class tuple {
+public:
+  tuple() = default;
+  tuple(const tuple &) = default;
+  tuple(tuple &&) = default;
+
+  tuple(const Ts &...) {}
+  tuple(Ts &&...) {}
+
+  template  tuple(const tuple &){};
+  template  tuple(tuple &&) {}
+
+  template  tuple(const pair &) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+  template  tuple(pair &&) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+};
+
+template 
+tuple::type...> make_tuple(Ts &&...) {
+  return {};
+}
+
 template 
 class unique_ptr {
 public:
@@ -207,6 +239,30 @@
   // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37)));
 }
 
+void testTuple() {
+  std::vector> v;
+  v.push_back(std::tuple(false, 'x', 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'x', 1);
+
+  v.push_back(std::tuple{false, 'y', 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'y', 2);
+
+  v.push_back({true, 'z', 3});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(true, 'z', 3);
+
+  std::vector> x;
+  x.push_back(std::make_pair(1, false));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(1, false);
+
+  x.push_back(std::make_pair(2LL, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(2LL, 1);
+}
+
 struct Base {
   Base(int, int *, int = 42);
 };
@@ -328,6 +384,77 @@
   // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
+void testMakeTuple() {
+  std::vector> v;
+  v.push_back(std::make_tuple(1, true, 'v'));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, true, 'v');
+
+  v.push_back(std::make_tuple(2ULL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(2ULL, 1, 0);
+
+  v.push_back(std::make_tuple(3LL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_tuple(3LL, 1, 0));
+  // make_tuple is not removed when there are explicit template
+  // arguments provided.
+}
+
+namespace test {
+template  struct Single {
+  Single() = default;
+  Single(const Single &) = default;
+  Single(Single &&) = default;
+
+  Single(const T &) {}
+  Single(T &&) {}
+
+  template  Single(const Single &) {}
+  template  Single(Single &&) {}
+
+  template  Single(const std::tuple &) {}
+  template  Single(std::tuple &&) {}
+};
+
+template 
+Single::type> MakeSingle(T &&) {
+  return {};
+}
+} // namespace test
+
+void testOtherTuples() {
+  std::vector> v;
+  v.push_back(test::Single(1));
+  // CHECK-M

[PATCH] D32923: [clang-tidy] Use cxxStdInitializerListExpr in modernize-use-emplace

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL302317: [clang-tidy] Use cxxStdInitializerListExpr in 
modernize-use-emplace (authored by kuhar).

Changed prior to commit:
  https://reviews.llvm.org/D32923?vs=98024&id=98034#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32923

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp


Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,12 +20,6 @@
   return Node.hasExplicitTemplateArgs();
 }
 
-namespace impl {
-// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
-const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
-} // namespace impl
-
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -81,9 +75,7 @@
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
   auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
-   has(impl::cxxStdInitializerListExpr()));
-  // FIXME: Replace internal C++ initializer list matcher with one from
-  // ASTMatchers.h
+   has(cxxStdInitializerListExpr()));
 
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.


Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,12 +20,6 @@
   return Node.hasExplicitTemplateArgs();
 }
 
-namespace impl {
-// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
-const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
-} // namespace impl
-
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -81,9 +75,7 @@
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
   auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
-   has(impl::cxxStdInitializerListExpr()));
-  // FIXME: Replace internal C++ initializer list matcher with one from
-  // ASTMatchers.h
+   has(cxxStdInitializerListExpr()));
 
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32923: [clang-tidy] Use cxxStdInitializerListExpr in modernize-use-emplace

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.

Use the cxxStdInitializerListExp matcher from ASTMatchers.h instead of a local 
one.


Repository:
  rL LLVM

https://reviews.llvm.org/D32923

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp


Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,12 +20,6 @@
   return Node.hasExplicitTemplateArgs();
 }
 
-namespace impl {
-// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
-const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
-} // namespace impl
-
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -81,9 +75,7 @@
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
   auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
-   has(impl::cxxStdInitializerListExpr()));
-  // FIXME: Replace internal C++ initializer list matcher with one from
-  // ASTMatchers.h
+   has(cxxStdInitializerListExpr()));
 
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.


Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,12 +20,6 @@
   return Node.hasExplicitTemplateArgs();
 }
 
-namespace impl {
-// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
-const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
-} // namespace impl
-
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -81,9 +75,7 @@
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
   auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
-   has(impl::cxxStdInitializerListExpr()));
-  // FIXME: Replace internal C++ initializer list matcher with one from
-  // ASTMatchers.h
+   has(cxxStdInitializerListExpr()));
 
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32810: Add cxxStdInitializerListExpr AST matcher

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL302287: Add cxxStdInitializerListExpr AST matcher (authored 
by kuhar).

Changed prior to commit:
  https://reviews.llvm.org/D32810?vs=97700&id=98018#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32810

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -1223,6 +1223,20 @@
   InnerMatcher.matches(*SyntForm, Finder, Builder));
 }
 
+/// \brief Matches C++ initializer list expressions.
+///
+/// Given
+/// \code
+///   std::vector a({ 1, 2, 3 });
+///   std::vector b = { 4, 5 };
+///   int c[] = { 6, 7 };
+///   std::pair d = { 8, 9 };
+/// \endcode
+/// cxxStdInitializerListExpr()
+///   matches "{ 1, 2, 3 }" and "{ 4, 5 }"
+const internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+
 /// \brief Matches implicit initializers of init list expressions.
 ///
 /// Given
Index: cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -153,6 +153,7 @@
   REGISTER_MATCHER(cxxRecordDecl);
   REGISTER_MATCHER(cxxReinterpretCastExpr);
   REGISTER_MATCHER(cxxStaticCastExpr);
+  REGISTER_MATCHER(cxxStdInitializerListExpr);
   REGISTER_MATCHER(cxxTemporaryObjectExpr);
   REGISTER_MATCHER(cxxThisExpr);
   REGISTER_MATCHER(cxxThrowExpr);
Index: cfe/trunk/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1020,6 +1020,29 @@
 matches("int i[1] = {42, [0] = 43};", integerLiteral(equals(42;
 }
 
+TEST(CXXStdInitializerListExpression, MatchesCXXStdInitializerListExpression) {
+  const std::string code = "namespace std {"
+   "template  class initializer_list {"
+   "  public: initializer_list() noexcept {}"
+   "};"
+   "}"
+   "struct A {"
+   "  A(std::initializer_list) {}"
+   "};";
+  EXPECT_TRUE(matches(code + "A a{0};",
+  cxxConstructExpr(has(cxxStdInitializerListExpr()),
+   hasDeclaration(cxxConstructorDecl(
+   ofClass(hasName("A")));
+  EXPECT_TRUE(matches(code + "A a = {0};",
+  cxxConstructExpr(has(cxxStdInitializerListExpr()),
+   hasDeclaration(cxxConstructorDecl(
+   ofClass(hasName("A")));
+
+  EXPECT_TRUE(notMatches("int a[] = { 1, 2 };", cxxStdInitializerListExpr()));
+  EXPECT_TRUE(notMatches("struct B { int x, y; }; B b = { 5, 6 };",
+ cxxStdInitializerListExpr()));
+}
+
 TEST(UsingDeclaration, MatchesUsingDeclarations) {
   EXPECT_TRUE(matches("namespace X { int x; } using X::x;",
   usingDecl()));
Index: cfe/trunk/docs/LibASTMatchersReference.html
===
--- cfe/trunk/docs/LibASTMatchersReference.html
+++ cfe/trunk/docs/LibASTMatchersReference.html
@@ -924,6 +924,19 @@
 
 
 
+MatcherStmt>cxxStdInitializerListExprMatcherCXXStdInitializerListExpr>...
+Matches C++ initializer list expressions.
+
+Given
+  std::vector a({ 1, 2, 3 });
+  std::vector b = { 4, 5 };
+  int c[] = { 6, 7 };
+  std::pair d = { 8, 9 };
+cxxStdInitializerListExpr()
+  matches "{ 1, 2, 3 }" and "{ 4, 5 }"
+
+
+
 MatcherStmt>cxxTemporaryObjectExprMatcherCXXTemporaryObjectExpr>...
 Matches functional cast expressions having N != 1 arguments
 
@@ -1160,7 +1173,7 @@
 Matches nodes where temporaries are materialized.
 
 Example: Given
-  struct T {void func()};
+  struct T {void func();};
   T f();
   void g(T);
 materializeTemporaryExpr() matches 'f()' in these statements
@@ -5233,7 +5246,7 @@
 Matches on the receiver of an ObjectiveC Message expression.
 
 Example
-matcher = objCMessageExpr(hasRecieverType(asString("UIWebView *")));
+matcher = objCMessageExpr(hasReceiverType(asString("UIWebView *")));
 matches the [webView ...] mes

[PATCH] D32767: [clang-tidy] Fix PR32896: detect initializer lists in modernize-use-empalce

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL302281: [clang-tidy] Fix PR32896: detect initializer lists 
in modernize-use-empalce (authored by kuhar).

Changed prior to commit:
  https://reviews.llvm.org/D32767?vs=97704&id=98013#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32767

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp


Index: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
@@ -4,9 +4,19 @@
 // RUN:   value: '::std::vector; ::std::list; ::std::deque; 
llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
+template 
+class initializer_list
+{
+public:
+  initializer_list() noexcept {}
+};
+
 template 
 class vector {
 public:
+  vector() = default;
+  vector(initializer_list) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -455,3 +465,16 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
 }
+
+void testInitializerList() {
+  std::vector> v;
+  v.push_back(std::vector({1}));
+  // Test against the bug reported in PR32896.
+
+  v.push_back({{2}});
+
+  using PairIntVector = std::pair>;
+  std::vector x;
+  x.push_back(PairIntVector(3, {4}));
+  x.push_back({5, {6}});
+}
Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,6 +20,12 @@
   return Node.hasExplicitTemplateArgs();
 }
 
+namespace impl {
+// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
+const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+} // namespace impl
+
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -74,7 +80,11 @@
   // emplace_back can't access private constructor.
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto HasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
+   has(impl::cxxStdInitializerListExpr()));
+  // FIXME: Replace internal C++ initializer list matcher with one from
+  // ASTMatchers.h
+
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
   auto SoughtConstructExpr =


Index: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp
@@ -4,9 +4,19 @@
 // RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
+template 
+class initializer_list
+{
+public:
+  initializer_list() noexcept {}
+};
+
 template 
 class vector {
 public:
+  vector() = default;
+  vector(initializer_list) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -455,3 +465,16 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
 }
+
+void testInitializerList() {
+  std::vector> v;
+  v.push_back(std::vector({1}));
+  // Test against the bug reported in PR32896.
+
+  v.push_back({{2}});
+
+  using PairIntVector = std::pair>;
+  std::vector x;
+  x.push_back(PairIntVector(3, {4}));
+  x.push_back({5, {6}});
+}
Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,6 +20,12 @@
   return Node.hasExplicitTemplateArgs();
 }
 
+namespace impl {
+// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
+const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+} // namespace impl
+
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -74,7 +80,11 @@
   // emplace_back can't access private constructor.
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto HasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
+   has(impl::cxxStdInitializerListExpr()));
+  // FIXME: Replace internal C++ initializer list matcher with one 

[PATCH] D32767: [clang-tidy] Fix PR32896: detect initializer lists in modernize-use-empalce

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 97704.
kuhar added a comment.

I switched from a narrowing matcher to a "normal" node matcher.


Repository:
  rL LLVM

https://reviews.llvm.org/D32767

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  test/clang-tidy/modernize-use-emplace.cpp


Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -4,9 +4,19 @@
 // RUN:   value: '::std::vector; ::std::list; ::std::deque; 
llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
+template 
+class initializer_list
+{
+public:
+  initializer_list() noexcept {}
+};
+
 template 
 class vector {
 public:
+  vector() = default;
+  vector(initializer_list) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -455,3 +465,16 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
 }
+
+void testInitializerList() {
+  std::vector> v;
+  v.push_back(std::vector({1}));
+  // Test against the bug reported in PR32896.
+
+  v.push_back({{2}});
+
+  using PairIntVector = std::pair>;
+  std::vector x;
+  x.push_back(PairIntVector(3, {4}));
+  x.push_back({5, {6}});
+}
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,6 +20,12 @@
   return Node.hasExplicitTemplateArgs();
 }
 
+namespace impl {
+// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
+const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+} // namespace impl
+
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -74,7 +80,11 @@
   // emplace_back can't access private constructor.
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto HasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
+   has(impl::cxxStdInitializerListExpr()));
+  // FIXME: Replace internal C++ initializer list matcher with one from
+  // ASTMatchers.h
+
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
   auto SoughtConstructExpr =


Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -4,9 +4,19 @@
 // RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
+template 
+class initializer_list
+{
+public:
+  initializer_list() noexcept {}
+};
+
 template 
 class vector {
 public:
+  vector() = default;
+  vector(initializer_list) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -455,3 +465,16 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
 }
+
+void testInitializerList() {
+  std::vector> v;
+  v.push_back(std::vector({1}));
+  // Test against the bug reported in PR32896.
+
+  v.push_back({{2}});
+
+  using PairIntVector = std::pair>;
+  std::vector x;
+  x.push_back(PairIntVector(3, {4}));
+  x.push_back({5, {6}});
+}
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,6 +20,12 @@
   return Node.hasExplicitTemplateArgs();
 }
 
+namespace impl {
+// FIXME: This matcher should be replaced by a matcher from ASTMatcher.h
+const ast_matchers::internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+} // namespace impl
+
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -74,7 +80,11 @@
   // emplace_back can't access private constructor.
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto HasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
+   has(impl::cxxStdInitializerListExpr()));
+  // FIXME: Replace internal C++ initializer list matcher with one from
+  // ASTMatchers.h
+
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
   auto SoughtConstructExpr =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32810: Add cxxStdInitializerListExpr AST matcher

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 97700.
kuhar added a comment.

I regenerated docs and added unit tests.

Thanks for help.


Repository:
  rL LLVM

https://reviews.llvm.org/D32810

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -1020,6 +1020,29 @@
 matches("int i[1] = {42, [0] = 43};", integerLiteral(equals(42;
 }
 
+TEST(CXXStdInitializerListExpression, MatchesCXXStdInitializerListExpression) {
+  const std::string code = "namespace std {"
+   "template  class initializer_list {"
+   "  public: initializer_list() noexcept {}"
+   "};"
+   "}"
+   "struct A {"
+   "  A(std::initializer_list) {}"
+   "};";
+  EXPECT_TRUE(matches(code + "A a{0};",
+  cxxConstructExpr(has(cxxStdInitializerListExpr()),
+   hasDeclaration(cxxConstructorDecl(
+   ofClass(hasName("A")));
+  EXPECT_TRUE(matches(code + "A a = {0};",
+  cxxConstructExpr(has(cxxStdInitializerListExpr()),
+   hasDeclaration(cxxConstructorDecl(
+   ofClass(hasName("A")));
+
+  EXPECT_TRUE(notMatches("int a[] = { 1, 2 };", cxxStdInitializerListExpr()));
+  EXPECT_TRUE(notMatches("struct B { int x, y; }; B b = { 5, 6 };",
+ cxxStdInitializerListExpr()));
+}
+
 TEST(UsingDeclaration, MatchesUsingDeclarations) {
   EXPECT_TRUE(matches("namespace X { int x; } using X::x;",
   usingDecl()));
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -153,6 +153,7 @@
   REGISTER_MATCHER(cxxRecordDecl);
   REGISTER_MATCHER(cxxReinterpretCastExpr);
   REGISTER_MATCHER(cxxStaticCastExpr);
+  REGISTER_MATCHER(cxxStdInitializerListExpr);
   REGISTER_MATCHER(cxxTemporaryObjectExpr);
   REGISTER_MATCHER(cxxThisExpr);
   REGISTER_MATCHER(cxxThrowExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -1223,6 +1223,20 @@
   InnerMatcher.matches(*SyntForm, Finder, Builder));
 }
 
+/// \brief Matches C++ initializer list expressions.
+///
+/// Given
+/// \code
+///   std::vector a({ 1, 2, 3 });
+///   std::vector b = { 4, 5 };
+///   int c[] = { 6, 7 };
+///   std::pair d = { 8, 9 };
+/// \endcode
+/// cxxStdInitializerListExpr()
+///   matches "{ 1, 2, 3 }" and "{ 4, 5 }"
+const internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+
 /// \brief Matches implicit initializers of init list expressions.
 ///
 /// Given
Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -924,6 +924,19 @@
 
 
 
+MatcherStmt>cxxStdInitializerListExprMatcherCXXStdInitializerListExpr>...
+Matches C++ initializer list expressions.
+
+Given
+  std::vector a({ 1, 2, 3 });
+  std::vector b = { 4, 5 };
+  int c[] = { 6, 7 };
+  std::pair d = { 8, 9 };
+cxxStdInitializerListExpr()
+  matches "{ 1, 2, 3 }" and "{ 4, 5 }"
+
+
+
 MatcherStmt>cxxTemporaryObjectExprMatcherCXXTemporaryObjectExpr>...
 Matches functional cast expressions having N != 1 arguments
 
@@ -1160,7 +1173,7 @@
 Matches nodes where temporaries are materialized.
 
 Example: Given
-  struct T {void func()};
+  struct T {void func();};
   T f();
   void g(T);
 materializeTemporaryExpr() matches 'f()' in these statements
@@ -5233,7 +5246,7 @@
 Matches on the receiver of an ObjectiveC Message expression.
 
 Example
-matcher = objCMessageExpr(hasRecieverType(asString("UIWebView *")));
+matcher = objCMessageExpr(hasReceiverType(asString("UIWebView *")));
 matches the [webView ...] message invocation.
   NSString *webViewJavaScript = ...
   UIWebView *webView = ...
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32767: [clang-tidy] Fix PR32896: detect initializer lists in modernize-use-empalce

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:23
 
+AST_MATCHER(CXXStdInitializerListExpr, cxxStdInitializerListExpr) {
+  return true;

Prazek wrote:
> kuhar wrote:
> > alexfh wrote:
> > > This should be a node matcher rather than a narrowing matcher, and it 
> > > should be placed to ASTMatchers.h, if there is no such matcher already.
> > I wanted to backport this fix to 4.0.1 release after applying it in trunk.
> > I thought that adding a new ASTMatcher would introduce an API change in 
> > clang, and I'm not sure if it's much welcome.
> > 
> > Maybe it would be better to introduce a new matcher in ASTMatchers.h in 
> > trunk and make it an internal narrowing matcher in the backported patch? 
> > What would be best?  
> Why adding new matcher to 4.0.1 release is bad? Will it break anything?
The example that I originally came up with was updating clang-tidy to 4.0.1 
while keeping clang libs 4.0.0, but since we just include all the matchers I 
think it should work fine.

Here's a patch with the new matcher: https://reviews.llvm.org/D32810


https://reviews.llvm.org/D32767



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

[PATCH] D32810: Add cxxStdInitializerListExpr AST matcher

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
Herald added a subscriber: klimek.

This adds a new ASTMatcher for CXXStdInitializerListExprs that matches C++ 
initializer list expressions.

The primary motivation is to use it to fix PR32896 
 (review here D32767 
).


Repository:
  rL LLVM

https://reviews.llvm.org/D32810

Files:
  include/clang/ASTMatchers/ASTMatchers.h
  lib/ASTMatchers/Dynamic/Registry.cpp


Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -153,6 +153,7 @@
   REGISTER_MATCHER(cxxRecordDecl);
   REGISTER_MATCHER(cxxReinterpretCastExpr);
   REGISTER_MATCHER(cxxStaticCastExpr);
+  REGISTER_MATCHER(cxxStdInitializerListExpr);
   REGISTER_MATCHER(cxxTemporaryObjectExpr);
   REGISTER_MATCHER(cxxThisExpr);
   REGISTER_MATCHER(cxxThrowExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -1223,6 +1223,20 @@
   InnerMatcher.matches(*SyntForm, Finder, Builder));
 }
 
+/// \brief Matches C++ initializer list expressions.
+///
+/// Given
+/// \code
+///   std::vector a({ 1, 2, 3 });
+///   std::vector b = { 4, 5 };
+///   int c[] = { 6, 7 };
+///   std::pair d = { 8, 9 }; 
+/// \endcode
+/// cxxStdInitializerListExpr()
+///   matches "{ 1, 2, 3 }" and "{ 4, 5 }"
+const internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+
 /// \brief Matches implicit initializers of init list expressions.
 ///
 /// Given


Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -153,6 +153,7 @@
   REGISTER_MATCHER(cxxRecordDecl);
   REGISTER_MATCHER(cxxReinterpretCastExpr);
   REGISTER_MATCHER(cxxStaticCastExpr);
+  REGISTER_MATCHER(cxxStdInitializerListExpr);
   REGISTER_MATCHER(cxxTemporaryObjectExpr);
   REGISTER_MATCHER(cxxThisExpr);
   REGISTER_MATCHER(cxxThrowExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -1223,6 +1223,20 @@
   InnerMatcher.matches(*SyntForm, Finder, Builder));
 }
 
+/// \brief Matches C++ initializer list expressions.
+///
+/// Given
+/// \code
+///   std::vector a({ 1, 2, 3 });
+///   std::vector b = { 4, 5 };
+///   int c[] = { 6, 7 };
+///   std::pair d = { 8, 9 }; 
+/// \endcode
+/// cxxStdInitializerListExpr()
+///   matches "{ 1, 2, 3 }" and "{ 4, 5 }"
+const internal::VariadicDynCastAllOfMatcher cxxStdInitializerListExpr;
+
 /// \brief Matches implicit initializers of init list expressions.
 ///
 /// Given
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32767: [clang-tidy] Fix PR32896: detect initializer lists in modernize-use-empalce

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:23
 
+AST_MATCHER(CXXStdInitializerListExpr, cxxStdInitializerListExpr) {
+  return true;

alexfh wrote:
> This should be a node matcher rather than a narrowing matcher, and it should 
> be placed to ASTMatchers.h, if there is no such matcher already.
I wanted to backport this fix to 4.0.1 release after applying it in trunk.
I thought that adding a new ASTMatcher would introduce an API change in clang, 
and I'm not sure if it's much welcome.

Maybe it would be better to introduce a new matcher in ASTMatchers.h in trunk 
and make it an internal narrowing matcher in the backported patch? What would 
be best?  


https://reviews.llvm.org/D32767



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

[PATCH] D32767: [clang-tidy] Fix PR32896: detect initializer lists in modernize-use-empalce

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.

This patch fixes PR32896 .

The problem was that modernize-use-emplace incorrectly removed changed 
push_back into emplace_back, removing explicit constructor call with 
initializer list parameter, resulting in compiler error after applying fixits.
modernize-use-emplace used to check if matched constructor had InitListExpr, 
but didn't check against CXXStdInitializerListExpr.

Eg.

  std::vector> v;
v.push_back(std::vector({1})); // --> v.emplace_back({1});


https://reviews.llvm.org/D32767

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  test/clang-tidy/modernize-use-emplace.cpp


Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -4,9 +4,19 @@
 // RUN:   value: '::std::vector; ::std::list; ::std::deque; 
llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
+template 
+class initializer_list
+{
+public:
+  initializer_list() noexcept {}
+};
+
 template 
 class vector {
 public:
+  vector() = default;
+  vector(initializer_list) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -455,3 +465,16 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
 }
+
+void testInitializerList() {
+  std::vector> v;
+  v.push_back(std::vector({1}));
+  // Test against the bug reported in PR32896.
+
+  v.push_back({{2}});
+
+  using PairIntVector = std::pair>;
+  std::vector x;
+  x.push_back(PairIntVector(3, {4}));
+  x.push_back({5, {6}});
+}
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,6 +20,10 @@
   return Node.hasExplicitTemplateArgs();
 }
 
+AST_MATCHER(CXXStdInitializerListExpr, cxxStdInitializerListExpr) {
+  return true;
+}
+
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -74,7 +78,8 @@
   // emplace_back can't access private constructor.
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto HasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
+   has(cxxStdInitializerListExpr()));
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
   auto SoughtConstructExpr =


Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -4,9 +4,19 @@
 // RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
+template 
+class initializer_list
+{
+public:
+  initializer_list() noexcept {}
+};
+
 template 
 class vector {
 public:
+  vector() = default;
+  vector(initializer_list) {}
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
@@ -455,3 +465,16 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
 }
+
+void testInitializerList() {
+  std::vector> v;
+  v.push_back(std::vector({1}));
+  // Test against the bug reported in PR32896.
+
+  v.push_back({{2}});
+
+  using PairIntVector = std::pair>;
+  std::vector x;
+  x.push_back(PairIntVector(3, {4}));
+  x.push_back({5, {6}});
+}
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -20,6 +20,10 @@
   return Node.hasExplicitTemplateArgs();
 }
 
+AST_MATCHER(CXXStdInitializerListExpr, cxxStdInitializerListExpr) {
+  return true;
+}
+
 const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 const auto DefaultSmartPointers =
@@ -74,7 +78,8 @@
   // emplace_back can't access private constructor.
   auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto HasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = anyOf(has(ignoringImplicit(initListExpr())),
+   has(cxxStdInitializerListExpr()));
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
   auto SoughtConstructExpr =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32690: [clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 97299.
kuhar edited the summary of this revision.
kuhar added a comment.

I went ahead and generalized the patch to support custom tuple-like types and 
custom make_ functions.
I added a bunch of tests and updated the docs.

Let me know what you think.


https://reviews.llvm.org/D32690

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -1,7 +1,12 @@
 // RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
 // RUN:   -config="{CheckOptions: \
 // RUN: [{key: modernize-use-emplace.ContainersWithPushBack, \
-// RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
+// RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}, \
+// RUN:  {key: modernize-use-emplace.TupleTypes, \
+// RUN:   value: '::std::pair; std::tuple; ::test::Single'}, \
+// RUN:  {key: modernize-use-emplace.TupleMakeFunctions, \
+// RUN:   value: '::std::make_pair; ::std::make_tuple; ::test::MakeSingle'}] \
+// RUN: }" -- -std=c++11
 
 namespace std {
 template 
@@ -36,27 +41,54 @@
   ~deque();
 };
 
-template 
-class pair {
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+
+template  class pair {
 public:
   pair() = default;
   pair(const pair &) = default;
   pair(pair &&) = default;
 
   pair(const T1 &, const T2 &) {}
   pair(T1 &&, T2 &&) {}
 
-  template 
-  pair(const pair &p){};
-  template 
-  pair(pair &&p){};
+  template  pair(const pair &){};
+  template  pair(pair &&){};
 };
 
 template 
-pair make_pair(T1&&, T2&&) {
+pair::type, typename remove_reference::type>
+make_pair(T1 &&, T2 &&) {
   return {};
 };
 
+template  class tuple {
+public:
+  tuple() = default;
+  tuple(const tuple &) = default;
+  tuple(tuple &&) = default;
+
+  tuple(const Ts &...) {}
+  tuple(Ts &&...) {}
+
+  template  tuple(const tuple &){};
+  template  tuple(tuple &&) {}
+
+  template  tuple(const pair &) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+  template  tuple(pair &&) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+};
+
+template 
+tuple::type...> make_tuple(Ts &&...) {
+  return {};
+}
+
 template 
 class unique_ptr {
 public:
@@ -197,6 +229,30 @@
   // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37)));
 }
 
+void testTuple() {
+  std::vector> v;
+  v.push_back(std::tuple(false, 'x', 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'x', 1);
+
+  v.push_back(std::tuple{false, 'y', 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'y', 2);
+
+  v.push_back({true, 'z', 3});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(true, 'z', 3);
+
+  std::vector> x;
+  x.push_back(std::make_pair(1, false));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(1, false);
+
+  x.push_back(std::make_pair(2LL, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(2LL, 1);
+}
+
 struct Base {
   Base(int, int *, int = 42);
 };
@@ -318,6 +374,77 @@
   // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
+void testMakeTuple() {
+  std::vector> v;
+  v.push_back(std::make_tuple(1, true, 'v'));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, true, 'v');
+
+  v.push_back(std::make_tuple(2ULL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(2ULL, 1, 0);
+
+  v.push_back(std::make_tuple(3LL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_tuple(3LL, 1, 0));
+  // make_tuple is not removed when there are explicit template
+  // arguments provided.
+}
+
+namespace test {
+template  struct Single {
+  Single() = default;
+  Single(const Single &) = default;
+  Single(Single &&) = default;
+
+  Single(const T &) {}
+  Single(T &&) {}
+
+  template  Single(const Single &) {}
+  template  Single(Single &&) {}
+
+  template  Single(const std::tuple &) {}
+  template  Single(std::tuple &&) {}
+};
+
+template 
+Single::type> MakeSingle(T &&) {
+  return {};
+}
+} // namespace test
+
+void testOtherTuples() {
+  std::vector> v;
+  v.push_back(test::Single(1));

[PATCH] D32690: [clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

In https://reviews.llvm.org/D32690#741952, @JonasToth wrote:

> Would it make sense to add boost pairs/tuples support?


I think it can be added the same way it is possible to specify smart pointers 
and vector-like containers now. It would require users to provide both types 
and make functions (eg. thingy and make_thingy). I'm not sure if that would be 
any useful in practise, though.

> Something for a later check: the same logic should apply for `insert` and 
> `emplace`, we should add support for these methods too.

Yeah, I plan to do that a few patches from now, after other things are improved.


https://reviews.llvm.org/D32690



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

[PATCH] D32690: [clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 97248.
kuhar added a comment.

Cosmetic changes.


https://reviews.llvm.org/D32690

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -36,27 +36,54 @@
   ~deque();
 };
 
-template 
-class pair {
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+
+template  class pair {
 public:
   pair() = default;
   pair(const pair &) = default;
   pair(pair &&) = default;
 
   pair(const T1 &, const T2 &) {}
   pair(T1 &&, T2 &&) {}
 
-  template 
-  pair(const pair &p){};
-  template 
-  pair(pair &&p){};
+  template  pair(const pair &){};
+  template  pair(pair &&){};
 };
 
 template 
-pair make_pair(T1&&, T2&&) {
+pair::type, typename remove_reference::type>
+make_pair(T1 &&, T2 &&) {
   return {};
 };
 
+template  class tuple {
+public:
+  tuple() = default;
+  tuple(const tuple &) = default;
+  tuple(tuple &&) = default;
+
+  tuple(const Ts &...) {}
+  tuple(Ts &&...) {}
+
+  template  tuple(const tuple &){};
+  template  tuple(tuple &&) {}
+
+  template  tuple(const pair &) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+  template  tuple(pair &&) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+};
+
+template 
+tuple::type...> make_tuple(Ts &&...) {
+  return {};
+}
+
 template 
 class unique_ptr {
 public:
@@ -197,6 +224,30 @@
   // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37)));
 }
 
+void testTuple() {
+  std::vector> v;
+  v.push_back(std::tuple(false, 'x', 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'x', 1);
+
+  v.push_back(std::tuple{false, 'y', 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'y', 2);
+
+  v.push_back({true, 'z', 3});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(true, 'z', 3);
+
+  std::vector> x;
+  x.push_back(std::make_pair(1, false));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(1, false);
+
+  x.push_back(std::make_pair(2LL, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(2LL, 1);
+}
+
 struct Base {
   Base(int, int *, int = 42);
 };
@@ -318,6 +369,23 @@
   // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
+void testMakeTuple() {
+  std::vector> v;
+  v.push_back(std::make_tuple(1, true, 'v'));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, true, 'v');
+
+  v.push_back(std::make_tuple(2ULL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(2ULL, 1, 0);
+
+  v.push_back(std::make_tuple(3LL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_tuple(3LL, 1, 0));
+  // make_tuple is not removed when there are explicit template
+  // arguments provided.
+}
+
 void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
@@ -437,7 +505,7 @@
   void doStuff() {
 std::vector v;
 // This should not change it because emplace back doesn't have permission.
-// Check currently doesn't support friend delcarations because pretty much
+// Check currently doesn't support friend declarations because pretty much
 // nobody would want to be friend with std::vector :(.
 v.push_back(PrivateCtor(42));
   }
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -109,8 +109,8 @@
 - Improved `modernize-use-emplace
   `_ check
 
-  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
-  into emplace_back(a, b).
+  Removes unnecessary ``std::make_pair`` and ``std::make_tuple`` calls in push_back calls
+  and turns them into emplace_back.
 
 Improvements to include-fixer
 -
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -85,20 +85,22 @@
   .bind("ctor");
   auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
 
-  auto MakePair = ignoringImplicit(
-  callExpr(callee(expr(ignoringImplicit(
-  declRefExpr(unless(hasExplicitTemplateArgs()),
-  

[PATCH] D32690: [clang-tidy] modernize-use-emplace: Remove unnecessary make_tuple calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added a project: clang-tools-extra.

This patch makes modernize-use-emplace remove unnecessary make_tuple calls from 
push_back calls and turn them into emplace_back -- the same way make_pair calls 
are handled.

Eq.

  std::vector> v;
  v.push_back(std::make_tuple(1, 'A', true)); // --> v.emplace_back(1, 'A', 
true);


https://reviews.llvm.org/D32690

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -36,27 +36,54 @@
   ~deque();
 };
 
-template 
-class pair {
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+template  struct remove_reference { using type = T; };
+
+template  class pair {
 public:
   pair() = default;
   pair(const pair &) = default;
   pair(pair &&) = default;
 
   pair(const T1 &, const T2 &) {}
   pair(T1 &&, T2 &&) {}
 
-  template 
-  pair(const pair &p){};
-  template 
-  pair(pair &&p){};
+  template  pair(const pair &){};
+  template  pair(pair &&){};
 };
 
 template 
-pair make_pair(T1&&, T2&&) {
+pair::type, typename remove_reference::type>
+make_pair(T1 &&, T2 &&) {
   return {};
 };
 
+template  class tuple {
+public:
+  tuple() = default;
+  tuple(const tuple &) = default;
+  tuple(tuple &&) = default;
+
+  tuple(const Ts &...) {}
+  tuple(Ts &&...) {}
+
+  template  tuple(const tuple &){};
+  template  tuple(tuple &&) {}
+
+  template  tuple(const pair &) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+  template  tuple(pair &&) {
+static_assert(sizeof...(Ts) == 2, "Wrong tuple size");
+  };
+};
+
+template 
+tuple::type...> make_tuple(Ts &&...) {
+  return {};
+}
+
 template 
 class unique_ptr {
 public:
@@ -197,6 +224,30 @@
   // CHECK-FIXES: v2.emplace_back(Something(42, 42), Zoz(Something(21, 37)));
 }
 
+void testTuple() {
+  std::vector> v;
+  v.push_back(std::tuple(false, 'x', 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'x', 1);
+
+  v.push_back(std::tuple{false, 'y', 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(false, 'y', 2);
+
+  v.push_back({true, 'z', 3});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(true, 'z', 3);
+
+  std::vector> x;
+  x.push_back(std::make_pair(1, false));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(1, false);
+
+  x.push_back(std::make_pair(2LL, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(2LL, 1);
+}
+
 struct Base {
   Base(int, int *, int = 42);
 };
@@ -318,6 +369,23 @@
   // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
+void testMakeTuple() {
+  std::vector> v;
+  v.push_back(std::make_tuple(1, true, 'v'));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, true, 'v');
+
+  v.push_back(std::make_tuple(2ULL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(2ULL, 1, 0);
+
+  v.push_back(std::make_tuple(3LL, 1, 0));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_tuple(3LL, 1, 0));
+  // make_tuple is not removed when there are explicit template
+  // arguments provided.
+}
+
 void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
@@ -437,7 +505,7 @@
   void doStuff() {
 std::vector v;
 // This should not change it because emplace back doesn't have permission.
-// Check currently doesn't support friend delcarations because pretty much
+// Check currently doesn't support friend declarations because pretty much
 // nobody would want to be friend with std::vector :(.
 v.push_back(PrivateCtor(42));
   }
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -109,8 +109,8 @@
 - Improved `modernize-use-emplace
   `_ check
 
-  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
-  into emplace_back(a, b).
+  Removes unnecessary ``std::make_pair`` and ``std::make_tuple`` calls in push_back calls
+  and turns them into emplace_back.
 
 Improvements to include-fixer
 -
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -85,21 +85

[PATCH] D32678: [clang-tidy] Fix naming convention in modernize-use-emplace

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL301780: [clang-tidy] Fix naming convention in 
modernize-use-emplace (authored by kuhar).

Changed prior to commit:
  https://reviews.llvm.org/D32678?vs=97221&id=97243#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32678

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp


Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -45,63 +45,63 @@
   // because this requires special treatment (it could cause performance
   // regression)
   // + match for emplace calls that should be replaced with insertion
-  auto callPushBack = cxxMemberCallExpr(
+  auto CallPushBack = cxxMemberCallExpr(
   hasDeclaration(functionDecl(hasName("push_back"))),
   on(hasType(cxxRecordDecl(hasAnyName(SmallVector(
   ContainersWithPushBack.begin(), ContainersWithPushBack.end()));
 
   // We can't replace push_backs of smart pointer because
   // if emplacement fails (f.e. bad_alloc in vector) we will have leak of
   // passed pointer because smart pointer won't be constructed
   // (and destructed) as in push_back case.
-  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
+  auto IsCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
   SmallVector(SmartPointers.begin(), 
SmartPointers.end());
 
   // Bitfields binds only to consts and emplace_back take it by universal ref.
-  auto bitFieldAsArgument = hasAnyArgument(
+  auto BitFieldAsArgument = hasAnyArgument(
   ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField());
 
   // Initializer list can't be passed to universal reference.
-  auto initializerListAsArgument = hasAnyArgument(
+  auto InitializerListAsArgument = hasAnyArgument(
   ignoringImplicit(cxxConstructExpr(isListInitialization(;
 
   // We could have leak of resource.
-  auto newExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
+  auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
   // We would call another constructor.
-  auto constructingDerived =
+  auto ConstructingDerived =
   hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
 
   // emplace_back can't access private constructor.
-  auto isPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
+  auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto hasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = has(ignoringImplicit(initListExpr()));
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
-  auto soughtConstructExpr =
+  auto SoughtConstructExpr =
   cxxConstructExpr(
-  unless(anyOf(isCtorOfSmartPtr, hasInitList, bitFieldAsArgument,
-   initializerListAsArgument, newExprAsArgument,
-   constructingDerived, isPrivateCtor)))
+  unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument,
+   InitializerListAsArgument, NewExprAsArgument,
+   ConstructingDerived, IsPrivateCtor)))
   .bind("ctor");
-  auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
+  auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
 
-  auto makePair = ignoringImplicit(
+  auto MakePair = ignoringImplicit(
   callExpr(callee(expr(ignoringImplicit(
   declRefExpr(unless(hasExplicitTemplateArgs()),
   to(functionDecl(hasName("::std::make_pair"
   .bind("make_pair"));
 
   // make_pair can return type convertible to container's element type.
   // Allow the conversion only on containers of pairs.
-  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
-  has(materializeTemporaryExpr(makePair)),
+  auto MakePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(MakePair)),
   hasDeclaration(cxxConstructorDecl(ofClass(hasName("::std::pair"));
 
-  auto soughtParam = materializeTemporaryExpr(
-  anyOf(has(makePair), has(makePairCtor),
-hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto SoughtParam = materializeTemporaryExpr(
+  anyOf(has(MakePair), has(MakePairCtor),
+HasConstructExpr, has(cxxFunctionalCastExpr(HasConstructExpr;
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(soughtParam),
+  Finder->addMatcher(cxxMemberCallExpr(CallPushBack, has(SoughtParam),
unless(isInTemplateInstantiation()))
  .bind("call"),
  this);


Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceChe

[PATCH] D32678: [clang-tidy] Fix naming convention in modernize-use-empace

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added a project: clang-tools-extra.

Conform to llvm naming convention for local variables in modernize-use-emplace 
check.


https://reviews.llvm.org/D32678

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp


Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -45,63 +45,63 @@
   // because this requires special treatment (it could cause performance
   // regression)
   // + match for emplace calls that should be replaced with insertion
-  auto callPushBack = cxxMemberCallExpr(
+  auto CallPushBack = cxxMemberCallExpr(
   hasDeclaration(functionDecl(hasName("push_back"))),
   on(hasType(cxxRecordDecl(hasAnyName(SmallVector(
   ContainersWithPushBack.begin(), ContainersWithPushBack.end()));
 
   // We can't replace push_backs of smart pointer because
   // if emplacement fails (f.e. bad_alloc in vector) we will have leak of
   // passed pointer because smart pointer won't be constructed
   // (and destructed) as in push_back case.
-  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
+  auto IsCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
   SmallVector(SmartPointers.begin(), 
SmartPointers.end());
 
   // Bitfields binds only to consts and emplace_back take it by universal ref.
-  auto bitFieldAsArgument = hasAnyArgument(
+  auto BitFieldAsArgument = hasAnyArgument(
   ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField());
 
   // Initializer list can't be passed to universal reference.
-  auto initializerListAsArgument = hasAnyArgument(
+  auto InitializerListAsArgument = hasAnyArgument(
   ignoringImplicit(cxxConstructExpr(isListInitialization(;
 
   // We could have leak of resource.
-  auto newExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
+  auto NewExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
   // We would call another constructor.
-  auto constructingDerived =
+  auto ConstructingDerived =
   hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
 
   // emplace_back can't access private constructor.
-  auto isPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
+  auto IsPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
 
-  auto hasInitList = has(ignoringImplicit(initListExpr()));
+  auto HasInitList = has(ignoringImplicit(initListExpr()));
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
-  auto soughtConstructExpr =
+  auto SoughtConstructExpr =
   cxxConstructExpr(
-  unless(anyOf(isCtorOfSmartPtr, hasInitList, bitFieldAsArgument,
-   initializerListAsArgument, newExprAsArgument,
-   constructingDerived, isPrivateCtor)))
+  unless(anyOf(IsCtorOfSmartPtr, HasInitList, BitFieldAsArgument,
+   InitializerListAsArgument, NewExprAsArgument,
+   ConstructingDerived, IsPrivateCtor)))
   .bind("ctor");
-  auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
+  auto HasConstructExpr = has(ignoringImplicit(SoughtConstructExpr));
 
-  auto makePair = ignoringImplicit(
+  auto MakePair = ignoringImplicit(
   callExpr(callee(expr(ignoringImplicit(
   declRefExpr(unless(hasExplicitTemplateArgs()),
   to(functionDecl(hasName("::std::make_pair"
   .bind("make_pair"));
 
   // make_pair can return type convertible to container's element type.
   // Allow the conversion only on containers of pairs.
-  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
-  has(materializeTemporaryExpr(makePair)),
+  auto MakePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(MakePair)),
   hasDeclaration(cxxConstructorDecl(ofClass(hasName("::std::pair"));
 
-  auto soughtParam = materializeTemporaryExpr(
-  anyOf(has(makePair), has(makePairCtor),
-hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto SoughtParam = materializeTemporaryExpr(
+  anyOf(has(MakePair), has(MakePairCtor),
+HasConstructExpr, has(cxxFunctionalCastExpr(HasConstructExpr;
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(soughtParam),
+  Finder->addMatcher(cxxMemberCallExpr(CallPushBack, has(SoughtParam),
unless(isInTemplateInstantiation()))
  .bind("call"),
  this);


Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -45,63 +45,63 @@
   // because this requires special treatment (it could cause p

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL301651: [clang-tidy] modernize-use-emplace: remove 
unnecessary make_pair calls (authored by kuhar).

Changed prior to commit:
  https://reviews.llvm.org/D32395?vs=96634&id=97113#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32395

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp

Index: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,10 +15,16 @@
 namespace tidy {
 namespace modernize {
 
-static const auto DefaultContainersWithPushBack =
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+
+const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
-static const auto DefaultSmartPointers =
+const auto DefaultSmartPointers =
 "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
+} // namespace
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
@@ -39,7 +45,6 @@
   // because this requires special treatment (it could cause performance
   // regression)
   // + match for emplace calls that should be replaced with insertion
-  // + match for make_pair calls.
   auto callPushBack = cxxMemberCallExpr(
   hasDeclaration(functionDecl(hasName("push_back"))),
   on(hasType(cxxRecordDecl(hasAnyName(SmallVector(
@@ -80,43 +85,64 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringImplicit(
+  declRefExpr(unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasName("::std::make_pair"
+  .bind("make_pair"));
+
+  // make_pair can return type convertible to container's element type.
+  // Allow the conversion only on containers of pairs.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(makePair)),
+  hasDeclaration(cxxConstructorDecl(ofClass(hasName("::std::pair"));
+
+  auto soughtParam = materializeTemporaryExpr(
+  anyOf(has(makePair), has(makePairCtor),
+hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(soughtParam),
unless(isInTemplateInstantiation()))
  .bind("call"),
  this);
 }
 
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Call = Result.Nodes.getNodeAs("call");
   const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor");
+  const auto *MakePairCall = Result.Nodes.getNodeAs("make_pair");
+  assert((InnerCtorCall || MakePairCall) && "No push_back parameter matched");
 
-  auto FunctionNameSourceRange = CharSourceRange::getCharRange(
+  const auto FunctionNameSourceRange = CharSourceRange::getCharRange(
   Call->getExprLoc(), Call->getArg(0)->getExprLoc());
 
   auto Diag = diag(Call->getExprLoc(), "use emplace_back instead of push_back");
 
   if (FunctionNameSourceRange.getBegin().isMacroID())
 return;
 
-  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
-   "emplace_back(");
+  const auto *EmplacePrefix = MakePairCall ? "emplace_back" : "emplace_back(";
+  Diag << FixItHint::CreateReplacement(FunctionNameSourceRange, EmplacePrefix);
 
-  auto CallParensRange = InnerCtorCall->getParenOrBraceRange();
+  const SourceRange CallParensRange =
+  MakePairCall ? SourceRange(MakePairCall->getCallee()->getLocEnd(),
+ MakePairCall->getRParenLoc())
+   : InnerCtorCall->getParenOrBraceRange();
 
   // Finish if there is no explicit constructor call.
   if (CallParensRange.getBegin().isInvalid())
 return;
 
+  const SourceLocation ExprBegin =
+  MakePairCall ? MakePairCall->getExprLoc() : InnerCtorCall->getExprLoc();
+
   // Range for constructor name and opening brace.
-  auto CtorCallSourceRange = CharSourceRange::getTokenRange(
-  InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
+  const auto ParamCallSourceRange =
+  CharSourceRange::getTokenRange(ExprBegin, CallParensRange.getBegin());
 
-  Diag << FixItHint::CreateRemoval(CtorCallSo

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

I run clang-tidy with this version of modernize-use-emplace on the whole llvm + 
clang + clang tools extra tree and it emitted ~500 fixits, ~100 of them were 
make_pair ones.
The whole codebase compiled fine and there were no new test failures with 
check-all.


https://reviews.llvm.org/D32395



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

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Jakub Kuderski via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL301365: [clang-tidy] run-clang-tidy.py: check if 
clang-apply-replacements succeeds (authored by kuhar).

Changed prior to commit:
  https://reviews.llvm.org/D32294?vs=96097&id=96642#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32294

Files:
  clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py

Index: clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
@@ -34,6 +34,7 @@
 http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 """
 
+from __future__ import print_function
 import argparse
 import json
 import multiprocessing
@@ -45,14 +46,15 @@
 import sys
 import tempfile
 import threading
+import traceback
 
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
 if os.path.realpath(result) == '/':
-  print 'Error: could not find compilation database.'
+  print('Error: could not find compilation database.')
   sys.exit(1)
 result += '../'
   return os.path.realpath(result)
@@ -87,14 +89,24 @@
   return start
 
 
+def check_clang_apply_replacements_binary(args):
+  """Checks if invoking supplied clang-apply-replacements binary works."""
+  try:
+subprocess.check_call([args.clang_apply_replacements_binary, '--version'])
+  except:
+print('Unable to run clang-apply-replacements. Is clang-apply-replacements '
+  'binary correctly specified?', file=sys.stderr)
+traceback.print_exc()
+sys.exit(1)
+
+
 def apply_fixes(args, tmpdir):
   """Calls clang-apply-fixes on a given directory. Deletes the dir when done."""
   invocation = [args.clang_apply_replacements_binary]
   if args.format:
 invocation.append('-format')
   invocation.append(tmpdir)
   subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
 
 
 def run_tidy(args, tmpdir, build_path, queue):
@@ -164,9 +176,9 @@
 if args.checks:
   invocation.append('-checks=' + args.checks)
 invocation.append('-')
-print subprocess.check_output(invocation)
+print(subprocess.check_output(invocation))
   except:
-print >>sys.stderr, "Unable to run clang-tidy."
+print("Unable to run clang-tidy.", file=sys.stderr)
 sys.exit(1)
 
   # Load the database and extract all files.
@@ -179,6 +191,7 @@
 
   tmpdir = None
   if args.fix:
+check_clang_apply_replacements_binary(args)
 tmpdir = tempfile.mkdtemp()
 
   # Build up a big regexy filter from all command line arguments.
@@ -204,14 +217,25 @@
   except KeyboardInterrupt:
 # This is a sad hack. Unfortunately subprocess goes
 # bonkers with ctrl-c and we start forking merrily.
-print '\nCtrl-C detected, goodbye.'
+print('\nCtrl-C detected, goodbye.')
 if args.fix:
   shutil.rmtree(tmpdir)
 os.kill(0, 9)
 
   if args.fix:
-print 'Applying fixes ...'
-apply_fixes(args, tmpdir)
+print('Applying fixes ...')
+successfully_applied = False
+
+try:
+  apply_fixes(args, tmpdir)
+  successfully_applied = True
+except:
+  print('Error applying fixes.\n', file=sys.stderr)
+  traceback.print_exc()
+
+shutil.rmtree(tmpdir)
+if not successfully_applied:
+  sys.exit(1)
 
 if __name__ == '__main__':
   main()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

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



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:115
+  const auto *MakePairCall = Result.Nodes.getNodeAs("make_pair");
+  assert(InnerCtorCall || MakePairCall);
 

Prazek wrote:
> JDevlieghere wrote:
> > It's highly recommended to put some kind of error message in the assertion 
> > statement, as per the LLVM coding standards.
> would it be better to change it to
> !innerCtorCall && !MakePairCall && "No .."
I don't think that this logic would work here. `!first && !second` ensures both 
are null.


https://reviews.llvm.org/D32395



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

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96634.
kuhar added a comment.

Use igoringImplicit instead of ignoringParenImpCasts. Options moved to the 
anonymous namespace.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,51 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The examples below illustrate the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
+
+  struct X {
+X(std::pair) {}
+  };
+  std::vector x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
+
+  struct Y {
+Y(std::pair&&) {}
+  };
+  std::vector y;
+  y.push_back(std::make_pair(2, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: y.emplace_back(std::make_pair(2, 3));
+  // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -36,8 +36,7 @@
 
 std::vector> w;
 w.emplace_back(21, 37);
-// This will be fixed to w.emplace_back(21L, 37L); in next version
-w.emplace_back(std::make_pair(21L, 37L);
+w.emplace_back(21L, 37L);
 
 The other situation is when we pass arguments that will be converted to a type
 inside a container.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
 
+- Improved `modernize-use-emplace
+  `_ check
+
+  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
+  into emplace_back(a, b).
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,10 +15,16 @@
 namespace tidy {
 namespace modernize {
 
-static const auto DefaultContainersWithPushBack =
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+
+const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
-static const auto DefaultSmartPointers =
+const auto DefaultSmartPointers =
 "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
+} // namespace
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
@@ -39,7 +45,6 @@
   // because this requires special treatment 

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96622.
kuhar marked 4 inline comments as done.
kuhar added a comment.

Added const where possible, moved from if-else to ternary.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,51 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The examples below illustrate the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
+
+  struct X {
+X(std::pair) {}
+  };
+  std::vector x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
+
+  struct Y {
+Y(std::pair&&) {}
+  };
+  std::vector y;
+  y.push_back(std::make_pair(2, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: y.emplace_back(std::make_pair(2, 3));
+  // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -36,8 +36,7 @@
 
 std::vector> w;
 w.emplace_back(21, 37);
-// This will be fixed to w.emplace_back(21L, 37L); in next version
-w.emplace_back(std::make_pair(21L, 37L);
+w.emplace_back(21L, 37L);
 
 The other situation is when we pass arguments that will be converted to a type
 inside a container.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
 
+- Improved `modernize-use-emplace
+  `_ check
+
+  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
+  into emplace_back(a, b).
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,43 +86,64 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added a comment.

Thanks for the suggestions, Jonas. Fixed.


https://reviews.llvm.org/D32395



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

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar requested review of this revision.
kuhar added a comment.

Is the current patch OK? Do you have any other comments?


https://reviews.llvm.org/D32294



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

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96384.
kuhar added a comment.

Don't remove make_pair calls when inserted type is different than std::pair.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,51 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The examples below illustrate the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
+
+  struct X {
+X(std::pair) {}
+  };
+  std::vector x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
+
+  struct Y {
+Y(std::pair&&) {}
+  };
+  std::vector y;
+  y.push_back(std::make_pair(2, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: y.emplace_back(std::make_pair(2, 3));
+  // make_pair cannot be removed here, as Y is not constructible with two ints.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -36,8 +36,7 @@
 
 std::vector> w;
 w.emplace_back(21, 37);
-// This will be fixed to w.emplace_back(21L, 37L); in next version
-w.emplace_back(std::make_pair(21L, 37L);
+w.emplace_back(21L, 37L);
 
 The other situation is when we pass arguments that will be converted to a type
 inside a container.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
 
+- Improved `modernize-use-emplace
+  `_ check
+
+  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
+  into emplace_back(a, b).
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,33 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+  declRefExpr(

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar added inline comments.



Comment at: test/clang-tidy/modernize-use-emplace.cpp:284
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));

kuhar wrote:
> Prazek wrote:
> > I would add here test like:
> > 
> >   class X {
> > X(std:;pair a) {}
> >   };
> >   
> >   std::vector v;
> >   v.push_back(make_pair(42, 42));
> >   
> > I guess as long as X ctor is not explicit this can happen, and we can't 
> > transform it to
> > emplace.back(42, 42)
> Nice idea for a test case, added.
A better test case would be to make X's ctor take a pair by const reference or 
rvalue reference. This way it wouldn't produce an extra CxxConstructExpr and 
will be currently (incorrectly) matched. This needs to be fixed.


https://reviews.llvm.org/D32395



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

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96332.
kuhar marked an inline comment as done.
kuhar added a comment.

Update modernize-use-emplace rst docs.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,42 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The example below illustrates the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
+
+  struct X {
+X(std::pair) {}
+  };
+  std::vector x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/clang-tidy/checks/modernize-use-emplace.rst
===
--- docs/clang-tidy/checks/modernize-use-emplace.rst
+++ docs/clang-tidy/checks/modernize-use-emplace.rst
@@ -36,8 +36,7 @@
 
 std::vector> w;
 w.emplace_back(21, 37);
-// This will be fixed to w.emplace_back(21L, 37L); in next version
-w.emplace_back(std::make_pair(21L, 37L);
+w.emplace_back(21L, 37L);
 
 The other situation is when we pass arguments that will be converted to a type
 inside a container.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
 
+- Improved `modernize-use-emplace
+  `_ check
+
+  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
+  into emplace_back(a, b).
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,31 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+  declRefExpr(unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasName("::std::make_pair"
+  .bind("make_pair"));
+
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporary

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

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



Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:93
+  to(functionDecl(hasName("::std::make_pair"
+  
+  .bind("make_pair"));

Prazek wrote:
> JonasToth wrote:
> > is the new line here necessary? i think it looks better if the `.bind` is 
> > on this line.
> Better question is "is it clang formated?"
It was clang-formatted, but I do agree that it looked a bit weird. I removed 
the extra newline.



Comment at: test/clang-tidy/modernize-use-emplace.cpp:284
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));

Prazek wrote:
> I would add here test like:
> 
>   class X {
> X(std:;pair a) {}
>   };
>   
>   std::vector v;
>   v.push_back(make_pair(42, 42));
>   
> I guess as long as X ctor is not explicit this can happen, and we can't 
> transform it to
> emplace.back(42, 42)
Nice idea for a test case, added.


https://reviews.llvm.org/D32395



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

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96329.
kuhar added a comment.

Add test for pair constructor argument, mention changes in release notes.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,42 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The example below illustrates the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
+
+  struct X {
+X(std::pair) {}
+  };
+  std::vector x;
+  x.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: x.emplace_back(std::make_pair(1, 2));
+  // make_pair cannot be removed here, as X is not constructible with two ints.
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -101,6 +101,12 @@
   Finds possible inefficient vector operations in for loops that may cause
   unnecessary memory reallocations.
 
+- Improved `modernize-use-emplace
+  `_ check
+
+  Removes unnecessary std::make_pair calls in push_back(std::make_pair(a, b)) calls and turns them
+  into emplace_back(a, b).
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,31 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+  declRefExpr(unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasName("::std::make_pair"
+  .bind("make_pair"));
+
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(makePair;
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  auto soughtParam = materializeTemporaryExpr(
+  anyOf(has(makePair), has(makePairCtor),
+hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+
+  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(soughtParam),
unless(isInTemplateInstantiation()))
  .bind("call"),
  this);
 }
 
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Call = Result.Nodes.getNod

[PATCH] D32395: [clang-tidy] modernize-use-emplace: remove unnecessary make_pair calls

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added a project: clang-tools-extra.

When there is a push_back with a call to make_pair, turn it into emplace_back 
and remove the unnecessary make_pair call.

Eg.

  std::vector> v;
  v.push_back(std::make_pair(1, 2)); // --> v.emplace_back(1, 2);

make_pair doesn't get removed when explicit template parameters are provided, 
because of potential problems with type conversions.


https://reviews.llvm.org/D32395

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -53,8 +53,8 @@
 };
 
 template 
-pair make_pair(T1, T2) {
-  return pair();
+pair make_pair(T1&&, T2&&) {
+  return {};
 };
 
 template 
@@ -274,18 +274,33 @@
 
 void testMakePair() {
   std::vector> v;
-  // FIXME: add functionality to change calls of std::make_pair
   v.push_back(std::make_pair(1, 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
 
-  // FIXME: This is not a bug, but call of make_pair should be removed in the
-  // future. This one matches because the return type of make_pair is different
-  // than the pair itself.
   v.push_back(std::make_pair(42LL, 13));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
-  // CHECK-FIXES: v.emplace_back(std::make_pair(42LL, 13));
+  // CHECK-FIXES: v.emplace_back(42LL, 13);
+
+  v.push_back(std::make_pair(0, 3));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(0, 3));
+  //
+  // Even though the call above could be turned into v.emplace_back(0, 3),
+  // we don't eliminate the make_pair call here, because of the explicit
+  // template parameters provided. make_pair's arguments can be convertible
+  // to its explicitly provided template parameter, but not to the pair's
+  // element type. The example below illustrates the problem.
+  struct D {
+D(...) {}
+operator char() const { return 0; }
+  };
+  v.push_back(std::make_pair(Something(), 2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(std::make_pair(Something(), 2));
 }
 
-void testOtherCointainers() {
+void testOtherContainers() {
   std::list l;
   l.push_back(Something(42, 41));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -15,6 +15,12 @@
 namespace tidy {
 namespace modernize {
 
+namespace {
+AST_MATCHER(DeclRefExpr, hasExplicitTemplateArgs) {
+  return Node.hasExplicitTemplateArgs();
+}
+} // namespace
+
 static const auto DefaultContainersWithPushBack =
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
@@ -80,18 +86,32 @@
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 
-  auto ctorAsArgument = materializeTemporaryExpr(
-  anyOf(hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+  auto makePair = ignoringImplicit(
+  callExpr(callee(expr(ignoringParenImpCasts(
+  declRefExpr(unless(hasExplicitTemplateArgs()),
+  to(functionDecl(hasName("::std::make_pair"
+  
+  .bind("make_pair"));
+
+  // make_pair can return type convertible to container's element type.
+  auto makePairCtor = ignoringImplicit(cxxConstructExpr(
+  has(materializeTemporaryExpr(makePair;
 
-  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(ctorAsArgument),
+  auto soughtParam = materializeTemporaryExpr(
+  anyOf(has(makePair), has(makePairCtor),
+hasConstructExpr, has(cxxFunctionalCastExpr(hasConstructExpr;
+
+  Finder->addMatcher(cxxMemberCallExpr(callPushBack, has(soughtParam),
unless(isInTemplateInstantiation()))
  .bind("call"),
  this);
 }
 
 void UseEmplaceCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Call = Result.Nodes.getNodeAs("call");
   const auto *InnerCtorCall = Result.Nodes.getNodeAs("ctor");
+  const auto *MakePairCall = Result.Nodes.getNodeAs("make_pair");
+  assert(InnerCtorCall || MakePairCall);
 
   auto FunctionNameSourceRange = CharSourceRange::getCharRange(
   Call->getExprLoc(), Call->getArg(0)->getExprLoc());
@@ -101,22 +121,34 @@
   if (FunctionNameSourceRange.getBegin().isMacroID())
 return;
 
+  const auto *EmplacePrefix = MakePairCall ? "emplace_back" : "emplace_back(";
   Diag << FixItHint::CreateReplacement(FunctionNameSourceRange,
-   "emplace_back(");
+

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96097.
kuhar added a comment.

Minor change to success detection logic.


https://reviews.llvm.org/D32294

Files:
  clang-tidy/tool/run-clang-tidy.py

Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -34,6 +34,7 @@
 http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 """
 
+from __future__ import print_function
 import argparse
 import json
 import multiprocessing
@@ -45,14 +46,15 @@
 import sys
 import tempfile
 import threading
+import traceback
 
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
 if os.path.realpath(result) == '/':
-  print 'Error: could not find compilation database.'
+  print('Error: could not find compilation database.')
   sys.exit(1)
 result += '../'
   return os.path.realpath(result)
@@ -87,14 +89,24 @@
   return start
 
 
+def check_clang_apply_replacements_binary(args):
+  """Checks if invoking supplied clang-apply-replacements binary works."""
+  try:
+subprocess.check_call([args.clang_apply_replacements_binary, '--version'])
+  except:
+print('Unable to run clang-apply-replacements. Is clang-apply-replacements '
+  'binary correctly specified?', file=sys.stderr)
+traceback.print_exc()
+sys.exit(1)
+
+
 def apply_fixes(args, tmpdir):
   """Calls clang-apply-fixes on a given directory. Deletes the dir when done."""
   invocation = [args.clang_apply_replacements_binary]
   if args.format:
 invocation.append('-format')
   invocation.append(tmpdir)
   subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
 
 
 def run_tidy(args, tmpdir, build_path, queue):
@@ -164,9 +176,9 @@
 if args.checks:
   invocation.append('-checks=' + args.checks)
 invocation.append('-')
-print subprocess.check_output(invocation)
+print(subprocess.check_output(invocation))
   except:
-print >>sys.stderr, "Unable to run clang-tidy."
+print("Unable to run clang-tidy.", file=sys.stderr)
 sys.exit(1)
 
   # Load the database and extract all files.
@@ -179,6 +191,7 @@
 
   tmpdir = None
   if args.fix:
+check_clang_apply_replacements_binary(args)
 tmpdir = tempfile.mkdtemp()
 
   # Build up a big regexy filter from all command line arguments.
@@ -204,14 +217,25 @@
   except KeyboardInterrupt:
 # This is a sad hack. Unfortunately subprocess goes
 # bonkers with ctrl-c and we start forking merrily.
-print '\nCtrl-C detected, goodbye.'
+print('\nCtrl-C detected, goodbye.')
 if args.fix:
   shutil.rmtree(tmpdir)
 os.kill(0, 9)
 
   if args.fix:
-print 'Applying fixes ...'
-apply_fixes(args, tmpdir)
+print('Applying fixes ...')
+successfully_applied = False
+
+try:
+  apply_fixes(args, tmpdir)
+  successfully_applied = True
+except:
+  print('Error applying fixes.\n', file=sys.stderr)
+  traceback.print_exc()
+
+shutil.rmtree(tmpdir)
+if not successfully_applied:
+  sys.exit(1)
 
 if __name__ == '__main__':
   main()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96045.
kuhar added a comment.

After thinking about Piotr's comment, I think that a better way to perform the 
check would be te invoking clang-apply-replacements with `--version` and seeing 
if it fails even before running clang-tidy.
This way it is possible to save a lot of time when run-clang-tidy.py is run on 
a big project and apply_fixes fails at the end.

Let me know what you think about this approach.


https://reviews.llvm.org/D32294

Files:
  clang-tidy/tool/run-clang-tidy.py

Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -34,6 +34,7 @@
 http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 """
 
+from __future__ import print_function
 import argparse
 import json
 import multiprocessing
@@ -45,14 +46,15 @@
 import sys
 import tempfile
 import threading
+import traceback
 
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
 if os.path.realpath(result) == '/':
-  print 'Error: could not find compilation database.'
+  print('Error: could not find compilation database.')
   sys.exit(1)
 result += '../'
   return os.path.realpath(result)
@@ -86,15 +88,24 @@
   start.append(f)
   return start
 
+def check_clang_apply_replacements_binary(args):
+  """Checks if invoking supplied clang-apply-replacements binary works."""
+  try:
+subprocess.check_call([args.clang_apply_replacements_binary, '--version'])
+  except:
+print('Unable to run clang-apply-replacements. Is clang-apply-replacements '
+  'binary correctly specified?', file=sys.stderr)
+traceback.print_exc()
+sys.exit(1)
+
 
 def apply_fixes(args, tmpdir):
   """Calls clang-apply-fixes on a given directory. Deletes the dir when done."""
   invocation = [args.clang_apply_replacements_binary]
   if args.format:
 invocation.append('-format')
   invocation.append(tmpdir)
   subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
 
 
 def run_tidy(args, tmpdir, build_path, queue):
@@ -164,9 +175,9 @@
 if args.checks:
   invocation.append('-checks=' + args.checks)
 invocation.append('-')
-print subprocess.check_output(invocation)
+print(subprocess.check_output(invocation))
   except:
-print >>sys.stderr, "Unable to run clang-tidy."
+print("Unable to run clang-tidy.", file=sys.stderr)
 sys.exit(1)
 
   # Load the database and extract all files.
@@ -179,6 +190,7 @@
 
   tmpdir = None
   if args.fix:
+check_clang_apply_replacements_binary(args)
 tmpdir = tempfile.mkdtemp()
 
   # Build up a big regexy filter from all command line arguments.
@@ -204,14 +216,25 @@
   except KeyboardInterrupt:
 # This is a sad hack. Unfortunately subprocess goes
 # bonkers with ctrl-c and we start forking merrily.
-print '\nCtrl-C detected, goodbye.'
+print('\nCtrl-C detected, goodbye.')
 if args.fix:
   shutil.rmtree(tmpdir)
 os.kill(0, 9)
 
   if args.fix:
-print 'Applying fixes ...'
-apply_fixes(args, tmpdir)
+print('Applying fixes ...')
+successfully_applied = True
+
+try:
+  apply_fixes(args, tmpdir)
+except:
+  successfully_applied = False
+  print('Error applying fixes.\n', file=sys.stderr)
+  traceback.print_exc()
+
+shutil.rmtree(tmpdir)
+if not successfully_applied:
+  sys.exit(1)
 
 if __name__ == '__main__':
   main()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32313: [clang-tidy] Don't turn .push_back({1, 2}) into .emplace_back(1, 2) for pairs

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added a project: clang-tools-extra.

This patch prevents modernize-use-emplace from changing push_backs of brace 
initialized pairs (`.push_back({1, 2})`) to `.emplace_back(1, 2)`.
Pair's constructor doesn't have any interesting logic and basically performs 
aggregate initialization. There also doesn't seem to be much win
in terms of making code more concise, thus is makes little sense to turn such 
push_back calls into emplace_backs.

The change is inspired by the recent discussion on changing and enforcing 
coding guidelines:
[llvm-dev] [cfe-dev] Modernizing LLVM Coding Style Guide andenforcing 
Clang-tidy
http://lists.llvm.org/pipermail/llvm-dev/2016-December/108559.html


https://reviews.llvm.org/D32313

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  test/clang-tidy/modernize-use-emplace.cpp


Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -186,6 +186,11 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(1, 2);
 
+  v.push_back({1, 2});
+  v.push_back(std::pair{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
   GetPair g;
   v.push_back(g.getPair());
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: clang-tidy/modernize/UseEmplaceCheck.h
===
--- clang-tidy/modernize/UseEmplaceCheck.h
+++ clang-tidy/modernize/UseEmplaceCheck.h
@@ -35,6 +35,7 @@
 private:
   std::vector ContainersWithPushBack;
   std::vector SmartPointers;
+  std::vector PairTypes;
 };
 
 } // namespace modernize
Index: clang-tidy/modernize/UseEmplaceCheck.cpp
===
--- clang-tidy/modernize/UseEmplaceCheck.cpp
+++ clang-tidy/modernize/UseEmplaceCheck.cpp
@@ -19,13 +19,16 @@
 "::std::vector; ::std::list; ::std::deque";
 static const auto DefaultSmartPointers =
 "::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
+static const auto DefaultPairTypes = "::std::pair";
 
 UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   ContainersWithPushBack(utils::options::parseStringList(Options.get(
   "ContainersWithPushBack", DefaultContainersWithPushBack))),
   SmartPointers(utils::options::parseStringList(
-  Options.get("SmartPointers", DefaultSmartPointers))) {}
+  Options.get("SmartPointers", DefaultSmartPointers))),
+  PairTypes(utils::options::parseStringList(
+  Options.get("PairTypes", DefaultPairTypes))) {}
 
 void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
@@ -72,11 +75,19 @@
   auto hasInitList = has(ignoringImplicit(initListExpr()));
   // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
   // overloaded functions and template names.
+
+  // Ignore push_back({first, second}) for pair types (eg. std::pair).
+  auto isPairBraceInit = expr(allOf(cxxConstructExpr(hasDeclaration(
+  cxxConstructorDecl(ofClass(hasAnyName(SmallVector(
+  PairTypes.begin(), PairTypes.end())),
+unless(cxxTemporaryObjectExpr())),
+  unless(hasParent(implicitCastExpr(;
+
   auto soughtConstructExpr =
   cxxConstructExpr(
   unless(anyOf(isCtorOfSmartPtr, hasInitList, bitFieldAsArgument,
initializerListAsArgument, newExprAsArgument,
-   constructingDerived, isPrivateCtor)))
+   constructingDerived, isPrivateCtor, isPairBraceInit)))
   .bind("ctor");
   auto hasConstructExpr = has(ignoringImplicit(soughtConstructExpr));
 


Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -186,6 +186,11 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(1, 2);
 
+  v.push_back({1, 2});
+  v.push_back(std::pair{1, 2});
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(1, 2);
+
   GetPair g;
   v.push_back(g.getPair());
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
Index: clang-tidy/modernize/UseEmplaceCheck.h
===
--- clang-tidy/modernize/UseEmplaceCheck.h
+++ clang-tidy/modernize/UseEmplaceCheck.h
@@ -35,6 +35,7 @@
 private:
   std::vector ContainersWithPushBack;
   std::vector SmartPointers;
+  std::vector PairTypes;
 };
 
 } // namespace modernize
Index: clang-tidy/moderniz

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Jakub Kuderski via Phabricator via cfe-commits]

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



Comment at: clang-tidy/tool/run-clang-tidy.py:100
+  except:
+print >>sys.stderr, "Unable to run clang-apply-replacements."
+sys.exit(1)

kimgr wrote:
> alexfh wrote:
> > "Unable to run clang-apply-replacements" without any details seems to be 
> > far worse than just a default stack trace from an unhandled exception. At 
> > the very least add the message from the exception.
> I don't know what the general Python3 ambitions of this script is, but it 
> would be nice if the new code didn't use the Python2-only print style.
> 
> You can:
> 
> from __future__ import print_function
> 
> at the top of the file, and then use Python3-style print:
> 
> print("Unable to run clang-apply-replacements", file=sys.stderr)
Thanks for feedback. I changed the code to print suggestion about 
clang-apply-replacements and stack trace. Is there a better way to make it easy 
to debug?


https://reviews.llvm.org/D32294



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

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar updated this revision to Diff 96001.
kuhar added a comment.

Use python3 printing. Move exception handling out of apply_fixes.

Now, the code prints the following on failure:

  Applying fixes ...
  Error applying fixes. Is clang-apply-replacements binary correctly specified?
  
  Traceback (most recent call last):
File 
"/home/kuhar/projects/llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py",
 line 217, in main
  apply_fixes(args, tmpdir)
File 
"/home/kuhar/projects/llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py",
 line 98, in apply_fixes
  subprocess.call(invocation)
File "/usr/lib/python2.7/subprocess.py", line 523, in call
  return Popen(*popenargs, **kwargs).wait()
File "/usr/lib/python2.7/subprocess.py", line 711, in __init__
  errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1343, in _execute_child
  raise child_exception
  OSError: [Errno 2] No such file or directory

I hope that suggesting problem with clang-apply-replacements binary and 
printing stack trace is more useful.


https://reviews.llvm.org/D32294

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -34,6 +34,7 @@
 http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 """
 
+from __future__ import print_function
 import argparse
 import json
 import multiprocessing
@@ -45,14 +46,15 @@
 import sys
 import tempfile
 import threading
+import traceback
 
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
 if os.path.realpath(result) == '/':
-  print 'Error: could not find compilation database.'
+  print('Error: could not find compilation database.')
   sys.exit(1)
 result += '../'
   return os.path.realpath(result)
@@ -94,7 +96,6 @@
 invocation.append('-format')
   invocation.append(tmpdir)
   subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
 
 
 def run_tidy(args, tmpdir, build_path, queue):
@@ -164,9 +165,9 @@
 if args.checks:
   invocation.append('-checks=' + args.checks)
 invocation.append('-')
-print subprocess.check_output(invocation)
+print(subprocess.check_output(invocation))
   except:
-print >>sys.stderr, "Unable to run clang-tidy."
+print("Unable to run clang-tidy.", file=sys.stderr)
 sys.exit(1)
 
   # Load the database and extract all files.
@@ -204,14 +205,22 @@
   except KeyboardInterrupt:
 # This is a sad hack. Unfortunately subprocess goes
 # bonkers with ctrl-c and we start forking merrily.
-print '\nCtrl-C detected, goodbye.'
+print('\nCtrl-C detected, goodbye.')
 if args.fix:
   shutil.rmtree(tmpdir)
 os.kill(0, 9)
 
   if args.fix:
-print 'Applying fixes ...'
-apply_fixes(args, tmpdir)
+print('Applying fixes ...')
+
+try:
+  apply_fixes(args, tmpdir)
+except:
+  print('Error applying fixes. Is clang-apply-replacements binary '
+'correctly specified?\n', file=sys.stderr)
+  traceback.print_exc()
+
+shutil.rmtree(tmpdir)
 
 if __name__ == '__main__':
   main()


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -34,6 +34,7 @@
 http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 """
 
+from __future__ import print_function
 import argparse
 import json
 import multiprocessing
@@ -45,14 +46,15 @@
 import sys
 import tempfile
 import threading
+import traceback
 
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
 if os.path.realpath(result) == '/':
-  print 'Error: could not find compilation database.'
+  print('Error: could not find compilation database.')
   sys.exit(1)
 result += '../'
   return os.path.realpath(result)
@@ -94,7 +96,6 @@
 invocation.append('-format')
   invocation.append(tmpdir)
   subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
 
 
 def run_tidy(args, tmpdir, build_path, queue):
@@ -164,9 +165,9 @@
 if args.checks:
   invocation.append('-checks=' + args.checks)
 invocation.append('-')
-print subprocess.check_output(invocation)
+print(subprocess.check_output(invocation))
   except:
-print >>sys.stderr, "Unable to run clang-tidy."
+print("Unable to run clang-tidy.", file=sys.stderr)
 sys.exit(1)
 
   # Load the database and extract all files.
@@ -204,14 +205,22 @@
   except KeyboardInterrupt:
 # This is a sad hack. Unfortunately subprocess goes
 # bonkers with ctrl-c and we start forking merrily.
-print '\nCtrl-C detected, good

[PATCH] D32294: [clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar created this revision.
kuhar added a project: clang-tools-extra.

When running run-clang-tidy.py with -fix it tries to apply found replacements 
at the end.
If there are errors running clang-apply-replacements, the script currently 
crashes or displays no error at all.

This patch checks for errors running clang-apply-replacements the same way 
clang-tidy binary is handled.

Another option would be probably checking for clang-apply-replacements (when 
-fix is passed) even before running clang-tidy.


https://reviews.llvm.org/D32294

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -89,12 +89,16 @@
 
 def apply_fixes(args, tmpdir):
   """Calls clang-apply-fixes on a given directory. Deletes the dir when 
done."""
-  invocation = [args.clang_apply_replacements_binary]
-  if args.format:
-invocation.append('-format')
-  invocation.append(tmpdir)
-  subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
+  try:
+invocation = [args.clang_apply_replacements_binary]
+if args.format:
+  invocation.append('-format')
+invocation.append(tmpdir)
+subprocess.call(invocation)
+shutil.rmtree(tmpdir)
+  except:
+print >>sys.stderr, "Unable to run clang-apply-replacements."
+sys.exit(1)
 
 
 def run_tidy(args, tmpdir, build_path, queue):


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -89,12 +89,16 @@
 
 def apply_fixes(args, tmpdir):
   """Calls clang-apply-fixes on a given directory. Deletes the dir when done."""
-  invocation = [args.clang_apply_replacements_binary]
-  if args.format:
-invocation.append('-format')
-  invocation.append(tmpdir)
-  subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
+  try:
+invocation = [args.clang_apply_replacements_binary]
+if args.format:
+  invocation.append('-format')
+invocation.append(tmpdir)
+subprocess.call(invocation)
+shutil.rmtree(tmpdir)
+  except:
+print >>sys.stderr, "Unable to run clang-apply-replacements."
+sys.exit(1)
 
 
 def run_tidy(args, tmpdir, build_path, queue):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits