[clang-tools-extra] r322497 - [clang-tidy] Expand readability-redundant-smartptr-get to understand implicit converions to bool in more contexts.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Mon Jan 15 10:03:20 2018
New Revision: 322497

URL: http://llvm.org/viewvc/llvm-project?rev=322497=rev
Log:
[clang-tidy] Expand readability-redundant-smartptr-get to understand implicit 
converions to bool in more contexts.

Summary: Expand readability-redundant-smartptr-get to understand implicit 
converions to bool in more contexts.

Reviewers: hokein

Subscribers: klimek, xazax.hun, cfe-commits

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

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

clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp?rev=322497=322496=322497=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp 
(original)
+++ 
clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp 
Mon Jan 15 10:03:20 2018
@@ -51,6 +51,20 @@ void registerMatchersForGetArrowStart(Ma
   unaryOperator(hasOperatorName("*"),
 hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
   Callback);
+
+  // Catch '!ptr.get()'
+  const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(recordDecl(
+  QuacksLikeASmartptr, has(cxxConversionDecl(returns(booleanType()));
+  Finder->addMatcher(
+  unaryOperator(hasOperatorName("!"), hasUnaryOperand(CallToGetAsBool)),
+  Callback);
+
+  // Catch 'if(ptr.get())'
+  Finder->addMatcher(ifStmt(hasCondition(CallToGetAsBool)), Callback);
+
+  // Catch 'ptr.get() ? X : Y'
+  Finder->addMatcher(conditionalOperator(hasCondition(CallToGetAsBool)),
+ Callback);
 }
 
 void registerMatchersForGetEquals(MatchFinder *Finder,
@@ -72,11 +86,6 @@ void registerMatchersForGetEquals(MatchF
  hasEitherOperand(callToGet(IsAKnownSmartptr))),
   Callback);
 
-  // Matches against if(ptr.get())
-  Finder->addMatcher(
-  ifStmt(hasCondition(ignoringImpCasts(callToGet(IsAKnownSmartptr,
-  Callback);
-
   // FIXME: Match and fix if (l.get() == r.get()).
 }
 

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp?rev=322497=322496=322497=diff
==
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp 
(original)
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp 
Mon Jan 15 10:03:20 2018
@@ -9,6 +9,7 @@ struct unique_ptr {
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 template 
@@ -16,6 +17,7 @@ struct shared_ptr {
   T& operator*() const;
   T* operator->() const;
   T* get() const;
+  explicit operator bool() const noexcept;
 };
 
 }  // namespace std
@@ -28,6 +30,7 @@ struct BarPtr {
   Bar* operator->();
   Bar& operator*();
   Bar* get();
+  explicit operator bool() const;
 };
 struct int_ptr {
   int* get();
@@ -110,6 +113,23 @@ void Positive() {
   // CHECK-MESSAGES: uu.get() == nullptr;
   // CHECK-FIXES: bool bb = uu == nullptr;
 
+  if (up->get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (up->get());
+  // CHECK-FIXES: if (*up);
+  if ((uu.get()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: if ((uu.get()));
+  // CHECK-FIXES: if ((uu));
+  bb = !ss->get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant get() call
+  // CHECK-MESSAGES: bb = !ss->get();
+  // CHECK-FIXES: bb = !*ss;
+  bb = u.get() ? true : false;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: bb = u.get() ? true : false;
+  // CHECK-FIXES: bb = u ? true : false;
+
   bb = nullptr != ss->get();
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
   // CHECK-MESSAGES: nullptr != ss->get();
@@ -146,10 +166,6 @@ void Positive() {
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant get() call
   // CHECK-MESSAGES: if (NULL == x.get());
   // CHECK-FIXES: if (NULL == x);
-  if (x.get());
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
-  // CHECK-MESSAGES: if (x.get());
-  // CHECK-FIXES: if (x);
 }
 
 void Negative() {
@@ -175,4 +191,6 @@ void Negative() {
 
   int_ptr ip;
   bool bb = ip.get() == nullptr;
+  bb = !ip.get();
+  bb = ip.get() ? true : false;
 }


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

[clang-tools-extra] r322002 - [clang-tidy] Fix DanglingHandleCheck for the correct conversion operation between basic_string and basic_string_view.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Mon Jan  8 07:59:08 2018
New Revision: 322002

URL: http://llvm.org/viewvc/llvm-project?rev=322002=rev
Log:
[clang-tidy] Fix DanglingHandleCheck for the correct conversion operation 
between basic_string and basic_string_view.

Summary:
Fix DanglingHandleCheck to handle the final implementation of std::string and 
std::string_view.
These use a conversion operator instead of a conversion constructor.

Reviewers: hokein

Subscribers: klimek, xazax.hun, cfe-commits

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

Modified:
clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp

Modified: clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp?rev=322002=322001=322002=diff
==
--- clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/bugprone/DanglingHandleCheck.cpp Mon Jan 
 8 07:59:08 2018
@@ -25,8 +25,12 @@ namespace {
 ast_matchers::internal::BindableMatcher
 handleFrom(const ast_matchers::internal::Matcher ,
const ast_matchers::internal::Matcher ) {
-  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
-  hasArgument(0, Arg));
+  return expr(
+  anyOf(cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+ hasArgument(0, Arg)),
+cxxMemberCallExpr(hasType(cxxRecordDecl(IsAHandle)),
+  callee(memberExpr(member(cxxConversionDecl(,
+  on(Arg;
 }
 
 ast_matchers::internal::Matcher handleFromTemporaryValue(

Modified: clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp?rev=322002=322001=322002=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp 
(original)
+++ clang-tools-extra/trunk/test/clang-tidy/bugprone-dangling-handle.cpp Mon 
Jan  8 07:59:08 2018
@@ -45,10 +45,15 @@ class map {
   value_type& operator[](Key&& key);
 };
 
+class basic_string_view;
+
 class basic_string {
  public:
   basic_string();
   basic_string(const char*);
+
+  operator basic_string_view() const noexcept;
+
   ~basic_string();
 };
 
@@ -57,7 +62,6 @@ typedef basic_string string;
 class basic_string_view {
  public:
   basic_string_view(const char*);
-  basic_string_view(const basic_string&);
 };
 
 typedef basic_string_view string_view;


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

Re: [PATCH] D24339: [clang-tidy] Add check 'readability-redundant-member-init'

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:33
@@ +32,3 @@
+  const auto *Init = Result.Nodes.getNodeAs("init");
+  const auto *Construct = 
Result.Nodes.getNodeAs("construct");
+  const auto arguments = Construct->arguments();

These construct expressions might actually have side effects that you would not 
get if you omit them.
For example, aggregates will be value initialized. If you remove it you change 
the behavior.

The tests should include defaulted vs non-defaulted default constructor, 
user-defined/user-provided/implicit default constructor, aggregates with and 
without trivially constructible members, etc.


Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:34
@@ +33,3 @@
+  const auto *Construct = 
Result.Nodes.getNodeAs("construct");
+  const auto arguments = Construct->arguments();
+

Prazek wrote:
> Arguments (upper case)
Arguments variable name. (should start with upper case)


Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:36
@@ +35,3 @@
+
+  using std::begin;
+  using std::end;

There's no need for these using declarations.
Just do arguments.begin()/arguments.end().
They are using in generic code. This is not generic code.


Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:39
@@ +38,3 @@
+
+  if (std::find_if(begin(arguments), end(arguments), [](const Expr *expr) {
+return !expr->isDefaultArgument();

You want `std::none_of` instead of `std::find_if`.
(or use `std::any_of` and don't negate the expression in the lambda)


Comment at: docs/clang-tidy/checks/readability-redundant-member-init.rst:6
@@ +5,3 @@
+
+Finds unnecessary member initializations.  
 
+   
 

Explain when they are unnecessary.


Repository:
  rL LLVM

https://reviews.llvm.org/D24339



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

Re: [PATCH] D22220: [clang-tidy] Add check 'misc-move-forwarding-reference'

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:64
@@ +63,3 @@
+void MoveForwardingReferenceCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus11)
+return;

I'm guessing this is checking Lang==C++11, but we actually want Lang>=C++11, 
right?
I'm not sure if there is a way to check that.


Comment at: test/clang-tidy/misc-move-forwarding-reference.cpp:49
@@ +48,3 @@
+// Create a correct fix if there are spaces around the overload resolution
+// operator.
+template  void f5(U &) {

I don't understand what this is trying to test.
What is the overload resolution operator?
Also, these two cases look exactly the same as the previous two.


Comment at: test/clang-tidy/misc-move-forwarding-reference.cpp:117
@@ +116,2 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: forwarding reference passed to
+}

This is missing one case for C++14's generic lambdas.

```
[&] (auto&& x) { y = std::move(x); }

```
It might already detect correctly, but it will need a special case for 
generating the `std::forward`
It might need to be something like:

```
[&] (auto&& x) { y = std::forward(x); }

```
We can also just leave a TODO for later.


https://reviews.llvm.org/D0



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

Re: [PATCH] D22220: [clang-tidy] Add check 'misc-move-forwarding-reference'

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:93
@@ +92,3 @@
+   hasArgument(0, ignoringParenImpCasts(declRefExpr(
+  to(ForwardingReferenceParmMatcher)
+  .bind("call-move"),

aaron.ballman wrote:
> I wonder if there's a reason for this behavior, or if it's simple a matter of 
> not being needed previously and so it was never implemented. @sbenza or 
> @klimek may know. I think we should be fixing the RecursiveASTVisitor, unless 
> there is a valid reason not to (which there may be), though that would be a 
> separate patch (and can happen after we land this one).
Even if the nodes are not visited through the recursive visitor, you can always 
have a matcher for it.
Eg: `hasAnyConstructorInitializer` / `cxxCtorInitializer`.

But what node are you trying to visit here?
The only node I see is `NamingClass`, which is not really a child node.
Like the referred `Decl` in a `DeclRefExpr` is not a child either. You can't 
use `has()` there, you have to use `to()`.


https://reviews.llvm.org/D0



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

Re: [PATCH] Add support for the 'unless' matcher in the dynamic layer.

[Samuel Benzaquen via cfe-commits]

One of the reasons we added the minimum was because these nodes added
overhead to the matching that was not unnecessary when they only had a
single node.
On the current implementation we could actually get rid of the node
completely for the one argument calls.
I would be ok with removing the lower bound.

On Tue, Jul 26, 2016 at 6:20 PM, Zac Hansen  wrote:

> I was wondering if there is any objection to removing the 2-element
> minimum on the eachOf, anyOf and allOf matchers.
>
> It is frustrating when playing with matchers to have to edit significant
> amounts of code to be able to temporarily go from 2 to 1 matcher inside an
> any- or allOf matcher.
>
> And overall it feels very "un-set-theory"-like.
>
> The change was made here:
> https://github.com/llvm-mirror/clang/commit/674e54c167eab0be7a54bca7082c07d2f1d0c8cc
>
>
> Thank you and apologies if I sent this to the wrong lists/people.
>
> --Zac
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22220: [clang-tidy] Add check 'misc-move-forwarding-reference'

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:20
@@ +19,3 @@
+
+static void ReplaceMoveWithForward(const UnresolvedLookupExpr *Callee,
+   const TemplateTypeParmType *TypeParmType,

aaron.ballman wrote:
> Might as well pass `Context` by const reference rather than pointer.
Function names start in lower case.


Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:30
@@ +29,3 @@
+  Callee->getLocStart(),
+  Lexer::getLocForEndOfToken(Callee->getLocEnd(), 0, SM, LangOpts)),
+  SM, LangOpts);

Isn't this just 
`CharSourceRange::getTokenRange(Callee->getLocStart(),Callee->getLocEnd())` ?


Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:83
@@ +82,3 @@
+  // same function...
+  references(templateTypeParmType(hasDeclaration(hasDeclContext(
+  functionDecl().bind("context"

Will this trigger on code like this:


```
template 
void F(T);

void G() { F(std::move(str)); }
```


https://reviews.llvm.org/D0



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

[clang-tools-extra] r274019 - [clang-tidy] Do not match on lambdas.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Tue Jun 28 09:19:41 2016
New Revision: 274019

URL: http://llvm.org/viewvc/llvm-project?rev=274019=rev
Log:
[clang-tidy] Do not match on lambdas.

We match on the generated FunctionDecl of the lambda and try to fix it.
This causes a crash.
The right behavior is to ignore lambdas, because they are a definition.

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

clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp?rev=274019=274018=274019=diff
==
--- clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp 
Tue Jun 28 09:19:41 2016
@@ -32,10 +32,14 @@ SourceRange getTypeRange(const ParmVarDe
 void AvoidConstParamsInDecls::registerMatchers(MatchFinder *Finder) {
   const auto ConstParamDecl =
   parmVarDecl(hasType(qualType(isConstQualified(.bind("param");
-  Finder->addMatcher(functionDecl(unless(isDefinition()),
-  has(typeLoc(forEach(ConstParamDecl
- .bind("func"),
- this);
+  Finder->addMatcher(
+  functionDecl(unless(isDefinition()),
+   // Lambdas are always their own definition, but they
+   // generate a non-definition FunctionDecl too. Ignore those.
+   unless(cxxMethodDecl(ofClass(cxxRecordDecl(isLambda(),
+   has(typeLoc(forEach(ConstParamDecl
+  .bind("func"),
+  this);
 }
 
 // Re-lex the tokens to get precise location of last 'const'

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp?rev=274019=274018=274019=diff
==
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp
 (original)
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp
 Tue Jun 28 09:19:41 2016
@@ -90,3 +90,7 @@ void ConstNotVisible(CONCAT(cons, t) int
 // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: parameter 'i'
 // We warn, but we can't give a fix
 // CHECK-FIXES: void ConstNotVisible(CONCAT(cons, t) int i);
+
+// Regression test. We should not warn (or crash) on lambda expressions
+auto lambda_with_name = [](const int n) {};
+auto lambda_without_name = [](const int) {};


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

r274015 - [ASTMatchers] Add isLambda() matcher.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Tue Jun 28 09:08:56 2016
New Revision: 274015

URL: http://llvm.org/viewvc/llvm-project?rev=274015=rev
Log:
[ASTMatchers] Add isLambda() matcher.

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

Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=274015=274014=274015=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Tue Jun 28 09:08:56 2016
@@ -2137,6 +2137,17 @@ matches A and C::f, but not B, C, or B::
 
 
 
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html;>CXXRecordDeclisLambda
+Matches the generated 
class of lambda expressions.
+
+Given:
+  auto x = []{};
+
+cxxRecordDecl(isLambda()) matches the implicit class declaration of
+decltype(x)
+
+
+
 Matcherhttp://clang.llvm.org/doxygen/classclang_1_1CXXRecordDecl.html;>CXXRecordDeclisSameOrDerivedFromstd::string 
BaseName
 Overloaded 
method as shortcut for
 isSameOrDerivedFrom(hasName(...)).

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=274015=274014=274015=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Tue Jun 28 09:08:56 2016
@@ -2197,6 +2197,19 @@ AST_MATCHER_P(CXXRecordDecl, hasMethod,
 Node.method_end(), Finder, Builder);
 }
 
+/// \brief Matches the generated class of lambda expressions.
+///
+/// Given:
+/// \code
+///   auto x = []{};
+/// \endcode
+///
+/// \c cxxRecordDecl(isLambda()) matches the implicit class declaration of
+/// \c decltype(x)
+AST_MATCHER(CXXRecordDecl, isLambda) {
+  return Node.isLambda();
+}
+
 /// \brief Matches AST nodes that have child AST nodes that match the
 /// provided matcher.
 ///

Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp?rev=274015=274014=274015=diff
==
--- cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp Tue Jun 28 09:08:56 2016
@@ -307,6 +307,7 @@ RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(isInteger);
   REGISTER_MATCHER(isIntegral);
   REGISTER_MATCHER(isInTemplateInstantiation);
+  REGISTER_MATCHER(isLambda);
   REGISTER_MATCHER(isListInitialization);
   REGISTER_MATCHER(isMemberInitializer);
   REGISTER_MATCHER(isMoveAssignmentOperator);

Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp?rev=274015=274014=274015=diff
==
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (original)
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp Tue Jun 28 
09:08:56 2016
@@ -537,6 +537,12 @@ TEST(DeclarationMatcher, ClassIsDerived)
 cxxRecordDecl(isDerivedFrom(namedDecl(hasName("X"));
 }
 
+TEST(DeclarationMatcher, IsLambda) {
+  const auto IsLambda = cxxMethodDecl(ofClass(cxxRecordDecl(isLambda(;
+  EXPECT_TRUE(matches("auto x = []{};", IsLambda));
+  EXPECT_TRUE(notMatches("struct S { void operator()() const; };", IsLambda));
+}
+
 TEST(Matcher, BindMatchedNodes) {
   DeclarationMatcher ClassX = has(recordDecl(hasName("::X")).bind("x"));
 


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

Re: [PATCH] D20964: [clang-tidy] Add modernize-use-emplace

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:34
@@ +33,3 @@
+  hasDeclaration(functionDecl(hasName("push_back"))),
+  on(hasType(cxxRecordDecl(hasAnyName("std::vector", "llvm::SmallVector",
+  "std::list", "std::deque");

We should not hard code this list. Specially not non-standard types like 
llvm::SmallVector.
This should come from an option string.
For example, like we do in performance/FasterStringFindCheck.cpp


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:42
@@ +41,3 @@
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
+  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
+ "std::weak_ptr";

These are not the only smart pointers around.
It might be a good idea to make this list also configurable by the user.


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:45
@@ +44,3 @@
+
+  // Bitfields binds only to consts and emplace_back take it by universal ref.
+  auto bitFieldAsArgument = hasAnyArgument(ignoringParenImpCasts(

There are a few more things that can break here:
  - `NULL` can't be passed through perfect forwarding. Will be deduced as 
`long`.
  - Function references/pointers can't be passed through PF if they are 
overloaded.
  - Class scope static variables that have no definition can't be passed 
through PF because they will be ODR used.


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp:92
@@ +91,3 @@
+  // Range for constructor name and opening brace.
+  auto CtorCallSourceRange = CharSourceRange::getCharRange(
+  InnerCtorCall->getExprLoc(),

You should avoid using offsets with locations.
For example, you are hardcoding `{` as one character, which might not be true 
in the case of digraphs.

This should be `getTokenRange(InnerCtorCall->getExprLoc(), 
CallParensRange.getBegin())`

Same thing for the other one, it should be:

`CharSourceRange::getTokenRange(CallParensRange.getEnd(), 
CallParensRange.getEnd())`


Comment at: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h:23
@@ +22,3 @@
+/// constructor of temporary object.
+///`
+/// For the user-facing documentation see:

runaway qoute


Comment at: clang-tools-extra/trunk/clang-tidy/utils/Matchers.h:48
@@ -47,1 +47,3 @@
 
+AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); }
+

This matcher is generic enough that it should go into main ASTMatchers.h file.


Comment at: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst:17
@@ +16,3 @@
+
+   std::vector > w;
+   w.push_back(std::make_pair(21, 37));

Don't add the space between the >>. This is not needed since we are in C++11.


Comment at: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst:52
@@ +51,3 @@
+   std::vector v;
+   v.push_back(new int(5));
+   auto *ptr = int;

This doesn't compile. unique_ptr is not implicitly convertible from a pointer.


Comment at: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst:53
@@ +52,3 @@
+   v.push_back(new int(5));
+   auto *ptr = int;
+   v.push_back(ptr);

I think you meant `auto* ptr = new int(5)` ?


Comment at: clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp:76
@@ +75,3 @@
+};
+
+struct Convertable {

You should also try with a type that has a user-defined destructor.
It changes the AST enough to make a difference in many cases.

And you should fix all the std classes above to have user-defined constructors 
too.


Comment at: 
clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp:209
@@ +208,3 @@
+  std::vector v2;
+  v2.push_back(new int(42));
+  // This call can't be replaced with emplace_back.

This test is broken. `unique_ptr` should not have an implicit conversion 
from T*.


Comment at: 
clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp:338
@@ +337,2 @@
+  // CHECK-FIXES: v.emplace_back(42, var);
+}

It is also missing tests with templates.
ie: the container being a dependent type, and the value_type being a dependent 
type.
We should not change the code in either of those cases.


Repository:
  rL LLVM

http://reviews.llvm.org/D20964



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

Re: [PATCH] D21185: [clang-tidy] Add performance-emplace-into-containers

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Missing the .rst file.
Did you use the check_clang_tidy.py script to create the template?



Comment at: clang-tidy/performance/EmplaceCheck.cpp:25
@@ +24,3 @@
+  cxxMemberCallExpr(
+  on(expr(hasType(cxxRecordDecl(hasName("std::vector"),
+  callee(functionDecl(hasName("push_back"))),

This change can potentially break exception guarantees of the code.
For example, push_back can throw before the object is constructed leaking the 
arguments.
We should at least say something about it in the docs.


Comment at: clang-tidy/performance/EmplaceCheck.cpp:26
@@ +25,3 @@
+  on(expr(hasType(cxxRecordDecl(hasName("std::vector"),
+  callee(functionDecl(hasName("push_back"))),
+  hasArgument(0, cxxConstructExpr().bind("construct_expr")))

Just say functionDecl(hasName("std::vector::push_back")) and you can remove the 
type check.


Comment at: clang-tidy/performance/EmplaceCheck.cpp:84
@@ +83,3 @@
+
+  StringRef ArgsStr = getAsString(SM, LO, ArgsR);
+  if (!ArgsStr.data())

It's usually preferable, and sometimes easier to write, to do erases instead of 
replacements.
For example, you could simply erase `X(`  and  `)`.
Something like `(Cons->getLogStart(), ArgsR.getBegin())` and `(Args.getEnd(), 
Args.getEnd())`


Comment at: clang-tidy/performance/EmplaceCheck.cpp:88
@@ +87,3 @@
+
+  SourceRange CalleeR = getPreciseTokenRange(SM, LO, CalleeStartLoc);
+  SourceRange ConstructR = Cons->getSourceRange();

What you want is `getTokenRange(Call->getCallee()->getSourceRange())`
No need to mess with the locations and offsets yourself.


Comment at: clang-tidy/performance/EmplaceCheck.h:18
@@ +17,3 @@
+namespace performance {
+
+class EmplaceCheck : public ClangTidyCheck {

Needs a comment


Comment at: clang-tidy/performance/EmplaceCheck.h:19
@@ +18,3 @@
+
+class EmplaceCheck : public ClangTidyCheck {
+public:

What's the scope?
Is it just vector::push_back/emplace_back?
Or is it going to be expanded to other map::insert, deque::push_front, etc.


Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:26
@@ +25,3 @@
+  std::vector v1;
+
+  v1.push_back(X());

We have to make sure the arguments are compatible with perfect forwarding.
This means no brace init lists and no NULL.
Eg:

v.push_back(Y({}));  // {} can't be passed to perfect forwarding
v.push_back(Y(NULL));  // NULL can't be passed to perfect forwarding


Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:28
@@ +27,3 @@
+  v1.push_back(X());
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: Consider constructing {{.*}}
+  // CHECK-FIXES: v1.emplace_back()

At least one of the cases should match the whole exact message, to make sure it 
is correct.


Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:42
@@ +41,3 @@
+  // CHECK-FIXES: v1/*a*/./*b*/emplace_back(/*c*//*d*/0,/*e*/0/*f*//*g*/)/*h*/ 
;
+
+  // Do not try to deal with initializer lists.

We should also test implicit conversions.
Eg:

v1.push_back(0);


Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:46
@@ +45,3 @@
+
+  // Do not try to deal with functional casts. FIXME?
+  X x{0, 0};

Why not?


Comment at: test/clang-tidy/performance-emplace-into-containers.cpp:50
@@ +49,3 @@
+  v1.push_back(X(0));
+
+  // Do not try to deal with weird expansions.

We need tests for copy/move construction.
Eg:

v1.push_back(x);  // should not be changed
v1.push_back(FunctionThatReturnsX());  // should not be changed


http://reviews.llvm.org/D21185



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

Re: [PATCH] D21036: Misplaced const-qualification with typedef types

[Samuel Benzaquen via cfe-commits]

sbenza accepted this revision.
This revision is now accepted and ready to land.


Comment at: clang-tidy/misc/MisplacedConstCheck.cpp:22
@@ +21,3 @@
+  Finder->addMatcher(
+  valueDecl(allOf(hasType(isConstQualified()),
+  hasType(typedefType(hasDeclaration(

aaron.ballman wrote:
> sbenza wrote:
> > allOf() is unnecessary
> Good catch, that was a holdover from when I was using clang-query.
You shouldn't need it for clang-query either.


http://reviews.llvm.org/D21036



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

Re: [PATCH] D21036: Misplaced const-qualification with typedef types

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/MisplacedConstCheck.cpp:22
@@ +21,3 @@
+  Finder->addMatcher(
+  valueDecl(allOf(hasType(isConstQualified()),
+  hasType(typedefType(hasDeclaration(

allOf() is unnecessary


Comment at: clang-tidy/misc/MisplacedConstCheck.cpp:26
@@ +25,3 @@
+  isConstQualified(),
+  ignoringParens(functionType(
+  .bind("typedef"))

Should we ignore pointer to members?
If not, we should add a test for those.


http://reviews.llvm.org/D21036



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

Re: [PATCH] D21036: Misplaced const-qualification with typedef types

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

I think this would be more interesting with macros.
Eg triggering in code like this:

  #define FOO(type, op) const type& X = op()
  FOO(int*, bar);



Comment at: clang-tidy/misc/MisplacedConstCheck.cpp:32
@@ +31,3 @@
+
+static QualType GuessAlternateQualification(ASTContext , QualType QT) {
+  // We're given a QualType from a typedef where the qualifiers apply to the

guessAlternateQualification (lower case)


Comment at: test/clang-tidy/misc-misplaced-const.c:20
@@ +19,3 @@
+  const volatile ip i4 = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'i4' declared with a 
const-qualified typedef type; results in the type being 'int *const volatile' 
instead of 'const int *volatile'
+}

If we are guessing that 'const' goes to the int, shouldn't we guess that 
volatile also goes to the int?


Comment at: test/clang-tidy/misc-misplaced-const.c:20
@@ +19,3 @@
+  const volatile ip i4 = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'i4' declared with a 
const-qualified typedef type; results in the type being 'int *const volatile' 
instead of 'const int *volatile'
+}

sbenza wrote:
> If we are guessing that 'const' goes to the int, shouldn't we guess that 
> volatile also goes to the int?
Only keep the first instance with the full message.
The rest can be collapsed with {{.*}} to try to fit in 80 cols.


Comment at: test/clang-tidy/misc-misplaced-const.cpp:20
@@ +19,3 @@
+
+template 
+struct S {

I assume this check does not trigger for 'const X&' where X is a pointer.
Please add a test for that.


http://reviews.llvm.org/D21036



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

[clang-tools-extra] r271896 - [clang-tidy] Do not try to suggest a fix if the parameter is partially in a macro.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Mon Jun  6 09:21:11 2016
New Revision: 271896

URL: http://llvm.org/viewvc/llvm-project?rev=271896=rev
Log:
[clang-tidy] Do not try to suggest a fix if the parameter is partially in a 
macro.

It is not easy to tell where to do the suggestion and whether the
suggestion will be correct.

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

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp?rev=271896=271895=271896=diff
==
--- clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp 
Mon Jun  6 09:21:11 2016
@@ -88,6 +88,12 @@ void AvoidConstParamsInDecls::check(cons
 Diag << Param;
   }
 
+  if (Param->getLocStart().isMacroID() != Param->getLocEnd().isMacroID()) {
+// Do not offer a suggestion if the part of the variable declaration comes
+// from a macro.
+return;
+  }
+
   CharSourceRange FileRange = Lexer::makeFileCharRange(
   CharSourceRange::getTokenRange(getTypeRange(*Param)),
   *Result.SourceManager, Result.Context->getLangOpts());


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

Re: [PATCH] D20917: [clang-tidy] Add RemoveStars option to the modernize-use-auto check

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Is it a typo in the description when it says that when RemoveStar is on we will 
write 'auto*' instead if 'auto'?


http://reviews.llvm.org/D20917



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

[clang-tools-extra] r271426 - Fix uninitialized memory access when the token 'const' is not present in

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Wed Jun  1 15:37:23 2016
New Revision: 271426

URL: http://llvm.org/viewvc/llvm-project?rev=271426=rev
Log:
Fix uninitialized memory access when the token 'const' is not present in
the program.

If the token is not there, we still warn but we don't try to give a
fixit hint.

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

clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp?rev=271426=271425=271426=diff
==
--- clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/readability/AvoidConstParamsInDecls.cpp 
Wed Jun  1 15:37:23 2016
@@ -8,6 +8,7 @@
 
//===--===//
 
 #include "AvoidConstParamsInDecls.h"
+#include "llvm/ADT/Optional.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Lex/Lexer.h"
@@ -38,8 +39,8 @@ void AvoidConstParamsInDecls::registerMa
 }
 
 // Re-lex the tokens to get precise location of last 'const'
-static Token ConstTok(CharSourceRange Range,
-  const MatchFinder::MatchResult ) {
+static llvm::Optional ConstTok(CharSourceRange Range,
+  const MatchFinder::MatchResult ) {
   const SourceManager  = *Result.SourceManager;
   std::pair LocInfo =
   Sources.getDecomposedLoc(Range.getBegin());
@@ -49,7 +50,7 @@ static Token ConstTok(CharSourceRange Ra
  Result.Context->getLangOpts(), File.begin(), TokenBegin,
  File.end());
   Token Tok;
-  Token ConstTok;
+  llvm::Optional ConstTok;
   while (!RawLexer.LexFromRawLexer(Tok)) {
 if (Sources.isBeforeInTranslationUnit(Range.getEnd(), Tok.getLocation()))
   break;
@@ -94,9 +95,11 @@ void AvoidConstParamsInDecls::check(cons
   if (!FileRange.isValid())
 return;
 
-  Token Tok = ConstTok(FileRange, Result);
+  auto Tok = ConstTok(FileRange, Result);
+  if (!Tok)
+return;
   Diag << FixItHint::CreateRemoval(
-  CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getLocation()));
+  CharSourceRange::getTokenRange(Tok->getLocation(), Tok->getLocation()));
 }
 
 } // namespace readability

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp?rev=271426=271425=271426=diff
==
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp
 (original)
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-avoid-const-params-in-decls.cpp
 Wed Jun  1 15:37:23 2016
@@ -83,3 +83,10 @@ void NF(const int&);
 void NF(const int*);
 void NF(const int* const*);
 void NF(alias_const_type);
+
+// Regression test for when the 'const' token is not in the code.
+#define CONCAT(a, b) a##b
+void ConstNotVisible(CONCAT(cons, t) int i);
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: parameter 'i'
+// We warn, but we can't give a fix
+// CHECK-FIXES: void ConstNotVisible(CONCAT(cons, t) int i);


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

Re: [PATCH] D20597: Speed up check by using a recursive visitor.

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL270714: Speed up check by using a recursive visitor. 
(authored by sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D20597?vs=58438=58440#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20597

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

Index: clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp
@@ -8,14 +8,66 @@
 //===--===//
 
 #include "FunctionSizeCheck.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+class FunctionASTVisitor : public RecursiveASTVisitor {
+  using Base = RecursiveASTVisitor;
+
+public:
+  bool TraverseStmt(Stmt *Node) {
+if (!Node)
+  return Base::TraverseStmt(Node);
+
+if (TrackedParent.back() && !isa(Node))
+  ++Info.Statements;
+
+switch (Node->getStmtClass()) {
+case Stmt::IfStmtClass:
+case Stmt::WhileStmtClass:
+case Stmt::DoStmtClass:
+case Stmt::CXXForRangeStmtClass:
+case Stmt::ForStmtClass:
+case Stmt::SwitchStmtClass:
+  ++Info.Branches;
+// fallthrough
+case Stmt::CompoundStmtClass:
+  TrackedParent.push_back(true);
+  break;
+default:
+  TrackedParent.push_back(false);
+  break;
+}
+
+Base::TraverseStmt(Node);
+
+TrackedParent.pop_back();
+return true;
+  }
+
+  bool TraverseDecl(Decl *Node) {
+TrackedParent.push_back(false);
+Base::TraverseDecl(Node);
+TrackedParent.pop_back();
+return true;
+  }
+
+  struct FunctionInfo {
+FunctionInfo() : Lines(0), Statements(0), Branches(0) {}
+unsigned Lines;
+unsigned Statements;
+unsigned Branches;
+  };
+  FunctionInfo Info;
+  std::vector TrackedParent;
+};
+
 FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   LineThreshold(Options.get("LineThreshold", -1U)),
@@ -29,76 +81,52 @@
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(
-  functionDecl(
-  unless(isInstantiated()),
-  forEachDescendant(
-  stmt(unless(compoundStmt()),
-   hasParent(stmt(anyOf(compoundStmt(), ifStmt(),
-anyOf(whileStmt(), doStmt(),
-  cxxForRangeStmt(), forStmt())
-  .bind("stmt"))).bind("func"),
-  this);
+  Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), this);
 }
 
 void FunctionSizeCheck::check(const MatchFinder::MatchResult ) {
   const auto *Func = Result.Nodes.getNodeAs("func");
 
-  FunctionInfo  = FunctionInfos[Func];
+  FunctionASTVisitor Visitor;
+  Visitor.TraverseDecl(const_cast(Func));
+  auto  = Visitor.Info;
+
+  if (FI.Statements == 0)
+return;
 
   // Count the lines including whitespace and comments. Really simple.
-  if (!FI.Lines) {
-if (const Stmt *Body = Func->getBody()) {
-  SourceManager *SM = Result.SourceManager;
-  if (SM->isWrittenInSameFile(Body->getLocStart(), Body->getLocEnd())) {
-FI.Lines = SM->getSpellingLineNumber(Body->getLocEnd()) -
-   SM->getSpellingLineNumber(Body->getLocStart());
-  }
+  if (const Stmt *Body = Func->getBody()) {
+SourceManager *SM = Result.SourceManager;
+if (SM->isWrittenInSameFile(Body->getLocStart(), Body->getLocEnd())) {
+  FI.Lines = SM->getSpellingLineNumber(Body->getLocEnd()) -
+ SM->getSpellingLineNumber(Body->getLocStart());
 }
   }
 
-  const auto *Statement = Result.Nodes.getNodeAs("stmt");
-  ++FI.Statements;
-
-  // TODO: switch cases, gotos
-  if (isa(Statement) || isa(Statement) ||
-  isa(Statement) || isa(Statement) ||
-  isa(Statement) || isa(Statement))
-++FI.Branches;
-}
-
-void FunctionSizeCheck::onEndOfTranslationUnit() {
-  // If we're above the limit emit a warning.
-  for (const auto  : FunctionInfos) {
-const FunctionInfo  = P.second;
-if (FI.Lines > LineThreshold || FI.Statements > StatementThreshold ||
-FI.Branches > BranchThreshold) {
-  diag(P.first->getLocation(),
-   "function %0 exceeds recommended size/complexity thresholds")
-  << P.first;
-}
-
-if (FI.Lines > LineThreshold) {
-  diag(P.first->getLocation(),
-   "%0 lines including whitespace and comments (threshold %1)",
-   DiagnosticIDs::Note)
-  << FI.Lines << LineThreshold;
-}
+  if 

[clang-tools-extra] r270714 - Speed up check by using a recursive visitor.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Wed May 25 11:19:23 2016
New Revision: 270714

URL: http://llvm.org/viewvc/llvm-project?rev=270714=rev
Log:
Speed up check by using a recursive visitor.

Summary:
Use a recursive visitor instead of forEachDescendant() matcher.
The latter requires several layers of virtual function calls for each node and
it is more expensive than the visitor.
Benchmark results show improvement of ~6% walltime in clang-tidy.

Reviewers: alexfh

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D20597

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

Modified: clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp?rev=270714=270713=270714=diff
==
--- clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/readability/FunctionSizeCheck.cpp Wed 
May 25 11:19:23 2016
@@ -8,6 +8,7 @@
 
//===--===//
 
 #include "FunctionSizeCheck.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
@@ -16,6 +17,57 @@ namespace clang {
 namespace tidy {
 namespace readability {
 
+class FunctionASTVisitor : public RecursiveASTVisitor {
+  using Base = RecursiveASTVisitor;
+
+public:
+  bool TraverseStmt(Stmt *Node) {
+if (!Node)
+  return Base::TraverseStmt(Node);
+
+if (TrackedParent.back() && !isa(Node))
+  ++Info.Statements;
+
+switch (Node->getStmtClass()) {
+case Stmt::IfStmtClass:
+case Stmt::WhileStmtClass:
+case Stmt::DoStmtClass:
+case Stmt::CXXForRangeStmtClass:
+case Stmt::ForStmtClass:
+case Stmt::SwitchStmtClass:
+  ++Info.Branches;
+// fallthrough
+case Stmt::CompoundStmtClass:
+  TrackedParent.push_back(true);
+  break;
+default:
+  TrackedParent.push_back(false);
+  break;
+}
+
+Base::TraverseStmt(Node);
+
+TrackedParent.pop_back();
+return true;
+  }
+
+  bool TraverseDecl(Decl *Node) {
+TrackedParent.push_back(false);
+Base::TraverseDecl(Node);
+TrackedParent.pop_back();
+return true;
+  }
+
+  struct FunctionInfo {
+FunctionInfo() : Lines(0), Statements(0), Branches(0) {}
+unsigned Lines;
+unsigned Statements;
+unsigned Branches;
+  };
+  FunctionInfo Info;
+  std::vector TrackedParent;
+};
+
 FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   LineThreshold(Options.get("LineThreshold", -1U)),
@@ -29,76 +81,52 @@ void FunctionSizeCheck::storeOptions(Cla
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(
-  functionDecl(
-  unless(isInstantiated()),
-  forEachDescendant(
-  stmt(unless(compoundStmt()),
-   hasParent(stmt(anyOf(compoundStmt(), ifStmt(),
-anyOf(whileStmt(), doStmt(),
-  cxxForRangeStmt(), forStmt())
-  .bind("stmt"))).bind("func"),
-  this);
+  Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), 
this);
 }
 
 void FunctionSizeCheck::check(const MatchFinder::MatchResult ) {
   const auto *Func = Result.Nodes.getNodeAs("func");
 
-  FunctionInfo  = FunctionInfos[Func];
+  FunctionASTVisitor Visitor;
+  Visitor.TraverseDecl(const_cast(Func));
+  auto  = Visitor.Info;
+
+  if (FI.Statements == 0)
+return;
 
   // Count the lines including whitespace and comments. Really simple.
-  if (!FI.Lines) {
-if (const Stmt *Body = Func->getBody()) {
-  SourceManager *SM = Result.SourceManager;
-  if (SM->isWrittenInSameFile(Body->getLocStart(), Body->getLocEnd())) {
-FI.Lines = SM->getSpellingLineNumber(Body->getLocEnd()) -
-   SM->getSpellingLineNumber(Body->getLocStart());
-  }
+  if (const Stmt *Body = Func->getBody()) {
+SourceManager *SM = Result.SourceManager;
+if (SM->isWrittenInSameFile(Body->getLocStart(), Body->getLocEnd())) {
+  FI.Lines = SM->getSpellingLineNumber(Body->getLocEnd()) -
+ SM->getSpellingLineNumber(Body->getLocStart());
 }
   }
 
-  const auto *Statement = Result.Nodes.getNodeAs("stmt");
-  ++FI.Statements;
-
-  // TODO: switch cases, gotos
-  if (isa(Statement) || isa(Statement) ||
-  isa(Statement) || isa(Statement) ||
-  isa(Statement) || isa(Statement))
-++FI.Branches;
-}
-
-void FunctionSizeCheck::onEndOfTranslationUnit() {
-  // If we're above the limit emit a warning.
-  for (const auto  : FunctionInfos) {
-const FunctionInfo  = P.second;
-

Re: [PATCH] D20597: Speed up check by using a recursive visitor.

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 58438.
sbenza marked an inline comment as done.
sbenza added a comment.

Reformat code


http://reviews.llvm.org/D20597

Files:
  clang-tidy/readability/FunctionSizeCheck.cpp
  clang-tidy/readability/FunctionSizeCheck.h

Index: clang-tidy/readability/FunctionSizeCheck.h
===
--- clang-tidy/readability/FunctionSizeCheck.h
+++ clang-tidy/readability/FunctionSizeCheck.h
@@ -34,21 +34,11 @@
   void storeOptions(ClangTidyOptions::OptionMap ) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
-  void onEndOfTranslationUnit() override;
 
 private:
-  struct FunctionInfo {
-FunctionInfo() : Lines(0), Statements(0), Branches(0) {}
-unsigned Lines;
-unsigned Statements;
-unsigned Branches;
-  };
-
   const unsigned LineThreshold;
   const unsigned StatementThreshold;
   const unsigned BranchThreshold;
-
-  llvm::DenseMap FunctionInfos;
 };
 
 } // namespace readability
Index: clang-tidy/readability/FunctionSizeCheck.cpp
===
--- clang-tidy/readability/FunctionSizeCheck.cpp
+++ clang-tidy/readability/FunctionSizeCheck.cpp
@@ -8,14 +8,66 @@
 //===--===//
 
 #include "FunctionSizeCheck.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
+class FunctionASTVisitor : public RecursiveASTVisitor {
+  using Base = RecursiveASTVisitor;
+
+public:
+  bool TraverseStmt(Stmt *Node) {
+if (!Node)
+  return Base::TraverseStmt(Node);
+
+if (TrackedParent.back() && !isa(Node))
+  ++Info.Statements;
+
+switch (Node->getStmtClass()) {
+case Stmt::IfStmtClass:
+case Stmt::WhileStmtClass:
+case Stmt::DoStmtClass:
+case Stmt::CXXForRangeStmtClass:
+case Stmt::ForStmtClass:
+case Stmt::SwitchStmtClass:
+  ++Info.Branches;
+// fallthrough
+case Stmt::CompoundStmtClass:
+  TrackedParent.push_back(true);
+  break;
+default:
+  TrackedParent.push_back(false);
+  break;
+}
+
+Base::TraverseStmt(Node);
+
+TrackedParent.pop_back();
+return true;
+  }
+
+  bool TraverseDecl(Decl *Node) {
+TrackedParent.push_back(false);
+Base::TraverseDecl(Node);
+TrackedParent.pop_back();
+return true;
+  }
+
+  struct FunctionInfo {
+FunctionInfo() : Lines(0), Statements(0), Branches(0) {}
+unsigned Lines;
+unsigned Statements;
+unsigned Branches;
+  };
+  FunctionInfo Info;
+  std::vector TrackedParent;
+};
+
 FunctionSizeCheck::FunctionSizeCheck(StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   LineThreshold(Options.get("LineThreshold", -1U)),
@@ -29,76 +81,52 @@
 }
 
 void FunctionSizeCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(
-  functionDecl(
-  unless(isInstantiated()),
-  forEachDescendant(
-  stmt(unless(compoundStmt()),
-   hasParent(stmt(anyOf(compoundStmt(), ifStmt(),
-anyOf(whileStmt(), doStmt(),
-  cxxForRangeStmt(), forStmt())
-  .bind("stmt"))).bind("func"),
-  this);
+  Finder->addMatcher(functionDecl(unless(isInstantiated())).bind("func"), this);
 }
 
 void FunctionSizeCheck::check(const MatchFinder::MatchResult ) {
   const auto *Func = Result.Nodes.getNodeAs("func");
 
-  FunctionInfo  = FunctionInfos[Func];
+  FunctionASTVisitor Visitor;
+  Visitor.TraverseDecl(const_cast(Func));
+  auto  = Visitor.Info;
+
+  if (FI.Statements == 0)
+return;
 
   // Count the lines including whitespace and comments. Really simple.
-  if (!FI.Lines) {
-if (const Stmt *Body = Func->getBody()) {
-  SourceManager *SM = Result.SourceManager;
-  if (SM->isWrittenInSameFile(Body->getLocStart(), Body->getLocEnd())) {
-FI.Lines = SM->getSpellingLineNumber(Body->getLocEnd()) -
-   SM->getSpellingLineNumber(Body->getLocStart());
-  }
+  if (const Stmt *Body = Func->getBody()) {
+SourceManager *SM = Result.SourceManager;
+if (SM->isWrittenInSameFile(Body->getLocStart(), Body->getLocEnd())) {
+  FI.Lines = SM->getSpellingLineNumber(Body->getLocEnd()) -
+ SM->getSpellingLineNumber(Body->getLocStart());
 }
   }
 
-  const auto *Statement = Result.Nodes.getNodeAs("stmt");
-  ++FI.Statements;
-
-  // TODO: switch cases, gotos
-  if (isa(Statement) || isa(Statement) ||
-  isa(Statement) || isa(Statement) ||
-  isa(Statement) || isa(Statement))
-++FI.Branches;
-}
-
-void FunctionSizeCheck::onEndOfTranslationUnit() {
-  // If we're above the limit emit a warning.
-  

Re: [PATCH] D19324: [ASTMatchers] new forEachOverriden matcher

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: include/clang/AST/ASTContext.h:824
@@ -823,1 +823,3 @@
   unsigned overridden_methods_size(const CXXMethodDecl *Method) const;
+  typedef llvm::iterator_range
+  overridden_method_range;

Sure. Sorry about that.
The main idea was to use a range-like API instead of returning a nullable 
pointer to a range.


http://reviews.llvm.org/D19324



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

Re: [PATCH] D20277: [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved.

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:83
@@ +82,3 @@
+  if (!IsConstQualified) {
+auto Matches = utils::decl_ref_expr::allDeclRefExprs(
+*Param, *Function->getBody(), *Result.Context);

We should not call this more than once as it is a potentially expensive 
operation.
That is, isOnlyUsedAsConst will do it too.
Also, in this case we don't care about _all_. We only care about != 1.


Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:144
@@ +143,3 @@
+  Diag << FixItHint::CreateReplacement(
+  CopyArgument.getSourceRange(),
+  (Twine("std::move(") + Var.getName() + ")").str());

Prefer two additions over a replacement.
They usually behave better in some conditions.


Comment at: clang-tidy/utils/DeclRefExprUtils.cpp:102
@@ +101,3 @@
+  auto Matches =
+  match(findAll(cxxConstructExpr(
+UsedAsConstRefArg,

why findAll() ?
You only care about one.

same as below.


Comment at: clang-tidy/utils/TypeTraits.cpp:131
@@ +130,3 @@
+  for (const auto *Constructor : Record->ctors()) {
+if (Constructor->isMoveConstructor() && !Constructor->isDeleted())
+  return true;

maybe also check that it is not trivial?

below too.


Repository:
  rL LLVM

http://reviews.llvm.org/D20277



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

Re: [PATCH] D19871: Add an AST matcher for CastExpr kind

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:78
@@ -77,2 +77,3 @@
   // Need Variant/Parser fixes:
+  // hasCastKind  
   // ofKind

Is it registered or not?
You add this comment, but also add the matcher in the registry below.


http://reviews.llvm.org/D19871



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

Re: [PATCH] D19871: Add an AST matcher for CastExpr kind

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: lib/ASTMatchers/Dynamic/Marshallers.h:102
@@ +101,3 @@
+  static clang::CastKind getCastKind(llvm::StringRef AttrKind) {
+return llvm::StringSwitch(AttrKind)
+  .Case("CK_Dependent", CK_Dependent)

etienneb wrote:
> aaron.ballman wrote:
> > This might be an awful idea, but let's explore it.
> > 
> > What if we moved the CastKind enumerator names into a .def (or .inc) file 
> > and use macros to generate the enumeration as well as this monster switch 
> > statement? We do this in other places where it makes sense to do so, such 
> > as in Expr.h:
> > ```
> >   enum AtomicOp {
> > #define BUILTIN(ID, TYPE, ATTRS)
> > #define ATOMIC_BUILTIN(ID, TYPE, ATTRS) AO ## ID,
> > #include "clang/Basic/Builtins.def"
> > // Avoid trailing comma
> > BI_First = 0
> >   };
> > ```
> Does the dynamic matching is used somewhere else than clang-query?
> I wonder the impact of refactoring to support them if it's barely used.
> It can't be worse than before as it wasn't supported at all (the matcher 
> didn't exists).
> 
> I believe there is a larger cleanup to do to support correctly dynamic 
> matcher like "equals".
> And, this case is one among others.
> 
> I'm not a fan of this huge switch that may just get out-of-sync with the 
> original enum.
> 
> I'm still in favor of adding this matcher to the unsupported list until we 
> push the more complicated fix.
> [which may fall in my plate anyway]
> 
> Any toughs?
I'm not a fan of this either.
If the enum has to be used in this way, it should be refactored to be generated 
with an .inc/.def file.


http://reviews.llvm.org/D19871



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

r268548 - Fix the doc extraction script to work with hasAnyName and with equalsNode.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Wed May  4 15:45:00 2016
New Revision: 268548

URL: http://llvm.org/viewvc/llvm-project?rev=268548=rev
Log:
Fix the doc extraction script to work with hasAnyName and with equalsNode.

The change from llvm::VariadicFunction to internal::VariadicFunction
broke the extraction of hasAnyName().
equalsNode was broken because the argument type is 'const *' and
the internal space caused a failure on the regex.

Modified:
cfe/trunk/docs/LibASTMatchersReference.html
cfe/trunk/docs/tools/dump_ast_matchers.py

Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=268548=268547=268548=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Wed May  4 15:45:00 2016
@@ -410,7 +410,7 @@ decl(hasDeclContext(translationUnitDecl(
 
 Given
   typedef int X;
-   using Y = int;
+  using Y = int;
 typeAliasDecl()
   matches "using Y = int", but not "typedef int X"
 
@@ -421,7 +421,7 @@ typeAliasDecl()
 
 Given
   typedef int X;
-   using Y = int;
+  using Y = int;
 typedefDecl()
   matches "typedef int X", but not "using Y = int"
 
@@ -432,7 +432,7 @@ typedefDecl()
 
 Given
   typedef int X;
-   using Y = int;
+  using Y = int;
 typedefNameDecl()
   matches "typedef int X" and "using Y = int"
 
@@ -2238,6 +2238,13 @@ and reference to that variable declarati
 
 
 
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Decl.html;>DeclequalsNodeconst Decl* Other
+Matches if a node equals 
another node.
+
+Decl has pointer identity in the AST.
+
+
+
 Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Decl.html;>DeclhasAttrattr::Kind AttrKind
 Matches declaration that 
has a given attribute.
 
@@ -2898,6 +2905,13 @@ and reference to that variable declarati
 
 
 
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtequalsNodeconst Stmt* Other
+Matches if a node equals 
another node.
+
+Stmt has pointer identity in the AST.
+
+
+
 Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtisExpansionInFileMatchingstd::string
 RegExp
 Matches 
AST nodes that were expanded within files whose name is
 partially matching a given regex.
@@ -3072,6 +3086,13 @@ and reference to that variable declarati
 
 
 
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Type.html;>TypeequalsNodeconst Type* Other
+Matches if a node equals 
another node.
+
+Type has pointer identity in the AST.
+
+
+
 Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Type.html;>TyperealFloatingPointType
 Matches any 
real floating-point type (float, double, long double).
 
@@ -3286,6 +3307,16 @@ expr(nullPointerConstant())
 
 
 
+Matcherinternal::Matcherhttp://clang.llvm.org/doxygen/classclang_1_1NamedDecl.html;>NamedDeclhasAnyNameStringRef, ..., 
StringRef
+Matches NamedDecl nodes 
that have any of the specified names.
+
+This matcher is only provided as a performance optimization of hasName.
+hasAnyName(a, b, c)
+ is equivalent to, but faster than
+anyOf(hasName(a), hasName(b), hasName(c))
+
+
+
 Matcherinternal::Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtisInTemplateInstantiation
 Matches 
statements inside of a template instantiation.
 

Modified: cfe/trunk/docs/tools/dump_ast_matchers.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/tools/dump_ast_matchers.py?rev=268548=268547=268548=diff
==
--- cfe/trunk/docs/tools/dump_ast_matchers.py (original)
+++ cfe/trunk/docs/tools/dump_ast_matchers.py Wed May  4 15:45:00 2016
@@ -95,7 +95,7 @@ def strip_doxygen(comment):
 def unify_arguments(args):
   """Gets rid of anything the user doesn't care about in the argument list."""
   args = re.sub(r'internal::', r'', args)
-  args = re.sub(r'const\s+', r'', args)
+  args = re.sub(r'const\s+(.*)&', r'\1 ', args)
   args = re.sub(r'&', r' ', args)
   args = re.sub(r'(^|\s)M\d?(\s)', r'\1Matcher<*>\2', args)
   return args
@@ -231,7 +231,7 @@ def act_on_decl(declaration, comment, al
 m = re.match(r"""^\s*AST_MATCHER(_P)?(.?)(?:_OVERLOAD)?\(
(?:\s*([^\s,]+)\s*,)?
   \s*([^\s,]+)\s*
-   (?:,\s*([^\s,]+)\s*
+   (?:,\s*([^,]+)\s*
   ,\s*([^\s,]+)\s*)?
(?:,\s*([^\s,]+)\s*
   ,\s*([^\s,]+)\s*)?
@@ -266,7 +266,7 @@ def act_on_decl(declaration, comment, al
 
 # Parse Variadic functions.
 m = re.match(
-r"""^.*llvm::VariadicFunction\s*<\s*([^,]+),\s*([^,]+),\s*[^>]+>\s*
+r"""^.*internal::VariadicFunction\s*<\s*([^,]+),\s*([^,]+),\s*[^>]+>\s*
   ([a-zA-Z]*)\s*=\s*{.*};$""",
 declaration, flags=re.X)
 if m:


___
cfe-commits mailing list

Re: [PATCH] D19877: [clang-tidy] Speedup misc-static-assert.

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL268430: [clang-tidy] Speedup misc-static-assert. (authored 
by sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D19877?vs=56028=56053#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D19877

Files:
  clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp

Index: clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp
@@ -63,11 +63,15 @@
 hasArgument(0, AssertCondition))),
 AssertCondition);
 
-  Finder->addMatcher(stmt(anyOf(conditionalOperator(hasCondition(Condition)),
-ifStmt(hasCondition(Condition))),
-  unless(isInTemplateInstantiation()))
+  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
+ unless(isInTemplateInstantiation()))
  .bind("condStmt"),
  this);
+
+  Finder->addMatcher(
+  ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
+  .bind("condStmt"),
+  this);
 }
 
 void StaticAssertCheck::check(const MatchFinder::MatchResult ) {


Index: clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp
@@ -63,11 +63,15 @@
 hasArgument(0, AssertCondition))),
 AssertCondition);
 
-  Finder->addMatcher(stmt(anyOf(conditionalOperator(hasCondition(Condition)),
-ifStmt(hasCondition(Condition))),
-  unless(isInTemplateInstantiation()))
+  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
+ unless(isInTemplateInstantiation()))
  .bind("condStmt"),
  this);
+
+  Finder->addMatcher(
+  ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
+  .bind("condStmt"),
+  this);
 }
 
 void StaticAssertCheck::check(const MatchFinder::MatchResult ) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r268430 - [clang-tidy] Speedup misc-static-assert.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Tue May  3 15:11:09 2016
New Revision: 268430

URL: http://llvm.org/viewvc/llvm-project?rev=268430=rev
Log:
[clang-tidy] Speedup misc-static-assert.

Summary:
Speedup the misc-static-assert check by not use `stmt()` as the toplevel 
matcher.
The framework runs a filter on the matchers before trying them on each node and
uses the toplevel type for this.
Using `stmt()` as the toplevel causes the matcher to be run on every `Stmt` 
node,
even if the node doesn't match the desired types.

This change speeds up clang-tidy by ~5% in a benchmark.

Reviewers: alexfh

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D19877

Modified:
clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp

Modified: clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp?rev=268430=268429=268430=diff
==
--- clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/StaticAssertCheck.cpp Tue May  3 
15:11:09 2016
@@ -63,11 +63,15 @@ void StaticAssertCheck::registerMatchers
 hasArgument(0, AssertCondition))),
 AssertCondition);
 
-  Finder->addMatcher(stmt(anyOf(conditionalOperator(hasCondition(Condition)),
-ifStmt(hasCondition(Condition))),
-  unless(isInTemplateInstantiation()))
+  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
+ unless(isInTemplateInstantiation()))
  .bind("condStmt"),
  this);
+
+  Finder->addMatcher(
+  ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
+  .bind("condStmt"),
+  this);
 }
 
 void StaticAssertCheck::check(const MatchFinder::MatchResult ) {


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

[PATCH] D19877: [clang-tidy] Speedup misc-static-assert.

[Samuel Benzaquen via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.

Speedup the misc-static-assert check by not use `stmt()` as the toplevel 
matcher.
The framework runs a filter on the matchers before trying them on each node and
uses the toplevel type for this.
Using `stmt()` as the toplevel causes the matcher to be run on every `Stmt` 
node,
even if the node doesn't match the desired types.

This change speeds up clang-tidy by ~5% in a benchmark.

http://reviews.llvm.org/D19877

Files:
  clang-tidy/misc/StaticAssertCheck.cpp

Index: clang-tidy/misc/StaticAssertCheck.cpp
===
--- clang-tidy/misc/StaticAssertCheck.cpp
+++ clang-tidy/misc/StaticAssertCheck.cpp
@@ -62,11 +62,15 @@
 hasArgument(0, AssertCondition))),
 AssertCondition);
 
-  Finder->addMatcher(stmt(anyOf(conditionalOperator(hasCondition(Condition)),
-ifStmt(hasCondition(Condition))),
-  unless(isInTemplateInstantiation()))
+  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
+ unless(isInTemplateInstantiation()))
  .bind("condStmt"),
  this);
+
+  Finder->addMatcher(
+  ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
+  .bind("condStmt"),
+  this);
 }
 
 void StaticAssertCheck::check(const MatchFinder::MatchResult ) {


Index: clang-tidy/misc/StaticAssertCheck.cpp
===
--- clang-tidy/misc/StaticAssertCheck.cpp
+++ clang-tidy/misc/StaticAssertCheck.cpp
@@ -62,11 +62,15 @@
 hasArgument(0, AssertCondition))),
 AssertCondition);
 
-  Finder->addMatcher(stmt(anyOf(conditionalOperator(hasCondition(Condition)),
-ifStmt(hasCondition(Condition))),
-  unless(isInTemplateInstantiation()))
+  Finder->addMatcher(conditionalOperator(hasCondition(Condition),
+ unless(isInTemplateInstantiation()))
  .bind("condStmt"),
  this);
+
+  Finder->addMatcher(
+  ifStmt(hasCondition(Condition), unless(isInTemplateInstantiation()))
+  .bind("condStmt"),
+  this);
 }
 
 void StaticAssertCheck::check(const MatchFinder::MatchResult ) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19871: Add an AST matcher for CastExpr kind

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.



> > So even if the above solution is working, we still need to call it that way 
> > (as a string):

> 

> >  clang-query> match castExpr(hasCastKind("CK_Dependent"))

> 

> 

> Correct, that's expected behavior for clang-query (though I would love if 
> someday we could expose actual enumerations somehow instead of string 
> literals).


It is not hard to do, but it would require changing the parser, the registry, 
the type-erased value wrapper, etc.
The late conversion from "string" to enum was the easiest solution at the time.


http://reviews.llvm.org/D19871



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

Re: [PATCH] D19871: Add an AST matcher for CastExpr kind

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

In http://reviews.llvm.org/D19871#419985, @aaron.ballman wrote:

> In http://reviews.llvm.org/D19871#419954, @etienneb wrote:
>
> > In http://reviews.llvm.org/D19871#419947, @aaron.ballman wrote:
> >
> > > Is this required for some purpose?
> >
> >
> > It's used in clang-tidy checkers.
> >
> >   see http://reviews.llvm.org/D19841
>
>
> It's good to have that context in a review for functionality that isn't part 
> of the proposed patch. :-) Looking at the other patch, I would prefer to keep 
> this matcher narrowed to just clang-tidy unless you can also solve how to 
> expose it via the dynamic registry so that it can be used by tools like 
> clang-query.


What we have done in the past with enum-arg matchers is to use string->enum 
conversion in the dynamic bindings.
See the specialization `ArgTypeTraits` in `Marshallers.h`.
We could add one for `CastKind` too.


http://reviews.llvm.org/D19871



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

Re: [PATCH] D18265: [clang-tidy] New: checker misc-unconventional-assign-operator replacing misc-assign-operator-signature

[Samuel Benzaquen via cfe-commits]

sbenza accepted this revision.


Comment at: clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp:69
@@ +68,3 @@
+void UnconventionalAssignOperatorCheck::check(const MatchFinder::MatchResult 
) {
+  if (const auto *RetStmt = Result.Nodes.getNodeAs("returnStmt")) {
+diag(RetStmt->getLocStart(), "operator=() should always return '*this'");

baloghadamsoftware wrote:
> sbenza wrote:
> > couldn't this be folded into the Messages array below?
> There would be no benefit of it since we use the location of RetStmt in this 
> message instead of Method.
Right. So it can't really be merged. That's fine.


http://reviews.llvm.org/D18265



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

Re: [PATCH] D19324: [ASTMatchers] new forEachOverriden matcher

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: include/clang/AST/DeclCXX.h:1830
@@ -1829,2 +1829,3 @@
   unsigned size_overridden_methods() const;
+  const ArrayRef overridden_methods() const;
 

Return type should have have toplevel `const`.


Comment at: lib/AST/ASTContext.cpp:1262
@@ -1261,2 +1261,3 @@
 ASTContext::overridden_methods_begin(const CXXMethodDecl *Method) const {
   llvm::DenseMap::const_iterator Pos
+  = OverriddenMethods.find(Method->getCanonicalDecl());

I would invert the calls here.
That is, make overridden_methods_begin/_end call overridden_methods() instead, 
and have overridden_methods() be the one that does the lookup.
This way we have a single place where the lookup happens. It would also make 
overridden_methods() faster.


http://reviews.llvm.org/D19324



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

Re: [PATCH] D18265: [clang-tidy] New: checker misc-unconventional-assign-operator replacing misc-assign-operator-signature

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp:69
@@ +68,3 @@
+void UnconventionalAssignOperatorCheck::check(const MatchFinder::MatchResult 
) {
+  if (const auto *RetStmt = Result.Nodes.getNodeAs("returnStmt")) {
+diag(RetStmt->getLocStart(), "operator=() should always return '*this'");

couldn't this be folded into the Messages array below?


http://reviews.llvm.org/D18265



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

Re: [PATCH] D19357: [ASTMatchers] New matcher forFunction

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Thanks for doing this!

I think we want the version that iterates all parents.
Otherwise it will have problems with templates.
That is, you won't know which `FunctionDecl` you will get: the template or the 
instantiation. And you might need one or the other specifically.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:5124
@@ +5123,3 @@
+///   but does match 'return > 0'
+AST_MATCHER_P(Stmt, forFunction, internal::Matcher,
+  InnerMatcher) {

I think this matcher works for Decls too, but we can leave it like this until 
we need the polymorphism.


Comment at: include/clang/ASTMatchers/ASTMatchers.h:5127
@@ +5126,3 @@
+  const auto  = Finder->getASTContext().getParents(Node);
+  assert(!Parents.empty() && "Found node that is not in the parent map.");
+

You should not assert() this.
Parents can be empty if the statement is not inside a function. eg a global 
variable initializer.


Comment at: include/clang/ASTMatchers/ASTMatchers.h:5132
@@ +5131,3 @@
+  while(!Stack.empty()) {
+const auto  = Stack.back();
+Stack.pop_back();

If you are taking from the back, use a vector<> instead.
Or SmallVector<> to avoid allocation in most cases.


Comment at: include/clang/ASTMatchers/ASTMatchers.h:5134
@@ +5133,3 @@
+Stack.pop_back();
+if(const auto *DeclNode = CurNode.get()) {
+  if(const auto *FuncDeclNode = dyn_cast(DeclNode)) {

Just call `CurNode.get`. It'll do the dyn_cast for you.


Comment at: include/clang/ASTMatchers/ASTMatchers.h:5139
@@ +5138,3 @@
+}
+  }
+}

We should check for `LambdaExpr` here too, and try to match against 
`LambdaExpr::getCallOperator()`


Comment at: include/clang/ASTMatchers/ASTMatchers.h:5141
@@ +5140,3 @@
+}
+const auto *StmtNode = CurNode.get();
+if(StmtNode&&!isa(StmtNode)) {

You don't need to get a Stmt. getParents() works with DynTypedNodes.
There might be some non-stmt nodes in the middle, like variable declarations.


Comment at: include/clang/ASTMatchers/ASTMatchers.h:5142
@@ +5141,3 @@
+const auto *StmtNode = CurNode.get();
+if(StmtNode&&!isa(StmtNode)) {
+  for(const auto : Finder->getASTContext().getParents(*StmtNode))

We should also not traverse FunctionDecls.
Eg:

```
void F() {
  struct S {
void F2() {
}
  };
}
```

So, on each node: if is a FunctionDecl or a LambdaExpr run the matcher, 
otherwise traverse.


Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:5581
@@ +5580,3 @@
+"  typedef PosVec *iterator;"
+"  typedef const PosVec *const_iterator;"
+"  iterator begin();"

All of this is kind of unnecessary for the test.
The function could just be:

```
PosVec& operator=(const PosVec&) {
  auto x = [] { return 1; };
  return *this;
}
```


Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:5596
@@ +5595,3 @@
+ has(unaryOperator(hasOperatorName("*"));
+  EXPECT_FALSE(
+matches(

`EXPECT_TRUE(notMatches(...`


Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:5601
@@ +5600,3 @@
+ has(binaryOperator(hasOperatorName(">"));
+ llvm::errs()<<"d";
+}

Please add test cases for the changes suggested to the matcher.


http://reviews.llvm.org/D19357



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

Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

> > We can proceed with this change if you want, but it is not required 
> > anymore. I don't know whether we need the extra complexity of 
> > `TemplateArgumentLess`.

> 

> 

> If this patch is not going to help with performance, I'm happy to abandon it.


It might help in the cases where you are doing hasAncestor with 
TemplateArgument bound nodes, but that is a small corner case and I don't think 
we need to optimize for it.


http://reviews.llvm.org/D19144



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

Re: [PATCH] D19231: [ASTMatchers] Do not try to memoize nodes we can't compare.

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL266748: [ASTMatchers] Do not try to memoize nodes we can't 
compare. (authored by sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D19231?vs=54089=54207#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D19231

Files:
  cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

Index: cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
+++ cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -616,6 +616,10 @@
 ActiveASTContext->getTranslationUnitDecl())
   return false;
 
+// For AST-nodes that don't have an identity, we can't memoize.
+if (!Builder->isComparable())
+  return matchesAncestorOfRecursively(Node, Matcher, Builder, MatchMode);
+
 MatchKey Key;
 Key.MatcherID = Matcher.getID();
 Key.Node = Node;
@@ -630,22 +634,34 @@
 }
 
 MemoizedMatchResult Result;
-Result.ResultOfMatch = false;
 Result.Nodes = *Builder;
+Result.ResultOfMatch =
+matchesAncestorOfRecursively(Node, Matcher, , MatchMode);
+
+MemoizedMatchResult  = ResultCache[Key];
+CachedResult = std::move(Result);
+
+*Builder = CachedResult.Nodes;
+return CachedResult.ResultOfMatch;
+  }
 
+  bool matchesAncestorOfRecursively(const ast_type_traits::DynTypedNode ,
+const DynTypedMatcher ,
+BoundNodesTreeBuilder *Builder,
+AncestorMatchMode MatchMode) {
 const auto  = ActiveASTContext->getParents(Node);
 assert(!Parents.empty() && "Found node that is not in the parent map.");
 if (Parents.size() == 1) {
   // Only one parent - do recursive memoization.
   const ast_type_traits::DynTypedNode Parent = Parents[0];
-  if (Matcher.matches(Parent, this, )) {
-Result.ResultOfMatch = true;
-  } else if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
-// Reset the results to not include the bound nodes from the failed
-// match above.
-Result.Nodes = *Builder;
-Result.ResultOfMatch = memoizedMatchesAncestorOfRecursively(
-Parent, Matcher, , MatchMode);
+  BoundNodesTreeBuilder BuilderCopy = *Builder;
+  if (Matcher.matches(Parent, this, )) {
+*Builder = std::move(BuilderCopy);
+return true;
+  }
+  if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
+return memoizedMatchesAncestorOfRecursively(Parent, Matcher, Builder,
+MatchMode);
 // Once we get back from the recursive call, the result will be the
 // same as the parent's result.
   }
@@ -655,10 +671,10 @@
   std::deque Queue(Parents.begin(),
   Parents.end());
   while (!Queue.empty()) {
-Result.Nodes = *Builder;
-if (Matcher.matches(Queue.front(), this, )) {
-  Result.ResultOfMatch = true;
-  break;
+BoundNodesTreeBuilder BuilderCopy = *Builder;
+if (Matcher.matches(Queue.front(), this, )) {
+  *Builder = std::move(BuilderCopy);
+  return true;
 }
 if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
   for (const auto  :
@@ -673,12 +689,7 @@
 Queue.pop_front();
   }
 }
-
-MemoizedMatchResult  = ResultCache[Key];
-CachedResult = std::move(Result);
-
-*Builder = CachedResult.Nodes;
-return CachedResult.ResultOfMatch;
+return false;
   }
 
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with
Index: cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
===
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -708,6 +708,19 @@
   decl(anyOf(hasDescendant(RD), hasDescendant(VD);
 }
 
+TEST(DeclarationMatcher, HasAncestorMemoization) {
+  // This triggers an hasAncestor with a TemplateArgument in the bound nodes.
+  // That node can't be memoized so we have to check for it before trying to put
+  // it on the cache.
+  DeclarationMatcher CannotMemoize = classTemplateSpecializationDecl(
+  hasAnyTemplateArgument(templateArgument().bind("targ")),
+  forEach(fieldDecl(hasAncestor(forStmt();
+
+  EXPECT_TRUE(notMatches("template  struct S;"
+ "template <> struct S{ int i; int j; };",
+ CannotMemoize));
+}
+
 TEST(DeclarationMatcher, HasAttr) {
   EXPECT_TRUE(matches("struct __attribute__((warn_unused)) X {};",
   decl(hasAttr(clang::attr::WarnUnused;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

r266748 - [ASTMatchers] Do not try to memoize nodes we can't compare.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Tue Apr 19 10:52:56 2016
New Revision: 266748

URL: http://llvm.org/viewvc/llvm-project?rev=266748=rev
Log:
[ASTMatchers] Do not try to memoize nodes we can't compare.

Summary:
Prevent hasAncestor from comparing nodes that are not supported.
hasDescendant was fixed some time ago to avoid this problem.
I'm applying the same fix to hasAncestor: if any object in the Builder map is
not comparable, skip the cache.

Reviewers: alexfh

Subscribers: klimek, cfe-commits

Differential Revision: http://reviews.llvm.org/D19231

Modified:
cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

Modified: cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp?rev=266748=266747=266748=diff
==
--- cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp Tue Apr 19 10:52:56 2016
@@ -616,6 +616,10 @@ private:
 ActiveASTContext->getTranslationUnitDecl())
   return false;
 
+// For AST-nodes that don't have an identity, we can't memoize.
+if (!Builder->isComparable())
+  return matchesAncestorOfRecursively(Node, Matcher, Builder, MatchMode);
+
 MatchKey Key;
 Key.MatcherID = Matcher.getID();
 Key.Node = Node;
@@ -630,22 +634,34 @@ private:
 }
 
 MemoizedMatchResult Result;
-Result.ResultOfMatch = false;
 Result.Nodes = *Builder;
+Result.ResultOfMatch =
+matchesAncestorOfRecursively(Node, Matcher, , MatchMode);
+
+MemoizedMatchResult  = ResultCache[Key];
+CachedResult = std::move(Result);
+
+*Builder = CachedResult.Nodes;
+return CachedResult.ResultOfMatch;
+  }
 
+  bool matchesAncestorOfRecursively(const ast_type_traits::DynTypedNode ,
+const DynTypedMatcher ,
+BoundNodesTreeBuilder *Builder,
+AncestorMatchMode MatchMode) {
 const auto  = ActiveASTContext->getParents(Node);
 assert(!Parents.empty() && "Found node that is not in the parent map.");
 if (Parents.size() == 1) {
   // Only one parent - do recursive memoization.
   const ast_type_traits::DynTypedNode Parent = Parents[0];
-  if (Matcher.matches(Parent, this, )) {
-Result.ResultOfMatch = true;
-  } else if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
-// Reset the results to not include the bound nodes from the failed
-// match above.
-Result.Nodes = *Builder;
-Result.ResultOfMatch = memoizedMatchesAncestorOfRecursively(
-Parent, Matcher, , MatchMode);
+  BoundNodesTreeBuilder BuilderCopy = *Builder;
+  if (Matcher.matches(Parent, this, )) {
+*Builder = std::move(BuilderCopy);
+return true;
+  }
+  if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
+return memoizedMatchesAncestorOfRecursively(Parent, Matcher, Builder,
+MatchMode);
 // Once we get back from the recursive call, the result will be the
 // same as the parent's result.
   }
@@ -655,10 +671,10 @@ private:
   std::deque Queue(Parents.begin(),
   Parents.end());
   while (!Queue.empty()) {
-Result.Nodes = *Builder;
-if (Matcher.matches(Queue.front(), this, )) {
-  Result.ResultOfMatch = true;
-  break;
+BoundNodesTreeBuilder BuilderCopy = *Builder;
+if (Matcher.matches(Queue.front(), this, )) {
+  *Builder = std::move(BuilderCopy);
+  return true;
 }
 if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
   for (const auto  :
@@ -673,12 +689,7 @@ private:
 Queue.pop_front();
   }
 }
-
-MemoizedMatchResult  = ResultCache[Key];
-CachedResult = std::move(Result);
-
-*Builder = CachedResult.Nodes;
-return CachedResult.ResultOfMatch;
+return false;
   }
 
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with

Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp?rev=266748=266747=266748=diff
==
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp (original)
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp Tue Apr 19 10:52:56 2016
@@ -708,6 +708,19 @@ TEST(DeclarationMatcher, HasDescendantMe
   decl(anyOf(hasDescendant(RD), hasDescendant(VD);
 }
 
+TEST(DeclarationMatcher, HasAncestorMemoization) {
+  // This triggers an hasAncestor with a TemplateArgument in the bound nodes.
+  // That node can't be memoized so we have to check for it before trying to 
put
+  // 

Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Sent http://reviews.llvm.org/D19231 to fix the underlying bug in `hasAncestor`.
We can proceed with this change if you want, but it is not required anymore. I 
don't know whether we need the extra complexity of `TemplateArgumentLess`.


http://reviews.llvm.org/D19144



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

[PATCH] D19231: [ASTMatchers] Do not try to memoize nodes we can't compare.

[Samuel Benzaquen via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

Prevent hasAncestor from comparing nodes that are not supported.
hasDescendant was fixed some time ago to avoid this problem.
I'm applying the same fix to hasAncestor: if any object in the Builder map is
not comparable, skip the cache.

http://reviews.llvm.org/D19231

Files:
  lib/ASTMatchers/ASTMatchFinder.cpp
  unittests/ASTMatchers/ASTMatchersTest.cpp

Index: unittests/ASTMatchers/ASTMatchersTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -708,6 +708,19 @@
   decl(anyOf(hasDescendant(RD), hasDescendant(VD);
 }
 
+TEST(DeclarationMatcher, HasAncestorMemoization) {
+  // This triggers an hasAncestor with a TemplateArgument in the bound nodes.
+  // That node can't be memoized so we have to check for it before trying to put
+  // it on the cache.
+  DeclarationMatcher CannotMemoize = classTemplateSpecializationDecl(
+  hasAnyTemplateArgument(templateArgument().bind("targ")),
+  forEach(fieldDecl(hasAncestor(forStmt();
+
+  EXPECT_TRUE(notMatches("template  struct S;"
+ "template <> struct S{ int i; int j; };",
+ CannotMemoize));
+}
+
 TEST(DeclarationMatcher, HasAttr) {
   EXPECT_TRUE(matches("struct __attribute__((warn_unused)) X {};",
   decl(hasAttr(clang::attr::WarnUnused;
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -616,6 +616,10 @@
 ActiveASTContext->getTranslationUnitDecl())
   return false;
 
+// For AST-nodes that don't have an identity, we can't memoize.
+if (!Builder->isComparable())
+  return matchesAncestorOfRecursively(Node, Matcher, Builder, MatchMode);
+
 MatchKey Key;
 Key.MatcherID = Matcher.getID();
 Key.Node = Node;
@@ -630,22 +634,34 @@
 }
 
 MemoizedMatchResult Result;
-Result.ResultOfMatch = false;
 Result.Nodes = *Builder;
+Result.ResultOfMatch =
+matchesAncestorOfRecursively(Node, Matcher, , MatchMode);
+
+MemoizedMatchResult  = ResultCache[Key];
+CachedResult = std::move(Result);
+
+*Builder = CachedResult.Nodes;
+return CachedResult.ResultOfMatch;
+  }
 
+  bool matchesAncestorOfRecursively(const ast_type_traits::DynTypedNode ,
+const DynTypedMatcher ,
+BoundNodesTreeBuilder *Builder,
+AncestorMatchMode MatchMode) {
 const auto  = ActiveASTContext->getParents(Node);
 assert(!Parents.empty() && "Found node that is not in the parent map.");
 if (Parents.size() == 1) {
   // Only one parent - do recursive memoization.
   const ast_type_traits::DynTypedNode Parent = Parents[0];
-  if (Matcher.matches(Parent, this, )) {
-Result.ResultOfMatch = true;
-  } else if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
-// Reset the results to not include the bound nodes from the failed
-// match above.
-Result.Nodes = *Builder;
-Result.ResultOfMatch = memoizedMatchesAncestorOfRecursively(
-Parent, Matcher, , MatchMode);
+  BoundNodesTreeBuilder BuilderCopy = *Builder;
+  if (Matcher.matches(Parent, this, )) {
+*Builder = std::move(BuilderCopy);
+return true;
+  }
+  if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
+return memoizedMatchesAncestorOfRecursively(Parent, Matcher, Builder,
+MatchMode);
 // Once we get back from the recursive call, the result will be the
 // same as the parent's result.
   }
@@ -655,10 +671,10 @@
   std::deque Queue(Parents.begin(),
   Parents.end());
   while (!Queue.empty()) {
-Result.Nodes = *Builder;
-if (Matcher.matches(Queue.front(), this, )) {
-  Result.ResultOfMatch = true;
-  break;
+BoundNodesTreeBuilder BuilderCopy = *Builder;
+if (Matcher.matches(Queue.front(), this, )) {
+  *Builder = std::move(BuilderCopy);
+  return true;
 }
 if (MatchMode != ASTMatchFinder::AMM_ParentOnly) {
   for (const auto  :
@@ -673,12 +689,7 @@
 Queue.pop_front();
   }
 }
-
-MemoizedMatchResult  = ResultCache[Key];
-CachedResult = std::move(Result);
-
-*Builder = CachedResult.Nodes;
-return CachedResult.ResultOfMatch;
+return false;
   }
 
   // Implements a BoundNodesTree::Visitor that calls a MatchCallback with
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

To be more specific, the problem comes from the use of `hasAnscestor` (done by 
`isInTemplateInstantiation` ) while there is a `TemplateArgument` in the bound 
nodes.
This tries to put the node into the cache.
To trigger this easily you only need to have a hit in the cache.
I think you can trigger it by adding a second line 
`boost::lexical_cast(42);` in the `string_as_T` test. That second 
hit should get a cache hit and trigger the bug.


http://reviews.llvm.org/D19144



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

Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

I think the bug is coming from `memoizedMatchesAncestorOfRecursively`.
`memoizedMatchesRecursively` has a special case at the top to skip the cache if 
the node is not sortable. The other function should do that too.
Although the check is stale also because it is only checking for 
memoizationData and not whether the node itself works for < and ==.

Note that adding TemplateArgument to the function is ok, but that won't fix the 
bug because we still have other nodes that are not comparable.



Comment at: include/clang/AST/ASTTypeTraits.h:269
@@ -268,3 +268,3 @@
   ///
   /// Supports comparison of nodes that support memoization.
   /// FIXME: Implement comparsion for other node types (currently

this comment is stale.


http://reviews.llvm.org/D19144



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

Re: [PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL266369: [clang-tidy] Add check misc-multiple-statement-macro 
(authored by sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D18766?vs=53398=53786#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18766

Files:
  clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp
  clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.h
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/misc-multiple-statement-macro.rst
  clang-tools-extra/trunk/test/clang-tidy/misc-multiple-statement-macro.cpp

Index: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
@@ -25,6 +25,7 @@
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
@@ -79,6 +80,8 @@
 "misc-move-const-arg");
 CheckFactories.registerCheck(
 "misc-move-constructor-init");
+CheckFactories.registerCheck(
+"misc-multiple-statement-macro");
 CheckFactories.registerCheck(
 "misc-new-delete-overloads");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.h
+++ clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.h
@@ -0,0 +1,37 @@
+//===--- MultipleStatementMacroCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect multiple statement macros that are used in unbraced conditionals.
+/// Only the first statement of the macro will be inside the conditional and the
+/// other ones will be executed unconditionally.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-multiple-statement-macro.html
+class MultipleStatementMacroCheck : public ClangTidyCheck {
+public:
+  MultipleStatementMacroCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
Index: clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp
@@ -0,0 +1,106 @@
+//===--- MultipleStatementMacroCheck.cpp - clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MultipleStatementMacroCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+AST_MATCHER(Expr, isInMacro) { return Node.getLocStart().isMacroID(); }
+
+/// \brief Find the next statement after `S`.
+const Stmt *nextStmt(const MatchFinder::MatchResult , const Stmt *S) {
+  auto Parents = Result.Context->getParents(*S);
+  if (Parents.empty())
+return nullptr;
+  const Stmt *Parent = Parents[0].get();
+  if (!Parent)
+return nullptr;
+  const Stmt* Prev = nullptr;
+  for (const Stmt *Child : Parent->children()) {
+if (Prev == S)
+  return Child;
+Prev = Child;
+  }
+  return nextStmt(Result, Parent);
+}
+
+using ExpansionRanges = std::vector>;
+
+/// \bried Get all the macro expansion ranges related to `Loc`.

[clang-tools-extra] r266369 - [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Thu Apr 14 16:15:57 2016
New Revision: 266369

URL: http://llvm.org/viewvc/llvm-project?rev=266369=rev
Log:
[clang-tidy] Add check misc-multiple-statement-macro

Summary:
The check detects multi-statement macros that are used in unbraced conditionals.
Only the first statement will be part of the conditionals and the rest will fall
outside of it and executed unconditionally.

Reviewers: alexfh

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D18766

Added:
clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.h

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-multiple-statement-macro.rst
clang-tools-extra/trunk/test/clang-tidy/misc-multiple-statement-macro.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt?rev=266369=266368=266369=diff
==
--- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt Thu Apr 14 16:15:57 
2016
@@ -17,6 +17,7 @@ add_clang_library(clangTidyMiscModule
   MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
+  MultipleStatementMacroCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp

Modified: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp?rev=266369=266368=266369=diff
==
--- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp Thu Apr 14 
16:15:57 2016
@@ -25,6 +25,7 @@
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
@@ -79,6 +80,8 @@ public:
 "misc-move-const-arg");
 CheckFactories.registerCheck(
 "misc-move-constructor-init");
+CheckFactories.registerCheck(
+"misc-multiple-statement-macro");
 CheckFactories.registerCheck(
 "misc-new-delete-overloads");
 CheckFactories.registerCheck(

Added: clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp?rev=266369=auto
==
--- clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp 
(added)
+++ clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp Thu 
Apr 14 16:15:57 2016
@@ -0,0 +1,106 @@
+//===--- MultipleStatementMacroCheck.cpp - 
clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MultipleStatementMacroCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+AST_MATCHER(Expr, isInMacro) { return Node.getLocStart().isMacroID(); }
+
+/// \brief Find the next statement after `S`.
+const Stmt *nextStmt(const MatchFinder::MatchResult , const Stmt *S) {
+  auto Parents = Result.Context->getParents(*S);
+  if (Parents.empty())
+return nullptr;
+  const Stmt *Parent = Parents[0].get();
+  if (!Parent)
+return nullptr;
+  const Stmt* Prev = nullptr;
+  for (const Stmt *Child : Parent->children()) {
+if (Prev == S)
+  return Child;
+Prev = Child;
+  }
+  return nextStmt(Result, Parent);
+}
+
+using ExpansionRanges = std::vector>;
+
+/// \bried Get all the macro expansion ranges related to `Loc`.
+///
+/// The result is ordered from most inner to most outer.
+ExpansionRanges getExpansionRanges(SourceLocation Loc,
+   const MatchFinder::MatchResult ) {
+  ExpansionRanges Locs;
+  while (Loc.isMacroID()) {
+Locs.push_back(Result.SourceManager->getImmediateExpansionRange(Loc));
+Loc = Locs.back().first;
+  }
+  return Locs;
+}
+
+}  // namespace
+
+void 

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63
@@ +62,3 @@
+
+  Finder->addMatcher(returnStmt(IsBadReturnStatement, 
hasAncestor(IsGoodAssign))
+ .bind("returnStmt"),

baloghadamsoftware wrote:
> sbenza wrote:
> > baloghadamsoftware wrote:
> > > sbenza wrote:
> > > > baloghadamsoftware wrote:
> > > > > sbenza wrote:
> > > > > > I dislike these uses of hasAnscestor. They are kind of slow.
> > > > > > But more importantly, they break with nested functions/types.
> > > > > > This particular example is not checking that the return statement 
> > > > > > is from the assignment operator, only that it is within it. For 
> > > > > > example, it would match a lambda.
> > > > > > I think this would trip the check:
> > > > > > 
> > > > > > F& operator=(const F& o) {
> > > > > >   std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v 
> > > > > > > 0; });
> > > > > >   return *this;
> > > > > > }
> > > > > I can change it to hasDescendant if it is faster, but it does not 
> > > > > solve the lambda problem. No solution for that comes to my mind with 
> > > > > the existing matchers. Maybe a new matcher hasDescendantStatement 
> > > > > could help which only traverses statements down the AST. Is this the 
> > > > > right way to go?
> > > > hasDescendant has the same problem has hasAnscestor.
> > > > I think the best is to write a matcher like:
> > > > 
> > > > AST_MATCHER_P(ReturnStmt, forFunction, 
> > > > internal::Matcher, InnerMatcher) {
> > > >   ...
> > > > }
> > > > 
> > > > In there we can find the right FunctionDecl that encloses the return 
> > > > statement and apply the matcher.
> > > > This matcher seems like a good candidate to add to ASTMatchers.h
> > > Maybe I am wrong, but your proposal also seems a bottom-up matcher which 
> > > is slow. That is the reason I proposed a top-down matcher, e.g. 
> > > hasDescendantStatement or something like this which is top-down and only 
> > > traverses statements so it does not search in a lambda which is a 
> > > declaration.
> > hasAnscestor is slow because it is way too general. There are tons of 
> > virtual function calls, cache lookups, memory allocations, etc. And the 
> > search will not stop until it find a match or runs out of anscestors. This 
> > means it will go all the way to the translationUnitDecl before it figures 
> > out that there are no matches.
> > Every parent will be run through the matcher.
> > 
> > What I propose is a simple matcher that:
> >  1. Finds the enclosing FunctionDecl for a statement.
> >  2. Runs the matcher on it, if there is an enclosing function.
> > 
> > (1) can be done without any virtual function call and with no/minimal 
> > memory allocations.
> > (2) will be done only once on the found node.
> > 
> I did something like your proposal and it is working, but before uploading a 
> patch I have a question. There are some nodes, even statement having multiple 
> parents, probably due to template instantiations. Is it enough if we chose 
> the first parent here (thus going up only to the template itself) or should 
> we do a depth-first (better to say height-search here, since we are not 
> searching downwards in a tree but upwards in a DAG) search here and go up to 
> every parent? hasAncestor uses breadth-first search which is out of the 
> question here.
Yes, with templates you can have multiple parents.
I think you can use any of the parents, but it would be unspecified which 
FunctionDecl you get.
Maybe step (1) can return a list of FunctionDecls instead.
In this particular case you might need the particular template instantiation 
because the return type might be a dependent type.


http://reviews.llvm.org/D18265



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

Re: [PATCH] D19059: Reorder ASTNodeKind::AllKindInfo to match NodeKindId.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Those tests are testing the code completion you get in clang-query.
The type list must match what that matcher supports.

`hasParent` is declared as

  const internal::ArgumentAdaptingMatcherFunc<
  internal::HasParentMatcher,
  internal::TypeList,
  internal::TypeList>
  LLVM_ATTRIBUTE_UNUSED hasParent = {};

so the completion message is correct now.



Comment at: unittests/AST/ASTTypeTraitsTest.cpp:118
@@ +117,3 @@
+  VERIFY_NAME(NestedNameSpecifier);
+  VERIFY_NAME(Decl);
+  VERIFY_NAME(Stmt);

Add a derived Decl type too.
You added one for Stmt and Type, but not for Decl.


http://reviews.llvm.org/D19059



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

Re: [PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/MultipleStatementMacroCheck.cpp:99
@@ +98,3 @@
+
+  diag(InnerRanges.back().first, "multiple statement macro spans unbraced "
+ "conditional and the following statement");

alexfh wrote:
> If I saw this message from a tool, I wouldn't get what it means right away. 
> Can you come up with an easier to read alternative? I can only suggest 
> `multiple statement macro used without braces`, but maybe a more 
> self-documenting message comes to your mind.
Sure.
I wrote my methodology to find the problem, not the actual problem.
What about this?


http://reviews.llvm.org/D18766



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

Re: [PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 53398.
sbenza marked an inline comment as done.
sbenza added a comment.

Change warning message.


http://reviews.llvm.org/D18766

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-multiple-statement-macro.rst
  test/clang-tidy/misc-multiple-statement-macro.cpp

Index: test/clang-tidy/misc-multiple-statement-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-multiple-statement-macro.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-multiple-statement-macro %t
+
+void F();
+
+#define BAD_MACRO(x) \
+  F();   \
+  F()
+
+#define GOOD_MACRO(x) \
+  do {\
+F();  \
+F();  \
+  } while (0)
+
+#define GOOD_MACRO2(x) F()
+
+#define GOOD_MACRO3(x) F();
+
+#define MACRO_ARG_MACRO(X) \
+  if (54)  \
+  X(2)
+
+#define ALL_IN_MACRO(X) \
+  if (43)   \
+F();\
+  F()
+
+#define GOOD_NESTED(x)   \
+  if (x)\
+GOOD_MACRO3(x); \
+  F();
+
+#define IF(x) if(x)
+
+void positives() {
+  if (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used without braces; some statements will be unconditionally executed [misc-multiple-statement-macro]
+  if (1) {
+  } else
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used
+  while (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used
+  for (;;)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro used
+
+  MACRO_ARG_MACRO(BAD_MACRO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro used
+  MACRO_ARG_MACRO(F(); int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro used
+  IF(1) BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: multiple statement macro used
+}
+
+void negatives() {
+  if (1) {
+BAD_MACRO(1);
+  } else {
+BAD_MACRO(1);
+  }
+  while (1) {
+BAD_MACRO(1);
+  }
+  for (;;) {
+BAD_MACRO(1);
+  }
+
+  if (1)
+GOOD_MACRO(1);
+  if (1) {
+GOOD_MACRO(1);
+  }
+  if (1)
+GOOD_MACRO2(1);
+  if (1)
+GOOD_MACRO3(1);
+
+  MACRO_ARG_MACRO(GOOD_MACRO);
+  ALL_IN_MACRO(1);
+
+  IF(1) GOOD_MACRO(1);
+}
Index: docs/clang-tidy/checks/misc-multiple-statement-macro.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-multiple-statement-macro.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-multiple-statement-macro
+
+misc-multiple-statement-macro
+=
+
+Detect multiple statement macros that are used in unbraced conditionals.
+Only the first statement of the macro will be inside the conditional and the other ones will be executed unconditionally.
+
+Example:
+
+.. code:: c++
+
+  #define INCREMENT_TWO(x, y) (x)++; (y)++
+  if (do_increment)
+INCREMENT_TWO(a, b);  // `(b)++;` will be executed unconditionally.
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -60,6 +60,7 @@
misc-macro-repeated-side-effects
misc-misplaced-widening-cast
misc-move-constructor-init
+   misc-multiple-statement-macro
misc-new-delete-overloads
misc-noexcept-move-constructor
misc-non-copyable-objects
Index: clang-tidy/misc/MultipleStatementMacroCheck.h
===
--- /dev/null
+++ clang-tidy/misc/MultipleStatementMacroCheck.h
@@ -0,0 +1,37 @@
+//===--- MultipleStatementMacroCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect multiple statement macros that are used in unbraced conditionals.
+/// Only the first statement of the macro will be inside the conditional and the
+/// other ones will be executed unconditionally.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-multiple-statement-macro.html
+class MultipleStatementMacroCheck : public ClangTidyCheck {
+public:
+  MultipleStatementMacroCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}

Re: [PATCH] D18914: [clang-tidy] new readability-redundant-inline

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/readability/RedundantInlineCheck.cpp:68
@@ +67,3 @@
+  }
+  llvm_unreachable("InlineTok() did not encounter the 'inline' token");
+}

This is still reachable.
The token might be hidden through macros, for example.
But even then we still can keep a simple API. The possible return values are a 
token or 'not found'.
Optional should be ok.


http://reviews.llvm.org/D18914



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

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63
@@ +62,3 @@
+
+  Finder->addMatcher(returnStmt(IsBadReturnStatement, 
hasAncestor(IsGoodAssign))
+ .bind("returnStmt"),

baloghadamsoftware wrote:
> sbenza wrote:
> > baloghadamsoftware wrote:
> > > sbenza wrote:
> > > > I dislike these uses of hasAnscestor. They are kind of slow.
> > > > But more importantly, they break with nested functions/types.
> > > > This particular example is not checking that the return statement is 
> > > > from the assignment operator, only that it is within it. For example, 
> > > > it would match a lambda.
> > > > I think this would trip the check:
> > > > 
> > > > F& operator=(const F& o) {
> > > >   std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v > 0; 
> > > > });
> > > >   return *this;
> > > > }
> > > I can change it to hasDescendant if it is faster, but it does not solve 
> > > the lambda problem. No solution for that comes to my mind with the 
> > > existing matchers. Maybe a new matcher hasDescendantStatement could help 
> > > which only traverses statements down the AST. Is this the right way to go?
> > hasDescendant has the same problem has hasAnscestor.
> > I think the best is to write a matcher like:
> > 
> > AST_MATCHER_P(ReturnStmt, forFunction, internal::Matcher, 
> > InnerMatcher) {
> >   ...
> > }
> > 
> > In there we can find the right FunctionDecl that encloses the return 
> > statement and apply the matcher.
> > This matcher seems like a good candidate to add to ASTMatchers.h
> Maybe I am wrong, but your proposal also seems a bottom-up matcher which is 
> slow. That is the reason I proposed a top-down matcher, e.g. 
> hasDescendantStatement or something like this which is top-down and only 
> traverses statements so it does not search in a lambda which is a declaration.
hasAnscestor is slow because it is way too general. There are tons of virtual 
function calls, cache lookups, memory allocations, etc. And the search will not 
stop until it find a match or runs out of anscestors. This means it will go all 
the way to the translationUnitDecl before it figures out that there are no 
matches.
Every parent will be run through the matcher.

What I propose is a simple matcher that:
 1. Finds the enclosing FunctionDecl for a statement.
 2. Runs the matcher on it, if there is an enclosing function.

(1) can be done without any virtual function call and with no/minimal memory 
allocations.
(2) will be done only once on the found node.



http://reviews.llvm.org/D18265



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

Re: [PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: docs/clang-tidy/checks/misc-multiple-statement-macro.rst:15
@@ +14,3 @@
+  if (do_increment)
+INCREMENT_TWO(a, b);
+

hokein wrote:
> Would be better to add a comment to explain the sample.
The sentence just before the example explains what the problem is.


http://reviews.llvm.org/D18766



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

Re: [PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 52702.
sbenza marked 2 inline comments as done and an inline comment as not done.
sbenza added a comment.

Minor fixes


http://reviews.llvm.org/D18766

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-multiple-statement-macro.rst
  test/clang-tidy/misc-multiple-statement-macro.cpp

Index: test/clang-tidy/misc-multiple-statement-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-multiple-statement-macro.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-multiple-statement-macro %t
+
+void F();
+
+#define BAD_MACRO(x) \
+  F();   \
+  F()
+
+#define GOOD_MACRO(x) \
+  do {\
+F();  \
+F();  \
+  } while (0)
+
+#define GOOD_MACRO2(x) F()
+
+#define GOOD_MACRO3(x) F();
+
+#define MACRO_ARG_MACRO(X) \
+  if (54)  \
+  X(2)
+
+#define ALL_IN_MACRO(X) \
+  if (43)   \
+F();\
+  F()
+
+#define GOOD_NESTED(x)   \
+  if (x)\
+GOOD_MACRO3(x); \
+  F();
+
+#define IF(x) if(x)
+
+void positives() {
+  if (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans unbraced conditional and the following statement [misc-multiple-statement-macro]
+  if (1) {
+  } else
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+  while (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+  for (;;)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+
+  MACRO_ARG_MACRO(BAD_MACRO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro spans
+  MACRO_ARG_MACRO(F(); int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro spans
+  IF(1) BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: multiple statement macro spans
+}
+
+void negatives() {
+  if (1) {
+BAD_MACRO(1);
+  } else {
+BAD_MACRO(1);
+  }
+  while (1) {
+BAD_MACRO(1);
+  }
+  for (;;) {
+BAD_MACRO(1);
+  }
+
+  if (1)
+GOOD_MACRO(1);
+  if (1) {
+GOOD_MACRO(1);
+  }
+  if (1)
+GOOD_MACRO2(1);
+  if (1)
+GOOD_MACRO3(1);
+
+  MACRO_ARG_MACRO(GOOD_MACRO);
+  ALL_IN_MACRO(1);
+
+  IF(1) GOOD_MACRO(1);
+}
Index: docs/clang-tidy/checks/misc-multiple-statement-macro.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-multiple-statement-macro.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-multiple-statement-macro
+
+misc-multiple-statement-macro
+=
+
+Detect multiple statement macros that are used in unbraced conditionals.
+Only the first statement of the macro will be inside the conditional and the other ones will be executed unconditionally.
+
+Example:
+
+.. code:: c++
+
+  #define INCREMENT_TWO(x, y) (x)++; (y)++
+  if (do_increment)
+INCREMENT_TWO(a, b);
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -60,6 +60,7 @@
misc-macro-repeated-side-effects
misc-misplaced-widening-cast
misc-move-constructor-init
+   misc-multiple-statement-macro
misc-new-delete-overloads
misc-noexcept-move-constructor
misc-non-copyable-objects
Index: clang-tidy/misc/MultipleStatementMacroCheck.h
===
--- /dev/null
+++ clang-tidy/misc/MultipleStatementMacroCheck.h
@@ -0,0 +1,37 @@
+//===--- MultipleStatementMacroCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect multiple statement macros that are used in unbraced conditionals.
+/// Only the first statement of the macro will be inside the conditional and the
+/// other ones will be executed unconditionally.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-multiple-statement-macro.html
+class MultipleStatementMacroCheck : public ClangTidyCheck {
+public:
+  MultipleStatementMacroCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void 

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63
@@ +62,3 @@
+
+  Finder->addMatcher(returnStmt(IsBadReturnStatement, 
hasAncestor(IsGoodAssign))
+ .bind("returnStmt"),

baloghadamsoftware wrote:
> sbenza wrote:
> > I dislike these uses of hasAnscestor. They are kind of slow.
> > But more importantly, they break with nested functions/types.
> > This particular example is not checking that the return statement is from 
> > the assignment operator, only that it is within it. For example, it would 
> > match a lambda.
> > I think this would trip the check:
> > 
> > F& operator=(const F& o) {
> >   std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v > 0; });
> >   return *this;
> > }
> I can change it to hasDescendant if it is faster, but it does not solve the 
> lambda problem. No solution for that comes to my mind with the existing 
> matchers. Maybe a new matcher hasDescendantStatement could help which only 
> traverses statements down the AST. Is this the right way to go?
hasDescendant has the same problem has hasAnscestor.
I think the best is to write a matcher like:

AST_MATCHER_P(ReturnStmt, forFunction, internal::Matcher, 
InnerMatcher) {
  ...
}

In there we can find the right FunctionDecl that encloses the return statement 
and apply the matcher.
This matcher seems like a good candidate to add to ASTMatchers.h


http://reviews.llvm.org/D18265



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

Re: [PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

sbenza marked an inline comment as done and an inline comment as not done.


Comment at: clang-tidy/misc/MultipleStatementMacroCheck.cpp:33
@@ +32,3 @@
+
+namespace {
+

etienneb wrote:
> I feel it nicer if you merge this namespace with the one at line 20.
I like to put the helper functions closer to where they are used.
Merging the namespaces means I have to put everything at the top.


Comment at: clang-tidy/misc/MultipleStatementMacroCheck.cpp:42
@@ +41,3 @@
+  for (const Stmt *Child : Parent->children()) {
+if (found)
+  return Child;

etienneb wrote:
> Why not this?
> 
>   if (found)
> return Child;
>   else
> found = Child == S;// why the "|" is needed?
Fixed a different way.


http://reviews.llvm.org/D18766



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

Re: [PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 52625.
sbenza marked an inline comment as done.
sbenza added a comment.

Minor fixes


http://reviews.llvm.org/D18766

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-multiple-statement-macro.rst
  test/clang-tidy/misc-multiple-statement-macro.cpp

Index: test/clang-tidy/misc-multiple-statement-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-multiple-statement-macro.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-multiple-statement-macro %t
+
+void F();
+
+#define BAD_MACRO(x) \
+  F();   \
+  F()
+
+#define GOOD_MACRO(x) \
+  do {\
+F();  \
+F();  \
+  } while (0)
+
+#define GOOD_MACRO2(x) F()
+
+#define GOOD_MACRO3(x) F();
+
+#define MACRO_ARG_MACRO(X) \
+  if (54)  \
+  X(2)
+
+#define ALL_IN_MACRO(X) \
+  if (43)   \
+F();\
+  F()
+
+#define GOOD_NESTED(x)   \
+  if (x)\
+GOOD_MACRO3(x); \
+  F();
+
+#define IF(x) if(x)
+
+void positives() {
+  if (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans unbraced conditional and the following statement [misc-multiple-statement-macro]
+  if (1) {
+  } else
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+  while (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+  for (;;)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+
+  MACRO_ARG_MACRO(BAD_MACRO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro spans
+  MACRO_ARG_MACRO(F(); int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro spans
+  IF(1) BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: multiple statement macro spans
+}
+
+void negatives() {
+  if (1) {
+BAD_MACRO(1);
+  } else {
+BAD_MACRO(1);
+  }
+  while (1) {
+BAD_MACRO(1);
+  }
+  for (;;) {
+BAD_MACRO(1);
+  }
+
+  if (1)
+GOOD_MACRO(1);
+  if (1) {
+GOOD_MACRO(1);
+  }
+  if (1)
+GOOD_MACRO2(1);
+  if (1)
+GOOD_MACRO3(1);
+
+  MACRO_ARG_MACRO(GOOD_MACRO);
+  ALL_IN_MACRO(1);
+
+  IF(1) GOOD_MACRO(1);
+}
Index: docs/clang-tidy/checks/misc-multiple-statement-macro.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-multiple-statement-macro.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-multiple-statement-macro
+
+misc-multiple-statement-macro
+=
+
+Detect multiple statement macros that are used in unbraced conditionals.
+Only the first statement of the macro will be inside the conditional and the other ones will be executed unconditionally.
+
+Example:
+
+.. code:: c++
+
+  #define INCREMENT_TWO(x, y) (x)++; (y)++
+  if (do_increment)
+INCREMENT_TWO(a, b);
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -60,6 +60,7 @@
misc-macro-repeated-side-effects
misc-misplaced-widening-cast
misc-move-constructor-init
+   misc-multiple-statement-macro
misc-new-delete-overloads
misc-noexcept-move-constructor
misc-non-copyable-objects
Index: clang-tidy/misc/MultipleStatementMacroCheck.h
===
--- /dev/null
+++ clang-tidy/misc/MultipleStatementMacroCheck.h
@@ -0,0 +1,37 @@
+//===--- MultipleStatementMacroCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect multiple statement macros that are used in unbraced conditionals.
+/// Only the first statement of the macro will be inside the conditional and the
+/// other ones will be executed unconditionally.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-multiple-statement-macro.html
+class MultipleStatementMacroCheck : public ClangTidyCheck {
+public:
+  MultipleStatementMacroCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) 

[PATCH] D18766: [clang-tidy] Add check misc-multiple-statement-macro

[Samuel Benzaquen via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.

The check detects multi-statement macros that are used in unbraced conditionals.
Only the first statement will be part of the conditionals and the rest will fall
outside of it and executed unconditionally.

http://reviews.llvm.org/D18766

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.cpp
  clang-tidy/misc/MultipleStatementMacroCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-multiple-statement-macro.rst
  test/clang-tidy/misc-multiple-statement-macro.cpp

Index: test/clang-tidy/misc-multiple-statement-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-multiple-statement-macro.cpp
@@ -0,0 +1,85 @@
+// RUN: %check_clang_tidy %s misc-multiple-statement-macro %t
+
+void F();
+
+#define BAD_MACRO(x) \
+  F();   \
+  F()
+
+#define GOOD_MACRO(x) \
+  do {\
+F();  \
+F();  \
+  } while (0)
+
+#define GOOD_MACRO2(x) F()
+
+#define GOOD_MACRO3(x) F();
+
+#define MACRO_ARG_MACRO(X) \
+  if (54)  \
+  X(2)
+
+#define ALL_IN_MACRO(X) \
+  if (43)   \
+F();\
+  F()
+
+#define GOOD_NESTED(x)   \
+  if (x)\
+GOOD_MACRO3(x); \
+  F();
+
+#define IF(x) if(x)
+
+void positives() {
+  if (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans unbraced conditional and the following statement [misc-multiple-statement-macro]
+  if (1) {
+  } else
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+  while (1)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+  for (;;)
+BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: multiple statement macro spans
+
+  MACRO_ARG_MACRO(BAD_MACRO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro spans
+  MACRO_ARG_MACRO(F(); int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: multiple statement macro spans
+  IF(1) BAD_MACRO(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: multiple statement macro spans
+}
+
+void negatives() {
+  if (1) {
+BAD_MACRO(1);
+  } else {
+BAD_MACRO(1);
+  }
+  while (1) {
+BAD_MACRO(1);
+  }
+  for (;;) {
+BAD_MACRO(1);
+  }
+
+  if (1)
+GOOD_MACRO(1);
+  if (1) {
+GOOD_MACRO(1);
+  }
+  if (1)
+GOOD_MACRO2(1);
+  if (1)
+GOOD_MACRO3(1);
+
+  MACRO_ARG_MACRO(GOOD_MACRO);
+  ALL_IN_MACRO(1);
+
+  IF(1) GOOD_MACRO(1);
+}
Index: docs/clang-tidy/checks/misc-multiple-statement-macro.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-multiple-statement-macro.rst
@@ -0,0 +1,16 @@
+.. title:: clang-tidy - misc-multiple-statement-macro
+
+misc-multiple-statement-macro
+=
+
+Detect multiple statement macros that are used in unbraced conditionals.
+Only the first statement of the macro will be inside the conditional and the other ones will be executed unconditionally.
+
+Example:
+
+.. code:: c++
+
+  #define INCREMENT_TWO(x, y) (x)++; (y)++
+  if (do_increment)
+INCREMENT_TWO(a, b);
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -60,6 +60,7 @@
misc-macro-repeated-side-effects
misc-misplaced-widening-cast
misc-move-constructor-init
+   misc-multiple-statement-macro
misc-new-delete-overloads
misc-noexcept-move-constructor
misc-non-copyable-objects
Index: clang-tidy/misc/MultipleStatementMacroCheck.h
===
--- /dev/null
+++ clang-tidy/misc/MultipleStatementMacroCheck.h
@@ -0,0 +1,37 @@
+//===--- MultipleStatementMacroCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MULTIPLE_STATEMENT_MACRO_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect multiple statement macros that are used in unbraced conditionals.
+/// Only the first statement of the macro will be inside the conditional and the
+/// other ones will be executed unconditionally.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-multiple-statement-macro.html
+class MultipleStatementMacroCheck : public ClangTidyCheck {

Re: [PATCH] D18191: [clang-tidy] Add check for function parameters that are const& to builtin types

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

In http://reviews.llvm.org/D18191#391168, @sdowney wrote:

> At least in my codebase, skipping templates is too strong. I run across ones 
> where the const& parameter is not one controlled by a template. It's often a 
> size_t.


The only check we are doing (other than matching type) is matching spelling.
This means that it will only skip templates where that type is spelled, for 
example, using the template argument.

So it will skip this `template  void Foo(const T&);` but not this 
`template  void Foo(const T&, const size_t&);` (for the `size_t` 
argument).
It will also skip things like `void Foo(const T&, const typename 
T::difference_type&);`

The idea is that if the spelling is not exact, then there is some external 
factor that can change what the type means and make the by-value option less 
efficient.

> I could easily see not fixing the typedef'd refs, also, although I think 
> warning on them is still useful. Particularly if they can then be added to a 
> list to be changed. E.g. size_t.


Warning on types that will never be added to the list and that should be taken 
by `const&` will lead to constant false positives.
We could make these warnings opt-in. This way some users might turn it on 
always or just turn it on to find the list of types.


http://reviews.llvm.org/D18191



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

Re: [PATCH] D18191: [clang-tidy] Add check for function parameters that are const& to builtin types

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

As Alex mentioned, we have a test like this.
It also adds a hardcoded list of user-defined types that are known to be better 
when passed by value (eg. StringRef)

One big difference is that we decided to not trigger on typedefs.
We can't know that the typedef is documented to be trivial and it could change 
in the future.
The check actually verifies that the spelling is the expected spelling.
That skips things like macros, templates, type traits, typedefs, aliases, etc.

I could upstream that check and make the user-defined type list configurable.


http://reviews.llvm.org/D18191



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

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63
@@ +62,3 @@
+
+  Finder->addMatcher(returnStmt(IsBadReturnStatement, 
hasAncestor(IsGoodAssign))
+ .bind("returnStmt"),

I dislike these uses of hasAnscestor. They are kind of slow.
But more importantly, they break with nested functions/types.
This particular example is not checking that the return statement is from the 
assignment operator, only that it is within it. For example, it would match a 
lambda.
I think this would trip the check:

F& operator=(const F& o) {
  std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v > 0; });
  return *this;
}


Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:69
@@ +68,3 @@
+void AssignOperatorCheck::check(const MatchFinder::MatchResult ) {
+  const auto *Method = Result.Nodes.getNodeAs("method");
+  const auto *RetStmt = Result.Nodes.getNodeAs("returnStmt");

Move this closer to where it is used.


Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:70
@@ +69,3 @@
+  const auto *Method = Result.Nodes.getNodeAs("method");
+  const auto *RetStmt = Result.Nodes.getNodeAs("returnStmt");
+  if (RetStmt) {

Move this into the if () statement.


Comment at: clang-tidy/misc/AssignOperatorCheck.h:19
@@ +18,3 @@
+
+/// Finds declarations of assignment operators with the wrong return and/or
+/// argument types.

This does not talk about the return statement, only the return type.


Comment at: test/clang-tidy/misc-assign-operator.cpp:16
@@ +15,3 @@
+  AlsoGood& operator=(AlsoGood);
+};
+

This is a very common C++98 way of implementing copy-and-swap with copy elision 
support.
You do: `T& operator=(T t) { swap(t); return *this; }`
And it will avoid the copy if the argument is already a temporary due to copy 
elision on the caller.


Comment at: test/clang-tidy/misc-assign-operator.cpp:69
@@ +68,3 @@
+n = rhs.n;
+return *this;
+  }

This case is not a bad return statement, so it should not be in this class.


http://reviews.llvm.org/D18265



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

Re: r264428 - [ASTMatchers] Fix build for VariadicFunction.

[Samuel Benzaquen via cfe-commits]

On Thu, Mar 31, 2016 at 11:16 AM, David Blaikie <dblai...@gmail.com> wrote:

> Yep, reproduces even with a plain (non template arg) function pointer -
> and immediately goes away with a real function of the same type.
>

Thanks for the extra reduction.


> That's bothersome. I played around with a few ways of writing the
> call-to-function-pointer but couldn't find any particular syntax that
> seemed to untickle GCC's problem here. Maybe if you find the filed GCC bug
> there might be a workaround listed (if it's been filed/investigated
> already).
>

I couldn't not find a bug for it. Maybe someone with a little experience in
bugzilla has a little more luck?


>
> On Thu, Mar 31, 2016 at 6:53 AM, Samuel Benzaquen <sbe...@google.com>
> wrote:
>
>> I was able to reduce the bug to this:
>>
>> struct A {
>>
>>   A(int (&)[1]);
>>
>> };
>>
>> template 
>>
>> int E() {
>>
>>   int X[] = {0};
>>
>>   return F(X);
>>
>> }
>>
>>
>> Note that the array is not empty and we are not using variadics anymore.
>> The problem seems to be related to the function pointer template
>> parameter.
>> gcc doesn't want to do the implicit conversion from X to A, but if I make
>> the conversion explicit it works.
>>
>>
>> On Fri, Mar 25, 2016 at 1:58 PM, Alexey Samsonov <vonos...@gmail.com>
>> wrote:
>>
>>>
>>> On Fri, Mar 25, 2016 at 10:55 AM, David Blaikie via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Mar 25, 2016 at 10:46 AM, Samuel Benzaquen via cfe-commits <
>>>> cfe-commits@lists.llvm.org> wrote:
>>>>
>>>>> Author: sbenza
>>>>> Date: Fri Mar 25 12:46:02 2016
>>>>> New Revision: 264428
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=264428=rev
>>>>> Log:
>>>>> [ASTMatchers] Fix build for VariadicFunction.
>>>>>
>>>>> Under some conditions the implicit conversion from array to ArrayRef<>
>>>>> is not working.
>>>>>
>>>>
>>>> Any idea what those conditions are?
>>>>
>>>
>>> This was causing the build failure with opt GCC. I will try to create a
>>> smaller reproducer later today.
>>>
>>>
>>>>
>>>>
>>>>> Fix the build by making it explicit.
>>>>>
>>>>> Modified:
>>>>> cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>>>>
>>>>> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264428=264427=264428=diff
>>>>>
>>>>> ==
>>>>> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>>>> (original)
>>>>> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar
>>>>> 25 12:46:02 2016
>>>>> @@ -90,7 +90,7 @@ private:
>>>>>// before we make the array.
>>>>>template  ResultT Execute(const ArgsT &... Args)
>>>>> const {
>>>>>  const ArgT *const ArgsArray[] = {};
>>>>> -return Func(ArgsArray);
>>>>> +return Func(ArrayRef(ArgsArray, sizeof...(ArgsT)));
>>>>>}
>>>>>  };
>>>>>
>>>>>
>>>>>
>>>>> ___
>>>>> cfe-commits mailing list
>>>>> cfe-commits@lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>
>>>>
>>>> ___
>>>> cfe-commits mailing list
>>>> cfe-commits@lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>
>>>
>>> --
>>> Alexey Samsonov
>>> vonos...@gmail.com
>>>
>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r264428 - [ASTMatchers] Fix build for VariadicFunction.

[Samuel Benzaquen via cfe-commits]

I was able to reduce the bug to this:

struct A {

  A(int (&)[1]);

};

template 

int E() {

  int X[] = {0};

  return F(X);

}


Note that the array is not empty and we are not using variadics anymore.
The problem seems to be related to the function pointer template parameter.
gcc doesn't want to do the implicit conversion from X to A, but if I make
the conversion explicit it works.


On Fri, Mar 25, 2016 at 1:58 PM, Alexey Samsonov <vonos...@gmail.com> wrote:

>
> On Fri, Mar 25, 2016 at 10:55 AM, David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>>
>>
>> On Fri, Mar 25, 2016 at 10:46 AM, Samuel Benzaquen via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: sbenza
>>> Date: Fri Mar 25 12:46:02 2016
>>> New Revision: 264428
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=264428=rev
>>> Log:
>>> [ASTMatchers] Fix build for VariadicFunction.
>>>
>>> Under some conditions the implicit conversion from array to ArrayRef<>
>>> is not working.
>>>
>>
>> Any idea what those conditions are?
>>
>
> This was causing the build failure with opt GCC. I will try to create a
> smaller reproducer later today.
>
>
>>
>>
>>> Fix the build by making it explicit.
>>>
>>> Modified:
>>> cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>>
>>> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264428=264427=264428=diff
>>>
>>> ==
>>> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
>>> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar 25
>>> 12:46:02 2016
>>> @@ -90,7 +90,7 @@ private:
>>>// before we make the array.
>>>template  ResultT Execute(const ArgsT &... Args)
>>> const {
>>>  const ArgT *const ArgsArray[] = {};
>>> -return Func(ArgsArray);
>>> +return Func(ArrayRef(ArgsArray, sizeof...(ArgsT)));
>>>}
>>>  };
>>>
>>>
>>>
>>> ___
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>
>
> --
> Alexey Samsonov
> vonos...@gmail.com
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

This is already being done in http://reviews.llvm.org/D18582

On Wed, Mar 30, 2016 at 12:44 PM, Richard  wrote:

> LegalizeAdulthood added a subscriber: LegalizeAdulthood.
> LegalizeAdulthood added a comment.
>
> Can you add a synopsis of this new check to `docs/ReleaseNotes.rst` please?
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D17811
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL264759: [clang-tidy] Add check to detect dangling references 
in value handlers. (authored by sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D17811?vs=51684=51956#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D17811

Files:
  clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp
  clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h
  clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/misc-dangling-handle.rst
  clang-tools-extra/trunk/test/clang-tidy/misc-dangling-handle.cpp

Index: clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h
+++ clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h
@@ -0,0 +1,43 @@
+//===--- DanglingHandleCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Detect dangling references in value handlers like
+/// std::experimental::string_view.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-dangling-handle.html
+class DanglingHandleCheck : public ClangTidyCheck {
+public:
+  DanglingHandleCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+
+private:
+  void registerMatchersForVariables(ast_matchers::MatchFinder *Finder);
+  void registerMatchersForReturn(ast_matchers::MatchFinder *Finder);
+
+  const std::vector HandleClasses;
+  const ast_matchers::internal::Matcher IsAHandle;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DANGLING_HANDLE_H
Index: clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp
@@ -0,0 +1,185 @@
+//===--- DanglingHandleCheck.cpp - clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "DanglingHandleCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+static const char HandleClassesDelimiter[] = ";";
+
+std::vector parseClasses(StringRef Option) {
+  SmallVector Classes;
+  Option.split(Classes, HandleClassesDelimiter);
+  std::vector Result;
+  for (StringRef  : Classes) {
+Class = Class.trim();
+if (!Class.empty())
+  Result.push_back(Class);
+  }
+  return Result;
+}
+
+ast_matchers::internal::BindableMatcher handleFrom(
+ast_matchers::internal::Matcher IsAHandle,
+ast_matchers::internal::Matcher Arg) {
+  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+  hasArgument(0, Arg));
+}
+
+ast_matchers::internal::Matcher handleFromTemporaryValue(
+ast_matchers::internal::Matcher IsAHandle) {
+  // If a ternary operator returns a temporary value, then both branches hold a
+  // temporary value. If one of them is not a temporary then it must be copied
+  // into one to satisfy the type of the operator.
+  const auto TemporaryTernary =
+  conditionalOperator(hasTrueExpression(cxxBindTemporaryExpr()),
+  hasFalseExpression(cxxBindTemporaryExpr()));
+
+  return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), TemporaryTernary));
+}
+
+ast_matchers::internal::Matcher isASequence() {
+  return hasAnyName("::std::deque", "::std::forward_list", "::std::list",
+"::std::vector");
+}
+
+ast_matchers::internal::Matcher isASet() {
+  return hasAnyName("::std::set", "::std::multiset", "::std::unordered_set",
+

[clang-tools-extra] r264759 - [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Tue Mar 29 13:02:26 2016
New Revision: 264759

URL: http://llvm.org/viewvc/llvm-project?rev=264759=rev
Log:
[clang-tidy] Add check to detect dangling references in value handlers.

Summary:
Add check misc-dangling-handle to detect dangling references in value
handlers like std::experimental::string_view.
It provides a configuration option to specify other handle types that
should also be checked.

Right now it detects:
 - Construction from temporaries.
 - Assignment from temporaries.
 - Return statements from temporaries or locals.
 - Insertion into containers from temporaries.

Reviewers: alexfh

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D17811

Added:
clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/misc-dangling-handle.rst
clang-tools-extra/trunk/test/clang-tidy/misc-dangling-handle.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt?rev=264759=264758=264759=diff
==
--- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt Tue Mar 29 13:02:26 
2016
@@ -5,6 +5,7 @@ add_clang_library(clangTidyMiscModule
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
+  DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp

Added: clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp?rev=264759=auto
==
--- clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/misc/DanglingHandleCheck.cpp Tue Mar 29 
13:02:26 2016
@@ -0,0 +1,185 @@
+//===--- DanglingHandleCheck.cpp - 
clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "DanglingHandleCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+static const char HandleClassesDelimiter[] = ";";
+
+std::vector parseClasses(StringRef Option) {
+  SmallVector Classes;
+  Option.split(Classes, HandleClassesDelimiter);
+  std::vector Result;
+  for (StringRef  : Classes) {
+Class = Class.trim();
+if (!Class.empty())
+  Result.push_back(Class);
+  }
+  return Result;
+}
+
+ast_matchers::internal::BindableMatcher handleFrom(
+ast_matchers::internal::Matcher IsAHandle,
+ast_matchers::internal::Matcher Arg) {
+  return cxxConstructExpr(hasDeclaration(cxxMethodDecl(ofClass(IsAHandle))),
+  hasArgument(0, Arg));
+}
+
+ast_matchers::internal::Matcher handleFromTemporaryValue(
+ast_matchers::internal::Matcher IsAHandle) {
+  // If a ternary operator returns a temporary value, then both branches hold a
+  // temporary value. If one of them is not a temporary then it must be copied
+  // into one to satisfy the type of the operator.
+  const auto TemporaryTernary =
+  conditionalOperator(hasTrueExpression(cxxBindTemporaryExpr()),
+  hasFalseExpression(cxxBindTemporaryExpr()));
+
+  return handleFrom(IsAHandle, anyOf(cxxBindTemporaryExpr(), 
TemporaryTernary));
+}
+
+ast_matchers::internal::Matcher isASequence() {
+  return hasAnyName("::std::deque", "::std::forward_list", "::std::list",
+"::std::vector");
+}
+
+ast_matchers::internal::Matcher isASet() {
+  return hasAnyName("::std::set", "::std::multiset", "::std::unordered_set",
+"::std::unordered_multiset");
+}
+
+ast_matchers::internal::Matcher isAMap() {
+  return hasAnyName("::std::map", "::std::multimap", "::std::unordered_map",
+"::std::unordered_multimap");
+}
+
+ast_matchers::internal::BindableMatcher
+makeContainerMatcher(ast_matchers::internal::Matcher IsAHandle) {
+  // This matcher could be expanded to detect:
+  //  - Constructors: eg. vector(3, string("A"));
+  //  - emplace*(): This requires a different logic to determine that
+  //the 

Re: [PATCH] D18475: [clang-tidy] Add more detection rules for redundant c_str calls.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

In http://reviews.llvm.org/D18475#385789, @alexfh wrote:

> Looks good to me. Adding Samuel, since he has done a lot of similar stuff by 
> now and might have good ideas on improving this and making this more general.


There are two things I've done before that might apply here.
First, you can move the function+arg into data and just loop through it to make 
the matchers.
Eg:

  struct Case {
StringRef Func;
unsigned Arg;
  };
  const Case Cases[] = {...};
  for (Case C : Cases) {
Finder->addMatcher(...);
  }

This reduces the amount of code duplication.

Another thing I've done is to find matching overloads dynamically instead of 
hardcoding the list of functions.
The idea is that you match something like 
`callExpr(hasAnyArgument(StringCStrCallExpr))` and then you figure out if that 
function has a matching overload that takes `const string&` on that specific 
argument.
This is flexible, but might give false positives.


http://reviews.llvm.org/D18475



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

Re: r264428 - [ASTMatchers] Fix build for VariadicFunction.

[Samuel Benzaquen via cfe-commits]

On Fri, Mar 25, 2016 at 7:01 PM, Richard Smith <rich...@metafoo.co.uk>
wrote:

> On Fri, Mar 25, 2016 at 10:55 AM, David Blaikie via cfe-commits
> <cfe-commits@lists.llvm.org> wrote:
> >
> >
> > On Fri, Mar 25, 2016 at 10:46 AM, Samuel Benzaquen via cfe-commits
> > <cfe-commits@lists.llvm.org> wrote:
> >>
> >> Author: sbenza
> >> Date: Fri Mar 25 12:46:02 2016
> >> New Revision: 264428
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=264428=rev
> >> Log:
> >> [ASTMatchers] Fix build for VariadicFunction.
> >>
> >> Under some conditions the implicit conversion from array to ArrayRef<>
> >> is not working.
> >
> > Any idea what those conditions are?
>
> Well, the code's ill-formed (relying on a compiler extension) when
> Args is empty. Maybe GCC diagnoses that in some cases?
>

But Args is never empty on this function.
It is only every called from the 1+ arg overload of operator().
There is another overload for the 0-arg case to prevent this problem.


>
> >> Fix the build by making it explicit.
> >>
> >> Modified:
> >> cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
> >>
> >> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264428=264427=264428=diff
> >>
> >>
> ==
> >> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
> >> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar 25
> >> 12:46:02 2016
> >> @@ -90,7 +90,7 @@ private:
> >>// before we make the array.
> >>template  ResultT Execute(const ArgsT &... Args)
> >> const {
> >>  const ArgT *const ArgsArray[] = {};
> >> -return Func(ArgsArray);
> >> +return Func(ArrayRef(ArgsArray, sizeof...(ArgsT)));
> >>}
> >>  };
> >>
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 51684.
sbenza added a comment.

Using new public hasAnyName API.


http://reviews.llvm.org/D17811

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/DanglingHandleCheck.cpp
  clang-tidy/misc/DanglingHandleCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-dangling-handle.rst
  test/clang-tidy/misc-dangling-handle.cpp

Index: test/clang-tidy/misc-dangling-handle.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-dangling-handle.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s misc-dangling-handle %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: misc-dangling-handle.HandleClasses, \
+// RUN:   value: 'std::basic_string_view; ::llvm::StringRef;'}]}" \
+// RUN:   -- -std=c++11
+
+namespace std {
+
+template 
+class vector {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+  using size_type = int;
+
+  void assign(size_type count, const T& value);
+  iterator insert(const_iterator pos, const T& value);
+  iterator insert(const_iterator pos, T&& value);
+  iterator insert(const_iterator pos, size_type count, const T& value);
+  void push_back(const T&);
+  void push_back(T&&);
+  void resize(size_type count, const T& value);
+};
+
+template 
+class pair {};
+
+template 
+class set {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+
+  std::pair insert(const T& value);
+  std::pair insert(T&& value);
+  iterator insert(const_iterator hint, const T& value);
+  iterator insert(const_iterator hint, T&& value);
+};
+
+template 
+class map {
+ public:
+  using value_type = pair;
+  value_type& operator[](const Key& key);
+  value_type& operator[](Key&& key);
+};
+
+class basic_string {
+ public:
+  basic_string();
+  basic_string(const char*);
+  ~basic_string();
+};
+
+typedef basic_string string;
+
+class basic_string_view {
+ public:
+  basic_string_view(const char*);
+  basic_string_view(const basic_string&);
+};
+
+typedef basic_string_view string_view;
+
+}  // namespace std
+
+namespace llvm {
+
+class StringRef {
+ public:
+  StringRef();
+  StringRef(const char*);
+  StringRef(const std::string&);
+};
+
+}  // namespace llvm
+
+std::string ReturnsAString();
+
+void Positives() {
+  std::string_view view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [misc-dangling-handle]
+
+  std::string_view view_2 = ReturnsAString();
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+
+  view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
+
+  const std::string& str_ref = "";
+  std::string_view view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+  view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
+
+  std::string_view view4(ReturnsAString());
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+}
+
+void OtherTypes() {
+  llvm::StringRef ref = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
+}
+
+const char static_array[] = "A";
+std::string_view ReturnStatements(int i, std::string value_arg,
+  const std::string _arg) {
+  const char array[] = "A";
+  const char* ptr = "A";
+  std::string s;
+  static std::string ss;
+  switch (i) {
+// Bad cases
+case 0:
+  return array;  // refers to local
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 1:
+  return s;  // refers to local
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 2:
+  return std::string();  // refers to temporary
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 3:
+  return value_arg;  // refers to by-value arg
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+
+// Ok cases
+case 100:
+  return ss;  // refers to static
+case 101:
+  return static_array;  // refers to static
+case 102:
+  return ptr;  // pointer is ok
+case 103:
+  return ref_arg;  // refers to by-ref arg
+  }
+
+  struct S {
+std::string_view view() { return value; }
+std::string value;
+  };
+
+  (void)[&]()->std::string_view {
+// This should not warn. The string is bound by reference.
+return s;
+  };
+  (void)[=]() -> std::string_view {
+// This should not warn. The reference is valid as long as the lambda.
+return s;
+  };
+  (void)[=]() -> std::string_view {
+// FIXME: This one should warn. We are returning a reference to a local
+// lambda variable.
+std::string local;
+return 

Re: r264417 - [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

Fixed in r264453.
Sorry for the inconvenience.

_Sam

On Fri, Mar 25, 2016 at 3:31 PM, Samuel Benzaquen <sbe...@google.com> wrote:

> Sorry, this is an unrelated problem.
> It is using brace-init lists and the supported MSVC version does not
> support them.
> I'll fix that.
>
>
> On Fri, Mar 25, 2016 at 3:27 PM, Mehdi Amini <mehdi.am...@apple.com>
> wrote:
>
>> The link I provided is testing r264430...
>>
>> --
>> Mehdi
>>
>> On Mar 25, 2016, at 12:25 PM, Samuel Benzaquen <sbe...@google.com> wrote:
>>
>> I believe r264428 fixes this problem.
>>
>> On Fri, Mar 25, 2016 at 2:18 PM, Mehdi Amini <mehdi.am...@apple.com>
>> wrote:
>>
>>> Hi,
>>>
>>> I think this broke clang-tidy somehow:
>>> http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/10881/steps/build%20stage%201/logs/stdio
>>>
>>> --
>>> Mehdi
>>>
>>>
>>>
>>>
>>> > On Mar 25, 2016, at 9:29 AM, Samuel Benzaquen via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>> >
>>> > Author: sbenza
>>> > Date: Fri Mar 25 11:29:30 2016
>>> > New Revision: 264417
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=264417=rev
>>> > Log:
>>> > [ASTMatchers] Add own version of VariadicFunction.
>>> >
>>> > Summary:
>>> > llvm::VariadicFunction is only being used by ASTMatchers.
>>> > Having our own copy here allows us to remove the other one from
>>> llvm/ADT.
>>> > Also, we can extend the API to meet our needs without modifying the
>>> common
>>> > implementation.
>>> >
>>> > Reviewers: alexfh
>>> >
>>> > Subscribers: klimek, cfe-commits
>>> >
>>> > Differential Revision: http://reviews.llvm.org/D18275
>>> >
>>> > Modified:
>>> >cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
>>> >cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>> >cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
>>> >cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
>>> >
>>> > Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
>>> > URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=264417=264416=264417=diff
>>> >
>>> ==
>>> > --- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
>>> > +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Fri Mar 25
>>> 11:29:30 2016
>>> > @@ -1990,8 +1990,8 @@ inline internal::Matcher hasN
>>> > /// \code
>>> > /// anyOf(hasName(a), hasName(b), hasName(c))
>>> > /// \endcode
>>> > -const llvm::VariadicFunction<internal::Matcher, StringRef,
>>> > - internal::hasAnyNameFunc>
>>> > +const internal::VariadicFunction<internal::Matcher,
>>> StringRef,
>>> > + internal::hasAnyNameFunc>
>>> > hasAnyName = {};
>>> >
>>> > /// \brief Matches NamedDecl nodes whose fully qualified names contain
>>> >
>>> > Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>> > URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264417=264416=264417=diff
>>> >
>>> ==
>>> > --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>>> (original)
>>> > +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar
>>> 25 11:29:30 2016
>>> > @@ -46,8 +46,9 @@
>>> > #include "clang/AST/StmtCXX.h"
>>> > #include "clang/AST/StmtObjC.h"
>>> > #include "clang/AST/Type.h"
>>> > +#include "llvm/ADT/ArrayRef.h"
>>> > #include "llvm/ADT/Optional.h"
>>> > -#include "llvm/ADT/VariadicFunction.h"
>>> > +#include "llvm/ADT/SmallVector.h"
>>> > #include "llvm/Support/ManagedStatic.h"
>>> > #include 
>>> > #include 
>>> > @@ -60,6 +61,39 @@ class BoundNodes;
>>> >
>>> > namespace internal {
>>> >
>>> &g

r264453 - [ASTMatchers] Don't use brace-init lists.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Fri Mar 25 14:41:32 2016
New Revision: 264453

URL: http://llvm.org/viewvc/llvm-project?rev=264453=rev
Log:
[ASTMatchers] Don't use brace-init lists.

They are not supported everywhere yet.
This fixes the MSVC build.

Modified:
cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264453=264452=264453=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar 25 
14:41:32 2016
@@ -69,7 +69,7 @@ namespace internal {
 template )>
 struct VariadicFunction {
-  ResultT operator()() const { return Func({}); }
+  ResultT operator()() const { return Func(None); }
 
   template 
   ResultT operator()(const ArgT , const ArgsT &... Args) const {


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

Re: r264417 - [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

Sorry, this is an unrelated problem.
It is using brace-init lists and the supported MSVC version does not
support them.
I'll fix that.

On Fri, Mar 25, 2016 at 3:27 PM, Mehdi Amini <mehdi.am...@apple.com> wrote:

> The link I provided is testing r264430...
>
> --
> Mehdi
>
> On Mar 25, 2016, at 12:25 PM, Samuel Benzaquen <sbe...@google.com> wrote:
>
> I believe r264428 fixes this problem.
>
> On Fri, Mar 25, 2016 at 2:18 PM, Mehdi Amini <mehdi.am...@apple.com>
> wrote:
>
>> Hi,
>>
>> I think this broke clang-tidy somehow:
>> http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/10881/steps/build%20stage%201/logs/stdio
>>
>> --
>> Mehdi
>>
>>
>>
>>
>> > On Mar 25, 2016, at 9:29 AM, Samuel Benzaquen via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>> >
>> > Author: sbenza
>> > Date: Fri Mar 25 11:29:30 2016
>> > New Revision: 264417
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=264417=rev
>> > Log:
>> > [ASTMatchers] Add own version of VariadicFunction.
>> >
>> > Summary:
>> > llvm::VariadicFunction is only being used by ASTMatchers.
>> > Having our own copy here allows us to remove the other one from
>> llvm/ADT.
>> > Also, we can extend the API to meet our needs without modifying the
>> common
>> > implementation.
>> >
>> > Reviewers: alexfh
>> >
>> > Subscribers: klimek, cfe-commits
>> >
>> > Differential Revision: http://reviews.llvm.org/D18275
>> >
>> > Modified:
>> >cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
>> >cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>> >cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
>> >cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
>> >
>> > Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=264417=264416=264417=diff
>> >
>> ==
>> > --- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
>> > +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Fri Mar 25
>> 11:29:30 2016
>> > @@ -1990,8 +1990,8 @@ inline internal::Matcher hasN
>> > /// \code
>> > /// anyOf(hasName(a), hasName(b), hasName(c))
>> > /// \endcode
>> > -const llvm::VariadicFunction<internal::Matcher, StringRef,
>> > - internal::hasAnyNameFunc>
>> > +const internal::VariadicFunction<internal::Matcher,
>> StringRef,
>> > + internal::hasAnyNameFunc>
>> > hasAnyName = {};
>> >
>> > /// \brief Matches NamedDecl nodes whose fully qualified names contain
>> >
>> > Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264417=264416=264417=diff
>> >
>> ==
>> > --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
>> > +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar
>> 25 11:29:30 2016
>> > @@ -46,8 +46,9 @@
>> > #include "clang/AST/StmtCXX.h"
>> > #include "clang/AST/StmtObjC.h"
>> > #include "clang/AST/Type.h"
>> > +#include "llvm/ADT/ArrayRef.h"
>> > #include "llvm/ADT/Optional.h"
>> > -#include "llvm/ADT/VariadicFunction.h"
>> > +#include "llvm/ADT/SmallVector.h"
>> > #include "llvm/Support/ManagedStatic.h"
>> > #include 
>> > #include 
>> > @@ -60,6 +61,39 @@ class BoundNodes;
>> >
>> > namespace internal {
>> >
>> > +/// \brief Variadic function object.
>> > +///
>> > +/// Most of the functions below that use VariadicFunction could be
>> implemented
>> > +/// using plain C++11 variadic functions, but the function object
>> allows us to
>> > +/// capture it on the dynamic matcher registry.
>> > +template > > +  ResultT (*Func)(ArrayRef)>
>> > +struct VariadicFunction {
>> > +  ResultT operator()() const { return Func({}); }
>> > +
>> > +  template 
>> > +  ResultT operator()

Re: r264417 - [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

I believe r264428 fixes this problem.

On Fri, Mar 25, 2016 at 2:18 PM, Mehdi Amini <mehdi.am...@apple.com> wrote:

> Hi,
>
> I think this broke clang-tidy somehow:
> http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/10881/steps/build%20stage%201/logs/stdio
>
> --
> Mehdi
>
>
>
>
> > On Mar 25, 2016, at 9:29 AM, Samuel Benzaquen via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> >
> > Author: sbenza
> > Date: Fri Mar 25 11:29:30 2016
> > New Revision: 264417
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=264417=rev
> > Log:
> > [ASTMatchers] Add own version of VariadicFunction.
> >
> > Summary:
> > llvm::VariadicFunction is only being used by ASTMatchers.
> > Having our own copy here allows us to remove the other one from llvm/ADT.
> > Also, we can extend the API to meet our needs without modifying the
> common
> > implementation.
> >
> > Reviewers: alexfh
> >
> > Subscribers: klimek, cfe-commits
> >
> > Differential Revision: http://reviews.llvm.org/D18275
> >
> > Modified:
> >cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
> >cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
> >cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
> >cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp
> >
> > Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=264417=264416=264417=diff
> >
> ==
> > --- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
> > +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Fri Mar 25
> 11:29:30 2016
> > @@ -1990,8 +1990,8 @@ inline internal::Matcher hasN
> > /// \code
> > /// anyOf(hasName(a), hasName(b), hasName(c))
> > /// \endcode
> > -const llvm::VariadicFunction<internal::Matcher, StringRef,
> > - internal::hasAnyNameFunc>
> > +const internal::VariadicFunction<internal::Matcher,
> StringRef,
> > + internal::hasAnyNameFunc>
> > hasAnyName = {};
> >
> > /// \brief Matches NamedDecl nodes whose fully qualified names contain
> >
> > Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264417=264416=264417=diff
> >
> ==
> > --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
> > +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar 25
> 11:29:30 2016
> > @@ -46,8 +46,9 @@
> > #include "clang/AST/StmtCXX.h"
> > #include "clang/AST/StmtObjC.h"
> > #include "clang/AST/Type.h"
> > +#include "llvm/ADT/ArrayRef.h"
> > #include "llvm/ADT/Optional.h"
> > -#include "llvm/ADT/VariadicFunction.h"
> > +#include "llvm/ADT/SmallVector.h"
> > #include "llvm/Support/ManagedStatic.h"
> > #include 
> > #include 
> > @@ -60,6 +61,39 @@ class BoundNodes;
> >
> > namespace internal {
> >
> > +/// \brief Variadic function object.
> > +///
> > +/// Most of the functions below that use VariadicFunction could be
> implemented
> > +/// using plain C++11 variadic functions, but the function object
> allows us to
> > +/// capture it on the dynamic matcher registry.
> > +template  > +  ResultT (*Func)(ArrayRef)>
> > +struct VariadicFunction {
> > +  ResultT operator()() const { return Func({}); }
> > +
> > +  template 
> > +  ResultT operator()(const ArgT , const ArgsT &... Args) const {
> > +return Execute(Arg1, static_cast(Args)...);
> > +  }
> > +
> > +  // We also allow calls with an already created array, in case the
> caller
> > +  // already had it.
> > +  ResultT operator()(ArrayRef Args) const {
> > +SmallVector InnerArgs;
> > +for (const ArgT  : Args)
> > +  InnerArgs.push_back();
> > +return Func(InnerArgs);
> > +  }
> > +
> > +private:
> > +  // Trampoline function to allow for implicit conversions to take place
> > +  // before we make the array.
> > +  template  ResultT Execute(const ArgsT &... Args)
> const {
> > +const ArgT *const ArgsArray[] = {};
> > +return Func(ArgsArray);
> &

Re: [PATCH] D18275: [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

On Fri, Mar 25, 2016 at 1:55 PM, Etienne Bergeron 
wrote:

> etienneb added a subscriber: etienneb.
> etienneb added a comment.
>
> Any plan for doing the same for : hasOverloadedOperatorName ?
>

hasAnyName() was added mostly for performance reasons.
We could add the 'Any' version of hasOverloadedOperatorName, but there
doesn't seem to be that much need for it right now.


>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D18275
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r264428 - [ASTMatchers] Fix build for VariadicFunction.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Fri Mar 25 12:46:02 2016
New Revision: 264428

URL: http://llvm.org/viewvc/llvm-project?rev=264428=rev
Log:
[ASTMatchers] Fix build for VariadicFunction.

Under some conditions the implicit conversion from array to ArrayRef<>
is not working.
Fix the build by making it explicit.

Modified:
cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264428=264427=264428=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar 25 
12:46:02 2016
@@ -90,7 +90,7 @@ private:
   // before we make the array.
   template  ResultT Execute(const ArgsT &... Args) const {
 const ArgT *const ArgsArray[] = {};
-return Func(ArgsArray);
+return Func(ArrayRef(ArgsArray, sizeof...(ArgsT)));
   }
 };
 


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

Re: [PATCH] D18275: [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL264417: [ASTMatchers] Add own version of VariadicFunction. 
(authored by sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D18275?vs=51447=51645#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18275

Files:
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
  cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
  cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -1990,8 +1990,8 @@
 /// \code
 /// anyOf(hasName(a), hasName(b), hasName(c))
 /// \endcode
-const llvm::VariadicFunction
+const internal::VariadicFunction
 hasAnyName = {};
 
 /// \brief Matches NamedDecl nodes whose fully qualified names contain
Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -46,8 +46,9 @@
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/Type.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/ADT/VariadicFunction.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ManagedStatic.h"
 #include 
 #include 
@@ -60,6 +61,39 @@
 
 namespace internal {
 
+/// \brief Variadic function object.
+///
+/// Most of the functions below that use VariadicFunction could be implemented
+/// using plain C++11 variadic functions, but the function object allows us to
+/// capture it on the dynamic matcher registry.
+template )>
+struct VariadicFunction {
+  ResultT operator()() const { return Func({}); }
+
+  template 
+  ResultT operator()(const ArgT , const ArgsT &... Args) const {
+return Execute(Arg1, static_cast(Args)...);
+  }
+
+  // We also allow calls with an already created array, in case the caller
+  // already had it.
+  ResultT operator()(ArrayRef Args) const {
+SmallVector InnerArgs;
+for (const ArgT  : Args)
+  InnerArgs.push_back();
+return Func(InnerArgs);
+  }
+
+private:
+  // Trampoline function to allow for implicit conversions to take place
+  // before we make the array.
+  template  ResultT Execute(const ArgsT &... Args) const {
+const ArgT *const ArgsArray[] = {};
+return Func(ArgsArray);
+  }
+};
+
 /// \brief Unifies obtaining the underlying type of a regular node through
 /// `getType` and a TypedefNameDecl node through `getUnderlyingType`.
 template 
@@ -1405,9 +1439,8 @@
 /// casted to CXXRecordDecl and all given matchers match.
 template 
 class VariadicDynCastAllOfMatcher
-: public llvm::VariadicFunction<
-BindableMatcher, Matcher,
-makeDynCastAllOfComposite > {
+: public VariadicFunction> {
 public:
   VariadicDynCastAllOfMatcher() {}
 };
@@ -1423,9 +1456,9 @@
 /// \c Matcher.
 /// The returned matcher matches if all given matchers match.
 template 
-class VariadicAllOfMatcher : public llvm::VariadicFunction<
-   BindableMatcher, Matcher,
-   makeAllOfComposite > {
+class VariadicAllOfMatcher
+: public VariadicFunction {
 public:
   VariadicAllOfMatcher() {}
 };
@@ -1546,8 +1579,8 @@
 new MatcherImpl(InnerMatcher, Getter::value()));
   }
 
-  struct Func : public llvm::VariadicFunction {
+  struct Func
+  : public VariadicFunction {
 Func() {}
   };
 
Index: cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
===
--- cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
@@ -325,8 +325,9 @@
 
   template )>
-  VariadicFuncMatcherDescriptor(llvm::VariadicFunction Func,
-  StringRef MatcherName)
+  VariadicFuncMatcherDescriptor(
+  ast_matchers::internal::VariadicFunction Func,
+  StringRef MatcherName)
   : Func(),
 MatcherName(MatcherName.str()),
 ArgsKind(ArgTypeTraits::getKind()) {
@@ -655,9 +656,9 @@
 /// \brief Variadic overload.
 template )>
-MatcherDescriptor *
-makeMatcherAutoMarshall(llvm::VariadicFunction 

r264417 - [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Fri Mar 25 11:29:30 2016
New Revision: 264417

URL: http://llvm.org/viewvc/llvm-project?rev=264417=rev
Log:
[ASTMatchers] Add own version of VariadicFunction.

Summary:
llvm::VariadicFunction is only being used by ASTMatchers.
Having our own copy here allows us to remove the other one from llvm/ADT.
Also, we can extend the API to meet our needs without modifying the common
implementation.

Reviewers: alexfh

Subscribers: klimek, cfe-commits

Differential Revision: http://reviews.llvm.org/D18275

Modified:
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=264417=264416=264417=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Fri Mar 25 11:29:30 2016
@@ -1990,8 +1990,8 @@ inline internal::Matcher hasN
 /// \code
 /// anyOf(hasName(a), hasName(b), hasName(c))
 /// \endcode
-const llvm::VariadicFunction
+const internal::VariadicFunction
 hasAnyName = {};
 
 /// \brief Matches NamedDecl nodes whose fully qualified names contain

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=264417=264416=264417=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Fri Mar 25 
11:29:30 2016
@@ -46,8 +46,9 @@
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/Type.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/ADT/VariadicFunction.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ManagedStatic.h"
 #include 
 #include 
@@ -60,6 +61,39 @@ class BoundNodes;
 
 namespace internal {
 
+/// \brief Variadic function object.
+///
+/// Most of the functions below that use VariadicFunction could be implemented
+/// using plain C++11 variadic functions, but the function object allows us to
+/// capture it on the dynamic matcher registry.
+template )>
+struct VariadicFunction {
+  ResultT operator()() const { return Func({}); }
+
+  template 
+  ResultT operator()(const ArgT , const ArgsT &... Args) const {
+return Execute(Arg1, static_cast(Args)...);
+  }
+
+  // We also allow calls with an already created array, in case the caller
+  // already had it.
+  ResultT operator()(ArrayRef Args) const {
+SmallVector InnerArgs;
+for (const ArgT  : Args)
+  InnerArgs.push_back();
+return Func(InnerArgs);
+  }
+
+private:
+  // Trampoline function to allow for implicit conversions to take place
+  // before we make the array.
+  template  ResultT Execute(const ArgsT &... Args) const {
+const ArgT *const ArgsArray[] = {};
+return Func(ArgsArray);
+  }
+};
+
 /// \brief Unifies obtaining the underlying type of a regular node through
 /// `getType` and a TypedefNameDecl node through `getUnderlyingType`.
 template 
@@ -1405,9 +1439,8 @@ inline bool ValueEqualsMatcher
 class VariadicDynCastAllOfMatcher
-: public llvm::VariadicFunction<
-BindableMatcher, Matcher,
-makeDynCastAllOfComposite > {
+: public VariadicFunction> {
 public:
   VariadicDynCastAllOfMatcher() {}
 };
@@ -1423,9 +1456,9 @@ public:
 /// \c Matcher.
 /// The returned matcher matches if all given matchers match.
 template 
-class VariadicAllOfMatcher : public llvm::VariadicFunction<
-   BindableMatcher, Matcher,
-   makeAllOfComposite > {
+class VariadicAllOfMatcher
+: public VariadicFunction {
 public:
   VariadicAllOfMatcher() {}
 };
@@ -1546,8 +1579,8 @@ public:
 new MatcherImpl(InnerMatcher, Getter::value()));
   }
 
-  struct Func : public llvm::VariadicFunction {
+  struct Func
+  : public VariadicFunction {
 Func() {}
   };
 

Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h?rev=264417=264416=264417=diff

Re: [PATCH] D18275: [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:80
@@ +79,3 @@
+  ResultT operator()(ArrayRef Args) const {
+std::vector InnerArgs;
+for (const ArgT  : Args)

alexfh wrote:
> It's unfortunate that we need to create an array of the argument pointers 
> here, but it seems there's no larger common denominator of the two ways this 
> functions can be called.
> 
> One nit though: SmallVector will be a better container here.
> It's unfortunate that we need to create an array of the argument pointers 
> here, but it seems there's no larger  common denominator of the two ways this 
> functions can be called.

Now that we control it, we can change it to be something different.
For example, instead of passing a function pointer as template parameter we 
could pass a class that contains overloaded operator() for both ArrayRef and 
ArrayRef.

On the other hand, constructing the matchers has never been the performance 
bottleneck of the framework.
They are usually created once and used thousands/millions of times.
Might not be worth it to optimize this too much.

> One nit though: SmallVector will be a better container here.

Done.


http://reviews.llvm.org/D18275



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

Re: [PATCH] D18275: [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 51447.
sbenza added a comment.

Minor fix


http://reviews.llvm.org/D18275

Files:
  include/clang/ASTMatchers/ASTMatchers.h
  include/clang/ASTMatchers/ASTMatchersInternal.h
  lib/ASTMatchers/Dynamic/Marshallers.h
  unittests/ASTMatchers/ASTMatchersTest.cpp

Index: unittests/ASTMatchers/ASTMatchersTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -3000,6 +3000,9 @@
   EXPECT_TRUE(notMatches(Code, recordDecl(hasAnyName("::C", "::b::C";
   EXPECT_TRUE(
   matches(Code, recordDecl(hasAnyName("::C", "::b::C", "::a::b::C";
+
+  std::vector Names = {"::C", "::b::C", "::a::b::C"};
+  EXPECT_TRUE(matches(Code, recordDecl(hasAnyName(Names;
 }
 
 TEST(Matcher, IsDefinition) {
Index: lib/ASTMatchers/Dynamic/Marshallers.h
===
--- lib/ASTMatchers/Dynamic/Marshallers.h
+++ lib/ASTMatchers/Dynamic/Marshallers.h
@@ -325,8 +325,9 @@
 
   template )>
-  VariadicFuncMatcherDescriptor(llvm::VariadicFunction Func,
-  StringRef MatcherName)
+  VariadicFuncMatcherDescriptor(
+  ast_matchers::internal::VariadicFunction Func,
+  StringRef MatcherName)
   : Func(),
 MatcherName(MatcherName.str()),
 ArgsKind(ArgTypeTraits::getKind()) {
@@ -655,9 +656,9 @@
 /// \brief Variadic overload.
 template )>
-MatcherDescriptor *
-makeMatcherAutoMarshall(llvm::VariadicFunction VarFunc,
-StringRef MatcherName) {
+MatcherDescriptor *makeMatcherAutoMarshall(
+ast_matchers::internal::VariadicFunction VarFunc,
+StringRef MatcherName) {
   return new VariadicFuncMatcherDescriptor(VarFunc, MatcherName);
 }
 
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -46,8 +46,9 @@
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/Type.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/ADT/VariadicFunction.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ManagedStatic.h"
 #include 
 #include 
@@ -60,6 +61,39 @@
 
 namespace internal {
 
+/// \brief Variadic function object.
+///
+/// Most of the functions below that use VariadicFunction could be implemented
+/// using plain C++11 variadic functions, but the function object allows us to
+/// capture it on the dynamic matcher registry.
+template )>
+struct VariadicFunction {
+  ResultT operator()() const { return Func({}); }
+
+  template 
+  ResultT operator()(const ArgT , const ArgsT &... Args) const {
+return Execute(Arg1, static_cast(Args)...);
+  }
+
+  // We also allow calls with an already created array, in case the caller
+  // already had it.
+  ResultT operator()(ArrayRef Args) const {
+SmallVector InnerArgs;
+for (const ArgT  : Args)
+  InnerArgs.push_back();
+return Func(InnerArgs);
+  }
+
+private:
+  // Trampoline function to allow for implicit conversions to take place
+  // before we make the array.
+  template  ResultT Execute(const ArgsT &... Args) const {
+const ArgT *const ArgsArray[] = {};
+return Func(ArgsArray);
+  }
+};
+
 /// \brief Unifies obtaining the underlying type of a regular node through
 /// `getType` and a TypedefNameDecl node through `getUnderlyingType`.
 template 
@@ -1405,9 +1439,8 @@
 /// casted to CXXRecordDecl and all given matchers match.
 template 
 class VariadicDynCastAllOfMatcher
-: public llvm::VariadicFunction<
-BindableMatcher, Matcher,
-makeDynCastAllOfComposite > {
+: public VariadicFunction> {
 public:
   VariadicDynCastAllOfMatcher() {}
 };
@@ -1423,9 +1456,9 @@
 /// \c Matcher.
 /// The returned matcher matches if all given matchers match.
 template 
-class VariadicAllOfMatcher : public llvm::VariadicFunction<
-   BindableMatcher, Matcher,
-   makeAllOfComposite > {
+class VariadicAllOfMatcher
+: public VariadicFunction {
 public:
   VariadicAllOfMatcher() {}
 };
@@ -1546,8 +1579,8 @@
 new MatcherImpl(InnerMatcher, Getter::value()));
   }
 
-  struct Func : public llvm::VariadicFunction {
+  struct Func
+  : public VariadicFunction {
 Func() {}
   };
 
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- 

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

In http://reviews.llvm.org/D17811#380124, @jbcoe wrote:

> Do you have commit access? I can apply this patch for you if not.


I do.
I am waiting on http://reviews.llvm.org/D18275 to fix the problem with using 
internal::HasNameMatcher directly.


http://reviews.llvm.org/D17811



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

Re: [PATCH] D17986: [ASTMatchers] New matcher hasReturnValue added

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

In http://reviews.llvm.org/D17986#379271, @baloghadamsoftware wrote:

> I can rerun the script, however it seems it was not executed before the last 
> commit on the master branch, thus if I rerun it then changes will appear in 
> my diff which are not related to my work. What is the exect policy about 
> running this scipt? Should it be rerun before the commit (then it was 
> forgotten by the last committer) or it is executed only before releases?


I don't think there is a specific policy here. We try to keep it up to date, 
but some changes forget to do it.
We can do it in a separate patch if you feel the extra diff should not be on 
this one.


http://reviews.llvm.org/D17986



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

[clang-tools-extra] r263963 - [clang-tidy] Fix check broken in rL263822.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Mon Mar 21 13:00:43 2016
New Revision: 263963

URL: http://llvm.org/viewvc/llvm-project?rev=263963=rev
Log:
[clang-tidy] Fix check broken in rL263822.

Add names missing from rL263822 and add tests to prevent future omissions.

Modified:
clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp

Modified: clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp?rev=263963=263962=263963=diff
==
--- clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp Mon 
Mar 21 13:00:43 2016
@@ -38,7 +38,8 @@ void InefficientAlgorithmCheck::register
  "::std::lower_bound", "::std::upper_bound");
   const auto ContainerMatcher = classTemplateSpecializationDecl(hasAnyName(
   "::std::set", "::std::map", "::std::multiset", "::std::multimap",
-  "::std::unordered_set", "::std::unordered_map"));
+  "::std::unordered_set", "::std::unordered_map",
+  "::std::unordered_multiset", "::std::unordered_multimap"));
 
   const auto Matcher =
   callExpr(

Modified: clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp?rev=263963=263962=263963=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp 
(original)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-inefficient-algorithm.cpp Mon 
Mar 21 13:00:43 2016
@@ -35,7 +35,11 @@ template  struct multimap : map {};
 template  struct unordered_set : set {};
+template  struct unordered_map : map {};
+template  struct unordered_multiset : set {};
+template  struct unordered_multimap : map {};
 
 template > struct multiset : set {};
 
@@ -114,10 +118,30 @@ int main() {
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be
   // CHECK-FIXES: {{^  }}us.find(10);{{$}}
 
+  std::unordered_multiset ums;
+  find(ums.begin(), ums.end(), 10);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be
+  // CHECK-FIXES: {{^  }}ums.find(10);{{$}}
+
   std::map intmap;
   find(intmap.begin(), intmap.end(), 46);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be
   // CHECK-FIXES: {{^  }}find(intmap.begin(), intmap.end(), 46);{{$}}
+
+  std::multimap intmmap;
+  find(intmmap.begin(), intmmap.end(), 46);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be
+  // CHECK-FIXES: {{^  }}find(intmmap.begin(), intmmap.end(), 46);{{$}}
+
+  std::unordered_map umap;
+  find(umap.begin(), umap.end(), 46);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be
+  // CHECK-FIXES: {{^  }}find(umap.begin(), umap.end(), 46);{{$}}
+
+  std::unordered_multimap ummap;
+  find(ummap.begin(), ummap.end(), 46);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this STL algorithm call should be
+  // CHECK-FIXES: {{^  }}find(ummap.begin(), ummap.end(), 46);{{$}}
 }
 
 struct Value {


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

Re: [PATCH] D17986: [ASTMatchers] New matcher hasReturnValue added

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Can you rerun the doc script (dump_ast_matchers.py)?


http://reviews.llvm.org/D17986



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

[clang-tools-extra] r263822 - [clang-tidy] Use hasAnyName() instead of matchesName().

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Fri Mar 18 15:14:35 2016
New Revision: 263822

URL: http://llvm.org/viewvc/llvm-project?rev=263822=rev
Log:
[clang-tidy] Use hasAnyName() instead of matchesName().

matchesName() uses regular expressions and it is very slow.
hasAnyName() gives the same result and it is much faster.
A benchmark (with all the checks enabled) shows a ~5% speed up of
clang-tidy.

Modified:
clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp

Modified: clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp?rev=263822=263821=263822=diff
==
--- clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/misc/InefficientAlgorithmCheck.cpp Fri 
Mar 18 15:14:35 2016
@@ -33,13 +33,16 @@ void InefficientAlgorithmCheck::register
   if (!getLangOpts().CPlusPlus)
 return;
 
-  const std::string Algorithms =
-  "^::std::(find|count|equal_range|lower_bound|upper_bound)$";
-  const auto ContainerMatcher = classTemplateSpecializationDecl(
-  matchesName("^::std::(unordered_)?(multi)?(set|map)$"));
+  const auto Algorithms =
+  hasAnyName("::std::find", "::std::count", "::std::equal_range",
+ "::std::lower_bound", "::std::upper_bound");
+  const auto ContainerMatcher = classTemplateSpecializationDecl(hasAnyName(
+  "::std::set", "::std::map", "::std::multiset", "::std::multimap",
+  "::std::unordered_set", "::std::unordered_map"));
+
   const auto Matcher =
   callExpr(
-  callee(functionDecl(matchesName(Algorithms))),
+  callee(functionDecl(Algorithms)),
   hasArgument(
   0, cxxConstructExpr(has(cxxMemberCallExpr(
  callee(cxxMethodDecl(hasName("begin"))),


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

Re: [PATCH] D18243: [ASTMatchers] Existing matcher hasAnyArgument fixed

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Please add a test for this.


http://reviews.llvm.org/D18243



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

Re: [PATCH] D17986: [ASTMatchers] New matcher hasReturnValue added

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: include/clang/ASTMatchers/ASTMatchers.h:4848
@@ +4847,3 @@
+/// \code
+///   return a+b;
+/// \endcode

`a + b` (with spaces)
A few in this comment, and one in the test.


Comment at: include/clang/ASTMatchers/ASTMatchers.h:4854
@@ +4853,3 @@
+///   matching 'a+b'
+AST_MATCHER_P(ReturnStmt, hasReturnValue, internal::Matcher, 
InnerMatcher) {
+  BoundNodesTreeBuilder Result(*Builder);

Wouldn't this just be:

AST_MATCHER_P(...) {
  return InnerMatcher.matches(*Node.getRetValue(), Finder, Builder);
}


http://reviews.llvm.org/D17986



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

Re: [PATCH] D18275: [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

Alex, this is what we discussed to make `hasAnyName` take a `vector` 
directly.


http://reviews.llvm.org/D18275



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

[PATCH] D18275: [ASTMatchers] Add own version of VariadicFunction.

[Samuel Benzaquen via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

llvm::VariadicFunction is only being used by ASTMatchers.
Having own copy here allows us to remove the other one from llvm/ADT.
Also, we can extend the API to our needs without modifying the common
implementation.

http://reviews.llvm.org/D18275

Files:
  include/clang/ASTMatchers/ASTMatchers.h
  include/clang/ASTMatchers/ASTMatchersInternal.h
  lib/ASTMatchers/Dynamic/Marshallers.h
  unittests/ASTMatchers/ASTMatchersTest.cpp

Index: unittests/ASTMatchers/ASTMatchersTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTest.cpp
@@ -2892,6 +2892,9 @@
   EXPECT_TRUE(notMatches(Code, recordDecl(hasAnyName("::C", "::b::C";
   EXPECT_TRUE(
   matches(Code, recordDecl(hasAnyName("::C", "::b::C", "::a::b::C";
+
+  std::vector Names = {"::C", "::b::C", "::a::b::C"};
+  EXPECT_TRUE(matches(Code, recordDecl(hasAnyName(Names;
 }
 
 TEST(Matcher, IsDefinition) {
Index: lib/ASTMatchers/Dynamic/Marshallers.h
===
--- lib/ASTMatchers/Dynamic/Marshallers.h
+++ lib/ASTMatchers/Dynamic/Marshallers.h
@@ -325,8 +325,9 @@
 
   template )>
-  VariadicFuncMatcherDescriptor(llvm::VariadicFunction Func,
-  StringRef MatcherName)
+  VariadicFuncMatcherDescriptor(
+  ast_matchers::internal::VariadicFunction Func,
+  StringRef MatcherName)
   : Func(),
 MatcherName(MatcherName.str()),
 ArgsKind(ArgTypeTraits::getKind()) {
@@ -655,9 +656,9 @@
 /// \brief Variadic overload.
 template )>
-MatcherDescriptor *
-makeMatcherAutoMarshall(llvm::VariadicFunction VarFunc,
-StringRef MatcherName) {
+MatcherDescriptor *makeMatcherAutoMarshall(
+ast_matchers::internal::VariadicFunction VarFunc,
+StringRef MatcherName) {
   return new VariadicFuncMatcherDescriptor(VarFunc, MatcherName);
 }
 
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -47,7 +47,6 @@
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/Type.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/ADT/VariadicFunction.h"
 #include "llvm/Support/ManagedStatic.h"
 #include 
 #include 
@@ -60,6 +59,39 @@
 
 namespace internal {
 
+/// \brief Variadic function object.
+///
+/// Most of the functions below that use VariadicFunction could be implemented
+/// using plain C++11 variadic functions, but the function object allows us to
+/// capture it on the dynamic matcher registry.
+template )>
+struct VariadicFunction {
+  ResultT operator()() const { return Func({}); }
+
+  template 
+  ResultT operator()(const ArgT , const ArgsT &... Args) const {
+return Execute(Arg1, static_cast(Args)...);
+  }
+
+  // We also allow calls with an already created array, in case the caller
+  // already had it.
+  ResultT operator()(ArrayRef Args) const {
+std::vector InnerArgs;
+for (const ArgT  : Args)
+  InnerArgs.push_back();
+return Func(InnerArgs);
+  }
+
+private:
+  // Trampoline function to allow for implicit conversions to take place
+  // before we make the array.
+  template  ResultT Execute(const ArgsT &... Args) const {
+const ArgT *const ArgsArray[] = {};
+return Func(ArgsArray);
+  }
+};
+
 /// \brief Unifies obtaining the underlying type of a regular node through
 /// `getType` and a TypedefNameDecl node through `getUnderlyingType`.
 template 
@@ -1408,9 +1440,8 @@
 /// casted to CXXRecordDecl and all given matchers match.
 template 
 class VariadicDynCastAllOfMatcher
-: public llvm::VariadicFunction<
-BindableMatcher, Matcher,
-makeDynCastAllOfComposite > {
+: public VariadicFunction> {
 public:
   VariadicDynCastAllOfMatcher() {}
 };
@@ -1426,9 +1457,9 @@
 /// \c Matcher.
 /// The returned matcher matches if all given matchers match.
 template 
-class VariadicAllOfMatcher : public llvm::VariadicFunction<
-   BindableMatcher, Matcher,
-   makeAllOfComposite > {
+class VariadicAllOfMatcher
+: public VariadicFunction {
 public:
   VariadicAllOfMatcher() {}
 };
@@ -1549,8 +1580,8 @@
 new MatcherImpl(InnerMatcher, Getter::value()));
   }
 
-  struct Func : public llvm::VariadicFunction {
+  struct Func
+  : public 

Re: [PATCH] D17986: [ASTMatchers] Existing matcher hasAnyArgument fixed and new matcher hasReturnValue added

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

> I will separate it, OK. In the Clang there is one use case that I fixed, 
> although it did not break the tests. Neither of the other "has..." checkers 
> (except the general ones) ignore implicit casts and parenthesized expressions 
> so this one should not do it either because it makes checking implicit casts 
> impossible.


I agree that we want it, just wanted to point out that it has to be done with 
care.

> Matcher "hasReturnValue" is needed because "has" ignores all implicit casts 
> and parenthesized expressions and we need to the check implicit casts. I will 
> add a test and add it to the dynamic registry.


I see. has() suffers from the same problem. Then it makes sense.


http://reviews.llvm.org/D17986



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

Re: [PATCH] D17986: [ASTMatchers] Existing matcher hasAnyArgument fixed and new matcher hasReturnValue added

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

The reason we haven't fixed hasAnyArgument is that it can potentially break its 
users.
I'd prefer if you separated the fix from the addition.
That way we can revert the fix if needed.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:4796
@@ +4795,3 @@
+/// \endcode
+/// hasReturnValue(binaryOperator())
+///   matches 'return a+b'

New matchers must be tested.
See ASTMatchersTest.cpp.

Also, they should be added to the dynamic registry.
See Dynamic/Registry.cpp


Comment at: include/clang/ASTMatchers/ASTMatchers.h:4800
@@ +4799,3 @@
+///   matching 'a+b'
+AST_MATCHER_P(ReturnStmt, hasReturnValue, internal::Matcher, 
InnerMatcher) {
+  BoundNodesTreeBuilder Result(*Builder);

I'm not sure this is needed.
`returnStmt(hasReturnValue(...))` is equivalent to `returnStmt(has(...))`
There are many checks using the latter already.


http://reviews.llvm.org/D17986



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

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

sbenza updated this revision to Diff 49998.
sbenza marked 3 inline comments as done.
sbenza added a comment.

Minor fixes


http://reviews.llvm.org/D17811

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/DanglingHandleCheck.cpp
  clang-tidy/misc/DanglingHandleCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-dangling-handle.rst
  test/clang-tidy/misc-dangling-handle.cpp

Index: test/clang-tidy/misc-dangling-handle.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-dangling-handle.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s misc-dangling-handle %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: misc-dangling-handle.HandleClasses, \
+// RUN:   value: 'std::basic_string_view; ::llvm::StringRef;'}]}" \
+// RUN:   -- -std=c++11
+
+namespace std {
+
+template 
+class vector {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+  using size_type = int;
+
+  void assign(size_type count, const T& value);
+  iterator insert(const_iterator pos, const T& value);
+  iterator insert(const_iterator pos, T&& value);
+  iterator insert(const_iterator pos, size_type count, const T& value);
+  void push_back(const T&);
+  void push_back(T&&);
+  void resize(size_type count, const T& value);
+};
+
+template 
+class pair {};
+
+template 
+class set {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+
+  std::pair insert(const T& value);
+  std::pair insert(T&& value);
+  iterator insert(const_iterator hint, const T& value);
+  iterator insert(const_iterator hint, T&& value);
+};
+
+template 
+class map {
+ public:
+  using value_type = pair;
+  value_type& operator[](const Key& key);
+  value_type& operator[](Key&& key);
+};
+
+class basic_string {
+ public:
+  basic_string();
+  basic_string(const char*);
+  ~basic_string();
+};
+
+typedef basic_string string;
+
+class basic_string_view {
+ public:
+  basic_string_view(const char*);
+  basic_string_view(const basic_string&);
+};
+
+typedef basic_string_view string_view;
+
+}  // namespace std
+
+namespace llvm {
+
+class StringRef {
+ public:
+  StringRef();
+  StringRef(const char*);
+  StringRef(const std::string&);
+};
+
+}  // namespace llvm
+
+std::string ReturnsAString();
+
+void Positives() {
+  std::string_view view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives its value [misc-dangling-handle]
+
+  std::string_view view_2 = ReturnsAString();
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+
+  view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
+
+  const std::string& str_ref = "";
+  std::string_view view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+  view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: std::basic_string_view outlives
+
+  std::string_view view4(ReturnsAString());
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+}
+
+void OtherTypes() {
+  llvm::StringRef ref = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: llvm::StringRef outlives its value
+}
+
+const char static_array[] = "A";
+std::string_view ReturnStatements(int i, std::string value_arg,
+  const std::string _arg) {
+  const char array[] = "A";
+  const char* ptr = "A";
+  std::string s;
+  static std::string ss;
+  switch (i) {
+// Bad cases
+case 0:
+  return array;  // refers to local
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 1:
+  return s;  // refers to local
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 2:
+  return std::string();  // refers to temporary
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 3:
+  return value_arg;  // refers to by-value arg
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+
+// Ok cases
+case 100:
+  return ss;  // refers to static
+case 101:
+  return static_array;  // refers to static
+case 102:
+  return ptr;  // pointer is ok
+case 103:
+  return ref_arg;  // refers to by-ref arg
+  }
+
+  struct S {
+std::string_view view() { return value; }
+std::string value;
+  };
+
+  (void)[&]()->std::string_view {
+// This should not warn. The string is bound by reference.
+return s;
+  };
+  (void)[=]() -> std::string_view {
+// This should not warn. The reference is valid as long as the lambda.
+return s;
+  };
+  (void)[=]() -> std::string_view {
+// FIXME: This one should warn. We are returning a reference to a local
+// lambda variable.
+std::string 

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:24
@@ +23,3 @@
+
+std::vector parseClasses(StringRef Option) {
+  SmallVector Classes;

alexfh wrote:
> A very similar code has been added recently to 
> clang-tidy/utils/HeaderFileExtensionsUtils.*. Maybe make that code generic 
> enough and reuse it?
It is not the same.
That one is filtering the characters using isAlphanumeric.
Type names can have non-alpha chars as part of template instantiations.

On the other hand, it is a copy of the one from FasterStringFindCheck.
We could move it to a central location.


Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:57
@@ +56,3 @@
+ast_matchers::internal::Matcher isASequence() {
+  return hasAnyName("::std::deque", "::std::forward_list", "::std::list",
+"::std::vector");

jbcoe wrote:
> Will this (and similar checkers) handle inline namespaces?
Yes. hasName() and hasAnyName() have been fixed to look through inline 
namespaces.


Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:116
@@ +115,3 @@
+  return cxxRecordDecl(internal::Matcher(
+   new internal::HasNameMatcher(HandleClasses)))
+  .bind("handle");

alexfh wrote:
> This use of internal classes doesn't look nice. Can you add a `hasAnyName` 
> overload taking a `set` or an `ArrayRef` of names (or maybe a template 
> version that'll take a range of something convertible to StringRef)?
Will do.


Comment at: test/clang-tidy/misc-dangling-handle.cpp:73
@@ +72,3 @@
+  StringRef(const char*);  // NOLINT
+  StringRef(const std::string&);  // NOLINT
+};

jbcoe wrote:
> What does `NOLINT` do here?
Removed. Not needed here.


Comment at: test/clang-tidy/misc-dangling-handle.cpp:85
@@ +84,3 @@
+  std::string_view view_2 = ReturnsAString();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: std::basic_string_view outlives
+

jbcoe wrote:
> This (and other) check-messages line looks truncated.
They are truncated on purpose.
We use the full message in the first one to check the message.
The rest are truncated to fit in 80 chars.


http://reviews.llvm.org/D17811



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

Re: [PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

In http://reviews.llvm.org/D17811#366482, @Eugene.Zelenko wrote:

> Does it make http://reviews.llvm.org/D17772 obsolete?


Yes. The other patch has already been abandoned.


http://reviews.llvm.org/D17811



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

[PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Samuel Benzaquen via cfe-commits]

sbenza created this revision.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.

Add check misc-dangling-handle to detect dangling references in value
handlers like std::experimental::string_view.
It provides a configuration option to specify other handle types that
should also be checked.

Right now it detects:
 - Construction from temporaries.
 - Assignment from temporaries.
 - Return statements from temporaries or locals.
 - Insertion into containers from temporaries.

http://reviews.llvm.org/D17811

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/DanglingHandleCheck.cpp
  clang-tidy/misc/DanglingHandleCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-dangling-handle.rst
  test/clang-tidy/misc-dangling-handle.cpp

Index: test/clang-tidy/misc-dangling-handle.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-dangling-handle.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s misc-dangling-handle %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: misc-dangling-handle.HandleClasses, \
+// RUN:   value: 'std::basic_string_view; ::llvm::StringRef;'}]}" \
+// RUN:   -- -std=c++11
+
+namespace std {
+
+template 
+class vector {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+  using size_type = int;
+
+  void assign(size_type count, const T& value);
+  iterator insert(const_iterator pos, const T& value);
+  iterator insert(const_iterator pos, T&& value);
+  iterator insert(const_iterator pos, size_type count, const T& value);
+  void push_back(const T&);
+  void push_back(T&&);
+  void resize(size_type count, const T& value);
+};
+
+template 
+class pair {};
+
+template 
+class set {
+ public:
+  using const_iterator = const T*;
+  using iterator = T*;
+
+  std::pair insert(const T& value);
+  std::pair insert(T&& value);
+  iterator insert(const_iterator hint, const T& value);
+  iterator insert(const_iterator hint, T&& value);
+};
+
+template 
+class map {
+ public:
+  using value_type = pair;
+  value_type& operator[](const Key& key);
+  value_type& operator[](Key&& key);
+};
+
+class basic_string {
+ public:
+  basic_string();
+  basic_string(const char*);  // NOLINT
+  ~basic_string();
+};
+
+typedef basic_string string;
+
+class basic_string_view {
+ public:
+  basic_string_view(const char*);
+  basic_string_view(const basic_string&);
+};
+
+typedef basic_string_view string_view;
+
+}  // namespace std
+
+namespace llvm {
+
+class StringRef {
+ public:
+  StringRef();
+  StringRef(const char*);  // NOLINT
+  StringRef(const std::string&);  // NOLINT
+};
+
+}  // namespace llvm
+
+std::string ReturnsAString();
+
+void Positives() {
+  std::string_view view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: std::basic_string_view outlives its value [misc-dangling-handle]
+
+  std::string_view view_2 = ReturnsAString();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: std::basic_string_view outlives
+
+  view1 = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: std::basic_string_view outlives
+
+  const std::string& str_ref = "";
+  std::string_view view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: std::basic_string_view outlives
+  view3 = true ? "A" : str_ref;
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: std::basic_string_view outlives
+
+  std::string_view view4(ReturnsAString());
+  // CHECK-MESSAGES: [[@LINE-1]]:20: warning: std::basic_string_view outlives
+}
+
+void OtherTypes() {
+  llvm::StringRef ref = std::string();
+  // CHECK-MESSAGES: [[@LINE-1]]:25: warning: llvm::StringRef outlives its value
+}
+
+const char static_array[] = "A";
+std::string_view ReturnStatements(int i, std::string value_arg,
+  const std::string _arg) {
+  const char array[] = "A";
+  const char* ptr = "A";
+  std::string s;
+  static std::string ss;
+  switch (i) {
+// Bad cases
+case 0:
+  return array;  // refers to local
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 1:
+  return s;  // refers to local
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 2:
+  return std::string();  // refers to temporary
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+case 3:
+  return value_arg;  // refers to by-value arg
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: std::basic_string_view outliv
+
+// Ok cases
+case 100:
+  return ss;  // refers to static
+case 101:
+  return static_array;  // refers to static
+case 102:
+  return ptr;  // pointer is ok
+case 103:
+  return ref_arg;  // refers to by-ref arg
+  }
+
+  struct S {
+std::string_view view() { return value; }
+std::string value;
+  };

Re: [PATCH] D17575: Determine if there's a getDecl member function using less hacks

[Samuel Benzaquen via cfe-commits]

sbenza added a comment.

I assume you checked that the new trait works on MSVC.
Aren't both the same type of expression SFINAE? Is somehow one supported but 
not the other?


http://reviews.llvm.org/D17575



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

Re: r261574 - [ASTMatchers] Add matcher hasAnyName.

[Samuel Benzaquen via cfe-commits]

On Mon, Feb 22, 2016 at 5:27 PM, Hans Wennborg <h...@chromium.org> wrote:

> On Mon, Feb 22, 2016 at 1:13 PM, Samuel Benzaquen via cfe-commits
> <cfe-commits@lists.llvm.org> wrote:
> > Author: sbenza
> > Date: Mon Feb 22 15:13:02 2016
> > New Revision: 261574
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=261574=rev
> > Log:
> > [ASTMatchers] Add matcher hasAnyName.
> >
> > Summary: Add matcher hasAnyName as an optimization over
> anyOf(hasName(),...)
> >
> > Reviewers: alexfh
> >
> > Subscribers: klimek, cfe-commits
> >
> > Differential Revision: http://reviews.llvm.org/D17163
>
> I've tried to fix the Visual Studio build in r261583. Please take a
> look to make sure the fixes seem reasonable.
>

Sorry about that. I always forget that we can't use brace init lists in
LLVM yet.
Yes. That fix is correct. It would be better to move the vector to avoid an
extra copy.


>
>
> > +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Mon Feb 22
> 15:13:02 2016
> > @@ -1844,11 +1844,24 @@ inline internal::Matcher sizeOfExp
> >  /// \code
> >  ///   namespace a { namespace b { class X; } }
> >  /// \endcode
> > -inline internal::Matcher hasName(std::string Name) {
> > -  return internal::Matcher(
> > -  new internal::HasNameMatcher(std::move(Name)));
> > +inline internal::Matcher hasName(const std::string ) {
> > +  return internal::Matcher(new
> internal::HasNameMatcher({Name}));
>
> That doesn't compile with Visual Studio 2013:
>
>
> C:\b\build\slave\ClangToTWin\build\src\third_party\llvm\tools\clang\include\clang/ASTMatchers/ASTMatchers.h(1848)
> : error C2440: 'initializing' : cannot convert from 'initializer-list'
> to 'clang::ast_matchers::internal::HasNameMatcher'
> Constructor for class
> 'clang::ast_matchers::internal::HasNameMatcher' is declared 'explicit'
>
> > +private:
> > +  struct Pattern {
> > +StringRef Pattern;
>
>
> C:\b\build\slave\ClangToTWin\build\src\third_party\llvm\tools\clang\lib\ASTMatchers\ASTMatchersInternal.cpp(394)
> : error C2380: type(s) preceding 'Pattern' (constructor with return
> type, or illegal redefinition of current class-name?)
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r261574 - [ASTMatchers] Add matcher hasAnyName.

[Samuel Benzaquen via cfe-commits]

On Mon, Feb 22, 2016 at 4:44 PM, Aaron Ballman <aa...@aaronballman.com>
wrote:

> On Mon, Feb 22, 2016 at 4:43 PM, Samuel Benzaquen <sbe...@google.com>
> wrote:
> >
> > On Mon, Feb 22, 2016 at 4:19 PM, Aaron Ballman <aa...@aaronballman.com>
> > wrote:
> >>
> >> On Mon, Feb 22, 2016 at 4:13 PM, Samuel Benzaquen via cfe-commits
> >> <cfe-commits@lists.llvm.org> wrote:
> >> > Author: sbenza
> >> > Date: Mon Feb 22 15:13:02 2016
> >> > New Revision: 261574
> >> >
> >> > URL: http://llvm.org/viewvc/llvm-project?rev=261574=rev
> >> > Log:
> >> > [ASTMatchers] Add matcher hasAnyName.
> >> >
> >> > Summary: Add matcher hasAnyName as an optimization over
> >> > anyOf(hasName(),...)
> >>
> >> Does this mean we can get a clang-tidy check to convert
> >> anyOf(hasName(), ...) into hasAnyName()? ;-)
> >>
> >> ~Aaron
> >
> >
> > I would be simple, but I don't think it the cost/benefit is there. =)
> > I changed all the checks manually in 5 minutes.
> > I'll be sending that change soon.
>
> Haha, I was joking about the new check, but am really glad to hear
> we'll be using the new AST matcher right away. Thank you for working
> on this!
>
> ~Aaron
>

Matching the name of the nodes is a notable part of the CPU time of
clang-tidy.
This is why I spent time making it more efficient more than once already.

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

Re: r261574 - [ASTMatchers] Add matcher hasAnyName.

[Samuel Benzaquen via cfe-commits]

On Mon, Feb 22, 2016 at 4:19 PM, Aaron Ballman <aa...@aaronballman.com>
wrote:

> On Mon, Feb 22, 2016 at 4:13 PM, Samuel Benzaquen via cfe-commits
> <cfe-commits@lists.llvm.org> wrote:
> > Author: sbenza
> > Date: Mon Feb 22 15:13:02 2016
> > New Revision: 261574
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=261574=rev
> > Log:
> > [ASTMatchers] Add matcher hasAnyName.
> >
> > Summary: Add matcher hasAnyName as an optimization over
> anyOf(hasName(),...)
>
> Does this mean we can get a clang-tidy check to convert
> anyOf(hasName(), ...) into hasAnyName()? ;-)
>
> ~Aaron
>

I would be simple, but I don't think it the cost/benefit is there. =)
I changed all the checks manually in 5 minutes.
I'll be sending that change soon.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r261574 - [ASTMatchers] Add matcher hasAnyName.

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Mon Feb 22 15:13:02 2016
New Revision: 261574

URL: http://llvm.org/viewvc/llvm-project?rev=261574=rev
Log:
[ASTMatchers] Add matcher hasAnyName.

Summary: Add matcher hasAnyName as an optimization over anyOf(hasName(),...)

Reviewers: alexfh

Subscribers: klimek, cfe-commits

Differential Revision: http://reviews.llvm.org/D17163

Modified:
cfe/trunk/docs/LibASTMatchersReference.html
cfe/trunk/docs/tools/dump_ast_matchers.py
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=261574=261573=261574=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Mon Feb 22 15:13:02 2016
@@ -3102,6 +3102,16 @@ expr(nullPointerConstant())
 
 
 
+Matcherinternal::Matcherhttp://clang.llvm.org/doxygen/classclang_1_1NamedDecl.html;>NamedDeclhasAnyNameStringRef, ..., 
StringRef
+Matches NamedDecl nodes 
that have any of the specified names.
+
+This matcher is only provided as a performance optimization of hasName.
+hasAnyName(a, b, c)
+ is equivalent but faster than
+anyOf(hasName(a), hasName(b), hasName(c))
+
+
+
 Matcherinternal::Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtisInTemplateInstantiation
 Matches 
statements inside of a template instantiation.
 

Modified: cfe/trunk/docs/tools/dump_ast_matchers.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/tools/dump_ast_matchers.py?rev=261574=261573=261574=diff
==
--- cfe/trunk/docs/tools/dump_ast_matchers.py (original)
+++ cfe/trunk/docs/tools/dump_ast_matchers.py Mon Feb 22 15:13:02 2016
@@ -264,6 +264,16 @@ def act_on_decl(declaration, comment, al
   add_matcher('*', name, 'Matcher<*>', comment)
   return
 
+# Parse Variadic functions.
+m = re.match(
+r"""^.*llvm::VariadicFunction\s*<\s*([^,]+),\s*([^,]+),\s*[^>]+>\s*
+  ([a-zA-Z]*)\s*=\s*{.*};$""",
+declaration, flags=re.X)
+if m:
+  result, arg, name = m.groups()[:3]
+  add_matcher(result, name, '%s, ..., %s' % (arg, arg), comment)
+  return
+
 # Parse Variadic operator matchers.
 m = re.match(
 r"""^.*VariadicOperatorMatcherFunc\s*<\s*([^,]+),\s*([^\s>]+)\s*>\s*

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=261574=261573=261574=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Mon Feb 22 15:13:02 2016
@@ -1844,11 +1844,24 @@ inline internal::Matcher sizeOfExp
 /// \code
 ///   namespace a { namespace b { class X; } }
 /// \endcode
-inline internal::Matcher hasName(std::string Name) {
-  return internal::Matcher(
-  new internal::HasNameMatcher(std::move(Name)));
+inline internal::Matcher hasName(const std::string ) {
+  return internal::Matcher(new internal::HasNameMatcher({Name}));
 }
 
+/// \brief Matches NamedDecl nodes that have any of the specified names.
+///
+/// This matcher is only provided as a performance optimization of hasName.
+/// \code
+/// hasAnyName(a, b, c)
+/// \endcode
+///  is equivalent to, but faster than
+/// \code
+/// anyOf(hasName(a), hasName(b), hasName(c))
+/// \endcode
+const llvm::VariadicFunction
+hasAnyName = {};
+
 /// \brief Matches NamedDecl nodes whose fully qualified names contain
 /// a substring matched by the given RegExp.
 ///

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=261574=261573=261574=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Mon Feb 22 
15:13:02 2016
@@ -637,10 +637,10 @@ private:
 
 /// \brief Matches named declarations with a specific name.
 ///
-/// See \c hasName() in ASTMatchers.h for details.
+/// See \c hasName() and \c hasAnyName() in ASTMatchers.h for details.
 class HasNameMatcher : public SingleNodeMatcherInterface {
  public:
-  explicit HasNameMatcher(std::string Name);
+  explicit HasNameMatcher(std::vector Names);
 
   bool matchesNode(const NamedDecl ) const override;
 
@@ -667,9 

Re: [PATCH] D17163: [ASTMatchers] Add matcher hasAnyName.

[Samuel Benzaquen via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL261574: [ASTMatchers] Add matcher hasAnyName. (authored by 
sbenza).

Changed prior to commit:
  http://reviews.llvm.org/D17163?vs=47802=48720#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D17163

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/docs/tools/dump_ast_matchers.py
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
  cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
  cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

Index: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -14,6 +14,7 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersInternal.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ManagedStatic.h"
 
 namespace clang {
@@ -293,15 +294,26 @@
   return false;
 }
 
-HasNameMatcher::HasNameMatcher(std::string NameRef)
-: UseUnqualifiedMatch(NameRef.find("::") == NameRef.npos),
-  Name(std::move(NameRef)) {
-  assert(!Name.empty());
+Matcher hasAnyNameFunc(ArrayRef NameRefs) {
+  std::vector Names;
+  for (auto *Name : NameRefs)
+Names.emplace_back(*Name);
+  return internal::Matcher(
+  new internal::HasNameMatcher(std::move(Names)));
+}
+
+HasNameMatcher::HasNameMatcher(std::vector N)
+: UseUnqualifiedMatch(std::all_of(
+  N.begin(), N.end(),
+  [](StringRef Name) { return Name.find("::") == Name.npos; })),
+  Names(std::move(N)) {
+  for (StringRef Name : Names)
+assert(!Name.empty());
 }
 
 namespace {
 
-bool ConsumeNameSuffix(StringRef , StringRef Suffix) {
+bool consumeNameSuffix(StringRef , StringRef Suffix) {
   StringRef Name = FullName;
   if (!Name.endswith(Suffix))
 return false;
@@ -315,79 +327,127 @@
   return true;
 }
 
-bool ConsumeNodeName(StringRef , const NamedDecl ) {
+StringRef getNodeName(const NamedDecl , llvm::SmallString<128> ) {
   // Simple name.
   if (Node.getIdentifier())
-return ConsumeNameSuffix(Name, Node.getName());
+return Node.getName();
 
   if (Node.getDeclName()) {
 // Name needs to be constructed.
-llvm::SmallString<128> NodeName;
-llvm::raw_svector_ostream OS(NodeName);
+Scratch.clear();
+llvm::raw_svector_ostream OS(Scratch);
 Node.printName(OS);
-return ConsumeNameSuffix(Name, OS.str());
+return OS.str();
   }
 
-  return ConsumeNameSuffix(Name, "(anonymous)");
+  return "(anonymous)";
 }
 
+StringRef getNodeName(const RecordDecl , llvm::SmallString<128> ) {
+  if (Node.getIdentifier()) {
+return Node.getName();
+  }
+  Scratch.clear();
+  return ("(anonymous " + Node.getKindName() + ")").toStringRef(Scratch);
+}
+
+StringRef getNodeName(const NamespaceDecl ,
+  llvm::SmallString<128> ) {
+  return Node.isAnonymousNamespace() ? "(anonymous namespace)" : Node.getName();
+}
+
+
+class PatternSet {
+public:
+  PatternSet(ArrayRef Names) {
+for (StringRef Name : Names)
+  Patterns.push_back({Name, Name.startswith("::")});
+  }
+
+  /// Consumes the name suffix from each pattern in the set and removes the ones
+  /// that didn't match.
+  /// Return true if there are still any patterns left.
+  bool consumeNameSuffix(StringRef NodeName, bool CanSkip) {
+for (size_t I = 0; I < Patterns.size();) {
+  if (internal::consumeNameSuffix(Patterns[I].Pattern, NodeName) ||
+  CanSkip) {
+++I;
+  } else {
+Patterns.erase(Patterns.begin() + I);
+  }
+}
+return !Patterns.empty();
+  }
+
+  /// Check if any of the patterns are a match.
+  /// A match will be a pattern that was fully consumed, that also matches the
+  /// 'fully qualified' requirement.
+  bool foundMatch(bool AllowFullyQualified) const {
+for (auto& P: Patterns)
+  if (P.Pattern.empty() && (AllowFullyQualified || !P.IsFullyQualified))
+return true;
+return false;
+  }
+
+private:
+  struct Pattern {
+StringRef Pattern;
+bool IsFullyQualified;
+  };
+  llvm::SmallVector Patterns;
+};
+
 }  // namespace
 
 bool HasNameMatcher::matchesNodeUnqualified(const NamedDecl ) const {
   assert(UseUnqualifiedMatch);
-  StringRef NodeName = Name;
-  return ConsumeNodeName(NodeName, Node) && NodeName.empty();
+  llvm::SmallString<128> Scratch;
+  StringRef NodeName = getNodeName(Node, Scratch);
+  return std::any_of(Names.begin(), Names.end(), [&](StringRef Name) {
+return consumeNameSuffix(Name, NodeName) && Name.empty();
+  });
 }
 
 bool HasNameMatcher::matchesNodeFullFast(const NamedDecl ) const {
+  PatternSet Patterns(Names);
+  llvm::SmallString<128> Scratch;
+
   // This function is copied and adapted from 

Re: [PATCH] D17447: Add check for CERT ENV33-C

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/cert/CommandProcessorCheck.cpp:36
@@ +35,3 @@
+  // is not a security risk by itself.
+  if (Fn->getName() == "system" && E->getNumArgs() == 1 &&
+  E->getArg(0)->isNullPointerConstant(*Result.Context,

You could move this into the matcher.
It could use the brand new nullPointerConstant()

unless(callExpr(callee(functionDecl(hasName("::system"))),
   argumentCountIs(1), hasArgument(0, nullPointerConstant(

Seems simpler.


http://reviews.llvm.org/D17447



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

Re: [PATCH] D17447: Add check for CERT ENV33-C

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.


Comment at: clang-tidy/cert/CommandProcessorCheck.cpp:22
@@ +21,3 @@
+  Finder->addMatcher(
+  callExpr(callee(functionDecl(anyOf(hasName("system"), hasName("popen"),
+ hasName("_popen")))

Should we check that it is calling ::system and not any function called system?


Comment at: clang-tidy/cert/CommandProcessorCheck.h:19
@@ +18,3 @@
+
+/// Execution of a command processor is can lead to security vulnerabilities,
+/// and is generally not required. Instead, prefer to launch executables

typo: is can


http://reviews.llvm.org/D17447



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

[clang-tools-extra] r261102 - [clang-tidy] Match the type against the get() method we are calling,

[Samuel Benzaquen via cfe-commits]

Author: sbenza
Date: Wed Feb 17 10:13:14 2016
New Revision: 261102

URL: http://llvm.org/viewvc/llvm-project?rev=261102=rev
Log:
[clang-tidy] Match the type against the get() method we are calling,
instead of a get() method we find in the class.

The duck typed smart pointer class could have overloaded get() methods
and we should only skip the one that matches.

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

clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp?rev=261102=261101=261102=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp 
(original)
+++ 
clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp 
Wed Feb 17 10:13:14 2016
@@ -25,7 +25,9 @@ internal::Matcher callToGet(intern
pointsTo(decl(OnClass).bind("ptr_to_ptr"))
 .bind("smart_pointer")),
  unless(callee(memberExpr(hasObjectExpression(cxxThisExpr(),
- callee(cxxMethodDecl(hasName("get"
+ callee(cxxMethodDecl(
+ hasName("get"),
+ returns(qualType(pointsTo(type().bind("getType")))
   .bind("redundant_get");
 }
 
@@ -35,10 +37,8 @@ void registerMatchersForGetArrowStart(Ma
   recordDecl().bind("duck_typing"),
   has(cxxMethodDecl(hasName("operator->"),
 returns(qualType(pointsTo(type().bind("op->Type")),
-  has(cxxMethodDecl(hasName("operator*"),
-
returns(qualType(references(type().bind("op*Type")),
-  has(cxxMethodDecl(hasName("get"),
-returns(qualType(pointsTo(type().bind("getType")));
+  has(cxxMethodDecl(hasName("operator*"), returns(qualType(references(
+  type().bind("op*Type")));
 
   // Catch 'ptr.get()->Foo()'
   Finder->addMatcher(memberExpr(expr().bind("memberExpr"), isArrow(),

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp?rev=261102=261101=261102=diff
==
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp 
(original)
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-redundant-smartptr-get.cpp 
Wed Feb 17 10:13:14 2016
@@ -44,6 +44,14 @@ struct Fail2 {
   int& operator*();
 };
 
+struct PointerWithOverloadedGet {
+  int* get();
+  template 
+  T* get();
+  int* operator->();
+  int& operator*();
+};
+
 void Positive() {
   BarPtr u;
   // CHECK-FIXES: BarPtr u;
@@ -100,6 +108,11 @@ void Positive() {
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
   // CHECK-MESSAGES: nullptr != ss->get();
   // CHECK-FIXES: bb = nullptr != *ss;
+
+  i = *PointerWithOverloadedGet().get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: i = *PointerWithOverloadedGet().get();
+  // CHECK-FIXES: i = *PointerWithOverloadedGet();
 }
 
 void Negative() {
@@ -113,6 +126,8 @@ void Negative() {
 }
   };
 
+  long l = *PointerWithOverloadedGet().get();
+
   std::unique_ptr* u;
   u->get()->Do();
 


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