Re: [PATCH] D18649: [clang-tidy] cppcoreguidelines-interfaces-global-init

[Clement Courbet via cfe-commits]

courbet added a comment.

In http://reviews.llvm.org/D18649#389363, @alexfh wrote:

> Thank you for working on the new clang-tidy check!
>
> We usually recommend authors to run their checks on a large code base to 
> ensure it doesn't crash and doesn't generate obvious false positives. It 
> would be nice, if you could provide a quick summary of such a run (total 
> number of hits, number of what seems to be a false positive in a sample of 
> ~100).


The tool generated 20k positives on our codebase. On a sample of 100, there are:

- 8 instances of the same exact code structure that's just wrong: const string 
var = FLAGS_some_flag + "some_sufix";
- 8 false positives.
- 84 possible issues. (interestingly 6 of these are from premature use of 
variations of "extern char* empty_string;"

The false positives fall into 3 categories:

1. 3 variations of:

extern int i;
static const int* pi = &i;  // diag
// Then pi is dereferenced later, once i is intialized. 
Public example of this: 
https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

2. 3 variations of:

// .h
class A {

  static const int i = 42;

};
// .cc
int A::i;  // diag

3. 2 variations of:

// .h
class A {
static int i;
static int j;
};
// .cc
int A::i = 0;
int A::j = i;  // diag


http://reviews.llvm.org/D18649



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

[PATCH] D18752: Documented standard substitutions defined by lit

[guibufolo+l...@gmail.com via cfe-commits]

RedX2501 created this revision.
RedX2501 added reviewers: EricWF, MatzeB, echristo, jroelofs.
RedX2501 added a subscriber: cfe-commits.

http://reviews.llvm.org/D18752

Files:
  docs/CommandGuide/lit.rst

Index: docs/CommandGuide/lit.rst
===
--- docs/CommandGuide/lit.rst
+++ docs/CommandGuide/lit.rst
@@ -355,6 +355,32 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various macros that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t tmpbase + ".tmp"
+ %T tmpdir
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 


Index: docs/CommandGuide/lit.rst
===
--- docs/CommandGuide/lit.rst
+++ docs/CommandGuide/lit.rst
@@ -355,6 +355,32 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various macros that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t tmpbase + ".tmp"
+ %T tmpdir
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18752: Documented standard substitutions defined by lit

[Matthias Braun via cfe-commits]

MatzeB added a comment.

Most patterns and several of the ones coming from lit.local.cfg are explained 
in TestingGuide.rst as well. On the other hand it makes sense to explain the 
lit specific substitutions in the lit docu, maybe just add an additional link 
to the TestingGuide.rst to reference this section for additional lit flags?


http://reviews.llvm.org/D18752



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

Re: [PATCH] D16579: Warn if friend function depends on template parameters.

[Serge Pavlov via cfe-commits]

sepavloff updated this revision to Diff 52527.
sepavloff added a comment.

Updated patch

Added a check for presence of template declaration corresponding to the friend
function. Presence of similar functions but with specializations as argument
types is also checked now.

Made an atempt to make messages more clear to end user, not sure however if
they become better.


http://reviews.llvm.org/D16579

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/Sema.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaDecl.cpp
  test/CXX/drs/dr3xx.cpp
  test/CXX/drs/dr5xx.cpp
  test/SemaCXX/friend.cpp
  test/SemaCXX/overload-call.cpp

Index: test/SemaCXX/overload-call.cpp
===
--- test/SemaCXX/overload-call.cpp
+++ test/SemaCXX/overload-call.cpp
@@ -574,13 +574,15 @@
   // Ensure that overload resolution attempts to complete argument types when
   // performing ADL.
   template struct S {
-friend int f(const S&);
+friend int f(const S&);  // expected-warning{{friend declaration 'IncompleteArg::f' depends on template parameter but is not a template function}}
+ // expected-note@-1{{if this is intentional, add function declaration to suppress the warning, if not - make sure the function template has already been declared and use '<>'}}
   };
   extern S s;
   int k = f(s);
 
   template struct Op {
-friend bool operator==(const Op &, const Op &);
+friend bool operator==(const Op &, const Op &);  // expected-warning{{friend declaration 'IncompleteArg::operator==' depends on template parameter but is not a template function}}
+ // expected-note@-1{{if this is intentional, add function declaration to suppress the warning, if not - make sure the function template has already been declared and use '<>'}}
   };
   extern Op op;
   bool b = op == op;
Index: test/SemaCXX/friend.cpp
===
--- test/SemaCXX/friend.cpp
+++ test/SemaCXX/friend.cpp
@@ -363,3 +363,102 @@
   f_pr6954(5); // expected-error{{undeclared identifier 'f_pr6954'}}
 }
 
+
+template void pr23342_func(T x);
+template
+struct pr23342_C1 {
+  friend void pr23342_func<>(T x);
+  friend bool func(T x);  // expected-warning{{friend declaration 'func' depends on template parameter but is not a template function}}
+  // expected-note@-1{{if this is intentional, add function declaration to suppress the warning, if not - make sure the function template has already been declared and use '<>'}}
+  friend bool func2(int x);
+  template friend bool func3(T2 x);
+  friend T func4();  // expected-warning{{friend declaration 'func4' depends on template parameter but is not a template function}}
+ // expected-note@-1{{if this is intentional, add function declaration to suppress the warning, if not - make sure the function template has already been declared and use '<>'}}
+};
+
+namespace pr23342 {
+
+template
+struct C1 {
+  friend void pr23342_func<>(T x);
+  friend bool func(T x);  // expected-warning{{friend declaration 'pr23342::func' depends on template parameter but is not a template function}}
+  // expected-note@-1{{if this is intentional, add function declaration to suppress the warning, if not - make sure the function template has already been declared and use '<>'}}
+  friend bool func2(int x);
+  template friend bool func3(T2 x);
+  friend T func4();// expected-warning{{friend declaration 'pr23342::func4' depends on template parameter but is not a template function}}
+   // expected-note@-1{{if this is intentional, add function declaration to suppress the warning, if not - make sure the function template has already been declared and use '<>'}}
+};
+
+template 
+struct Arg {
+  friend bool operator==(const Arg& lhs, T rhs) {
+   return false;
+  }
+  friend bool operator!=(const Arg& lhs, T rhs);  // expected-warning{{friend declaration 'pr23342::operator!=' depends on template parameter but is not a template function}}
+   // expected-note@-1{{to befriend a template specialization, use '<>'}}
+};
+template 
+bool operator!=(const Arg& lhs, T rhs) {
+  return true;
+}
+bool foo() {
+  Arg arg;
+  return (arg == 42) || (arg != 42);
+}
+
+
+template class C0 {
+  friend void func0(C0 &);  // expected-warning{{friend declaration 'pr23342::func0' depends on template parameter but is not a template function}}
+   // expected-note@-1{{if this is intentional, add function declaration to suppress the warning, if not - make sure the function template has already been declared and use '<>'}}
+};
+
+template class C0a {
+  friend void func0a(C0a &);  // expected-warning{{friend declaration 'pr23342::func0a' depends on template parameter but is not 

Re: [PATCH] D18752: Documented standard substitutions defined by lit

[guibufolo+l...@gmail.com via cfe-commits]

RedX2501 updated this revision to Diff 52530.
RedX2501 added a comment.

Added reference to testing infrastructure document.


http://reviews.llvm.org/D18752

Files:
  docs/CommandGuide/lit.rst

Index: docs/CommandGuide/lit.rst
===
--- docs/CommandGuide/lit.rst
+++ docs/CommandGuide/lit.rst
@@ -355,6 +355,35 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various macros that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t tmpbase + ".tmp"
+ %T tmpdir
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
+More information on the testing infrastucture can be found in the
+:doc:`../TestingGuide`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 


Index: docs/CommandGuide/lit.rst
===
--- docs/CommandGuide/lit.rst
+++ docs/CommandGuide/lit.rst
@@ -355,6 +355,35 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various macros that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t tmpbase + ".tmp"
+ %T tmpdir
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
+More information on the testing infrastucture can be found in the
+:doc:`../TestingGuide`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18752: Documented standard substitutions defined by lit

[Matthias Braun via cfe-commits]

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

I'd probably go for a term like "pattern" instead of macro (there is no 
parameters or anything more complicated). The explanation for %t/%T is not 
helpful in the current form. Maybe something like 'temporary file name unique 
to the test'/'temporary directory unique to the test'.

Apart from that LGTM.


http://reviews.llvm.org/D18752



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

Re: [PATCH] D15031: CFG: Add CFGElement for automatic variables that leave the scope

[Aleksei Sidorin via cfe-commits]

a.sidorin added a subscriber: a.sidorin.
a.sidorin added a comment.

Matthias,

Could you take a look at http://reviews.llvm.org/D16403? It looks like a 
duplicating work but it should cover not Objective-C only.


http://reviews.llvm.org/D15031



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

Re: [PATCH] D18752: Documented standard substitutions defined by lit

[guibufolo+l...@gmail.com via cfe-commits]

RedX2501 updated this revision to Diff 52531.
RedX2501 added a comment.

I had a hard time myself figuring out what tmpbase and tmpdir are. I have used 
your wording.

Somebody else needs to commit this as I don't have any rights for it.


http://reviews.llvm.org/D18752

Files:
  docs/CommandGuide/lit.rst

Index: docs/CommandGuide/lit.rst
===
--- docs/CommandGuide/lit.rst
+++ docs/CommandGuide/lit.rst
@@ -355,6 +355,35 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various patterns that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t temporary file name unique to the test
+ %T temporary directory unique to the test
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
+More information on the testing infrastucture can be found in the
+:doc:`../TestingGuide`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 


Index: docs/CommandGuide/lit.rst
===
--- docs/CommandGuide/lit.rst
+++ docs/CommandGuide/lit.rst
@@ -355,6 +355,35 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various patterns that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t temporary file name unique to the test
+ %T temporary directory unique to the test
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
+More information on the testing infrastucture can be found in the
+:doc:`../TestingGuide`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18180: [clang-tidy] Add a check to detect static definitions in anonymous namespace.

[Haojian Wu via cfe-commits]

hokein updated this revision to Diff 52532.
hokein marked an inline comment as done.
hokein added a comment.

Address comments.


http://reviews.llvm.org/D18180

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
  docs/clang-tidy/checks/list.rst
  
docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
  test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp

Index: test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s readability-static-definition-in-anonymous-namespace %t
+
+#define STATIC static
+#define DEFINE_STATIC_VAR(x) static int x = 2
+
+namespace {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'c' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
+// CHECK-FIXES: {{^}}int c = 1;
+static const int d = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'd' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int d = 1;
+const static int e = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'e' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int e = 1;
+
+void f() {
+  int a = 1;
+  static int b = 1;
+}
+
+static int g() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'g' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}int g() {
+  return 1;
+}
+
+STATIC int h = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'h' is a static definition in anonymous namespace
+
+DEFINE_STATIC_VAR(i);
+
+} // namespace
+
+namespace N {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+static const int d = 1;
+const static int e = 1;
+
+} // namespace N
Index: docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-static-definition-in-anonymous-namespace
+
+readability-static-definition-in-anonymous-namespace
+
+
+Finds static function and variable definitions in anonymous namespace.
+
+Static is redundant in the current translation unit because anonymous namespace
+is only visible in current transform unit.
+
+.. code:: c++
+  namespace {
+static int a = 1; // Warning.
+static const b = 1; // Warning.
+  }
+
+The check will apply a fix by removing the redundant 'static' qualifier.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -101,4 +101,5 @@
readability-redundant-string-cstr
readability-redundant-string-init
readability-simplify-boolean-expr
+   readability-static-definition-in-anonymous-namespace
readability-uniqueptr-delete-release
Index: clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
===
--- /dev/null
+++ clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
@@ -0,0 +1,35 @@
+//===--- StaticDefinitionInAnonymousNamespaceCheck.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_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds static function and variable definitions in anonymous namespace.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.html
+class StaticDefinitionInAnonymousNamespaceCheck : public ClangTidyCheck {
+public:
+  StaticDefinitionInAnonymousNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_DE

Re: [PATCH] D18180: [clang-tidy] Add a check to detect static definitions in anonymous namespace.

[Haojian Wu via cfe-commits]

hokein updated this revision to Diff 52533.
hokein marked 3 inline comments as done.
hokein added a comment.

Remove dump code.


http://reviews.llvm.org/D18180

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
  docs/clang-tidy/checks/list.rst
  
docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
  test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp

Index: test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s readability-static-definition-in-anonymous-namespace %t
+
+#define STATIC static
+#define DEFINE_STATIC_VAR(x) static int x = 2
+
+namespace {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'c' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
+// CHECK-FIXES: {{^}}int c = 1;
+static const int d = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'd' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int d = 1;
+const static int e = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'e' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int e = 1;
+
+void f() {
+  int a = 1;
+  static int b = 1;
+}
+
+static int g() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'g' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}int g() {
+  return 1;
+}
+
+STATIC int h = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'h' is a static definition in anonymous namespace
+
+DEFINE_STATIC_VAR(i);
+
+} // namespace
+
+namespace N {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+static const int d = 1;
+const static int e = 1;
+
+} // namespace N
Index: docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-static-definition-in-anonymous-namespace
+
+readability-static-definition-in-anonymous-namespace
+
+
+Finds static function and variable definitions in anonymous namespace.
+
+Static is redundant in the current translation unit because anonymous namespace
+is only visible in current transform unit.
+
+.. code:: c++
+  namespace {
+static int a = 1; // Warning.
+static const b = 1; // Warning.
+  }
+
+The check will apply a fix by removing the redundant 'static' qualifier.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -101,4 +101,5 @@
readability-redundant-string-cstr
readability-redundant-string-init
readability-simplify-boolean-expr
+   readability-static-definition-in-anonymous-namespace
readability-uniqueptr-delete-release
Index: clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
===
--- /dev/null
+++ clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
@@ -0,0 +1,35 @@
+//===--- StaticDefinitionInAnonymousNamespaceCheck.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_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds static function and variable definitions in anonymous namespace.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.html
+class StaticDefinitionInAnonymousNamespaceCheck : public ClangTidyCheck {
+public:
+  StaticDefinitionInAnonymousNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_DE

Re: [PATCH] D16749: [OpenMP] Map clause codegeneration.

[Alexey Bataev via cfe-commits]

ABataev added inline comments.


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4393-4431
@@ +4392,41 @@
+/// retrieved from the provided map clause expression.
+DeclarationMapInfoEntry(const Expr *MCE, OpenMPMapClauseKind MapType,
+OpenMPMapClauseKind MapTypeModifier)
+: MapType(MapType), MapTypeModifier(MapTypeModifier) {
+  assert(MCE && "Invalid expression??");
+  while (true) {
+MCE = MCE->IgnoreParenImpCasts();
+
+if (auto *CurE = dyn_cast(MCE)) {
+  Components.push_back(
+  ComponentTy(CurE, cast(CurE->getDecl(;
+  break;
+}
+
+if (auto *CurE = dyn_cast(MCE)) {
+  auto *BaseE = CurE->getBase()->IgnoreParenImpCasts();
+
+  Components.push_back(
+  ComponentTy(CurE, cast(CurE->getMemberDecl(;
+  if (isa(BaseE))
+break;
+
+  MCE = BaseE;
+  continue;
+}
+
+if (auto *CurE = dyn_cast(MCE)) {
+  Components.push_back(ComponentTy(CurE, nullptr));
+  MCE = CurE->getBase()->IgnoreParenImpCasts();
+  continue;
+}
+
+if (auto *CurE = dyn_cast(MCE)) {
+  Components.push_back(ComponentTy(CurE, nullptr));
+  MCE = CurE->getBase()->IgnoreParenImpCasts();
+  continue;
+}
+
+llvm_unreachable("Invalid map clause expression!");
+  }
+}

I do recall that similar code already exists in Sema. Maybe you'd better to 
store info about top-level bases in map clause in Sema rather than copy the 
code between modules?


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4462
@@ +4461,3 @@
+if (const auto *OAE = dyn_cast(E)) {
+  auto BaseTy = OMPArraySectionExpr::getBaseOriginalType(
+OAE->getBase()->IgnoreParenImpCasts())

Replace 'auto' by 'QualType'. Also, I think all such deep digging into 
structure of expressions must reside in Sema rather than in codegen and info 
must be stored within OMPMapClause


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4493-4498
@@ +4492,8 @@
+
+  /// \brief Generate the address of the lower bound of the section defined by
+  /// expression \a E.
+  llvm::Value *getLowerBoundOfElement(const Expr *E) const {
+return CGF.EmitLValue(E).getPointer();
+  }
+
+  /// \brief Return the corresponding bits for a given map clause modifier. Add

Do we really need this? It is quite simple and has nothing to do with the name 
of the function


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4681-4684
@@ +4680,6 @@
+if (I->second->getType()->isAnyPointerType() && std::next(I) != CE) {
+  auto PtrAddr =
+  CGF.MakeNaturalAlignAddrLValue(BP, I->second->getType());
+  BP = CGF.EmitLoadOfLValue(PtrAddr, SourceLocation()).getScalarVal();
+
+  // We do not need to generate individual map information for the

CGF has EmitLoadOfPointer() function


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4699-4735
@@ +4698,39 @@
+
+// A final array section, is one whose length can't be proved to be 
one.
+auto IsFinalArraySection = [this](const Expr *E) -> bool {
+  auto *OASE = dyn_cast(E);
+
+  // It is not an array section and therefore not a unity-size one.
+  if (!OASE)
+return false;
+
+  // An array section with no colon always refer to a single element.
+  if (OASE->getColonLoc().isInvalid())
+return false;
+
+  auto *Length = OASE->getLength();
+
+  // If we don't have a length we have to check if the array has size 1
+  // for this dimension. Also, we should always expect a length if the
+  // base type is pointer.
+  if (!Length) {
+auto BaseQTy = OMPArraySectionExpr::getBaseOriginalType(
+   OASE->getBase()->IgnoreParenImpCasts())
+   .getCanonicalType();
+if (auto *ATy = dyn_cast(BaseQTy.getTypePtr()))
+  return ATy->getSize().getSExtValue() != 1;
+// If we don't have a constant dimension length, we have to 
consider
+// the current section as having any size, so it is not necessarily
+// unitary. If it happen to be unity size, that's user fault.
+return true;
+  }
+
+  // Check if the length evaluates to 1.
+  llvm::APSInt ConstLength;
+  if (!Length->EvaluateAsInt(ConstLength, CGF.getContext()))
+return true; // Can have more that size 1.
+
+  return ConstLength.getSExtValue() != 1;
+}(I->first);
+
+// Get information on whether the element is a pointer. Have to do a

Maybe it is better to extract it as a member function?


Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4700
@@ 

Re: [PATCH] D18180: [clang-tidy] Add a check to detect static definitions in anonymous namespace.

[Haojian Wu via cfe-commits]

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

doc update.


http://reviews.llvm.org/D18180

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
  docs/clang-tidy/checks/list.rst
  
docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
  test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp

Index: test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s readability-static-definition-in-anonymous-namespace %t
+
+#define STATIC static
+#define DEFINE_STATIC_VAR(x) static int x = 2
+
+namespace {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'c' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
+// CHECK-FIXES: {{^}}int c = 1;
+static const int d = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'd' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int d = 1;
+const static int e = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'e' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int e = 1;
+
+void f() {
+  int a = 1;
+  static int b = 1;
+}
+
+static int g() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'g' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}int g() {
+  return 1;
+}
+
+STATIC int h = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'h' is a static definition in anonymous namespace
+
+DEFINE_STATIC_VAR(i);
+
+} // namespace
+
+namespace N {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+static const int d = 1;
+const static int e = 1;
+
+} // namespace N
Index: docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-static-definition-in-anonymous-namespace
+
+readability-static-definition-in-anonymous-namespace
+
+
+Finds static function and variable definitions in anonymous namespace.
+
+Static is redundant in the current translation unit because anonymous namespace
+is only visible in the unit.
+
+.. code:: c++
+  namespace {
+static int a = 1; // Warning.
+static const b = 1; // Warning.
+  }
+
+The check will apply a fix by removing the redundant 'static' qualifier.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -101,4 +101,5 @@
readability-redundant-string-cstr
readability-redundant-string-init
readability-simplify-boolean-expr
+   readability-static-definition-in-anonymous-namespace
readability-uniqueptr-delete-release
Index: clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
===
--- /dev/null
+++ clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
@@ -0,0 +1,35 @@
+//===--- StaticDefinitionInAnonymousNamespaceCheck.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_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds static function and variable definitions in anonymous namespace.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.html
+class StaticDefinitionInAnonymousNamespaceCheck : public ClangTidyCheck {
+public:
+  StaticDefinitionInAnonymousNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
Index: clang-tidy/readabili

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

[Andy Gibbs via cfe-commits]

AndyG updated this revision to Diff 52535.
AndyG marked 5 inline comments as done.
AndyG added a comment.

Implemented comments.


http://reviews.llvm.org/D17149

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/feature_tests.c
  test/Preprocessor/invalid-__has_warning1.c
  test/Preprocessor/invalid-__has_warning2.c
  test/Preprocessor/warning_tests.c

Index: test/Preprocessor/warning_tests.c
===
--- test/Preprocessor/warning_tests.c
+++ test/Preprocessor/warning_tests.c
@@ -12,7 +12,7 @@
 #endif
 
 // expected-error@+2 {{expected string literal in '__has_warning'}}
-// expected-error@+1 {{expected value in expression}}
+// expected-error@+1 {{missing ')'}} expected-note@+1 {{match}}
 #if __has_warning(-Wfoo)
 #endif
 
@@ -22,8 +22,7 @@
 #warning Not a valid warning flag
 #endif
 
-// expected-error@+2 {{builtin warning check macro requires a parenthesized string}}
-// expected-error@+1 {{invalid token}}
+// expected-error@+1 {{missing '(' after '__has_warning'}}
 #if __has_warning "not valid"
 #endif
 
@@ -33,7 +32,7 @@
 
 #define MY_ALIAS "-Wparentheses"
 
-// expected-error@+1 2{{expected}}
+// expected-error@+1 {{expected}}
 #if __has_warning(MY_ALIAS)
 #error Alias expansion not allowed
 #endif
Index: test/Preprocessor/invalid-__has_warning2.c
===
--- test/Preprocessor/invalid-__has_warning2.c
+++ test/Preprocessor/invalid-__has_warning2.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -verify %s
 
 // These must be the last lines in this test.
-// expected-error@+1{{expected string literal}} expected-error@+1{{expected}}
+// expected-error@+1{{too few arguments}}
 int i = __has_warning();
Index: test/Preprocessor/invalid-__has_warning1.c
===
--- test/Preprocessor/invalid-__has_warning1.c
+++ test/Preprocessor/invalid-__has_warning1.c
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -verify %s
 
 // These must be the last lines in this test.
-// expected-error@+1{{expected string literal}} expected-error@+1 2{{expected}}
+// expected-error@+1{{unterminated}} expected-error@+1 2{{expected}}
 int i = __has_warning(
Index: test/Preprocessor/feature_tests.c
===
--- test/Preprocessor/feature_tests.c
+++ test/Preprocessor/feature_tests.c
@@ -55,8 +55,50 @@
 #endif
 
 #ifdef VERIFY
-// expected-error@+2 {{builtin feature check macro requires a parenthesized identifier}}
-// expected-error@+1 {{expected value in expression}}
+// expected-error@+1 {{builtin feature check macro requires a parenthesized identifier}}
 #if __has_feature('x')
 #endif
+
+// The following are not identifiers:
+_Static_assert(!__is_identifier("string"), "oops");
+_Static_assert(!__is_identifier('c'), "oops");
+_Static_assert(!__is_identifier(123), "oops");
+_Static_assert(!__is_identifier(int), "oops");
+
+// The following are:
+_Static_assert(__is_identifier(abc /* comment */), "oops");
+_Static_assert(__is_identifier /* comment */ (xyz), "oops");
+
+// expected-error@+1 {{too few arguments}}
+#if __is_identifier()
+#endif
+
+// expected-error@+1 {{too many arguments}}
+#if __is_identifier(,())
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc xyz) // expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc())   // expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc.)// expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{nested parentheses not permitted in '__is_identifier'}} 
+#if __is_identifier((abc))
+#endif
+
+// expected-error@+1 {{missing '(' after '__is_identifier'}} expected-error@+1 {{expected value}}
+#if __is_identifier
+#endif
+
+// expected-error@+1 {{unterminated}} expected-error@+1 {{expected value}}
+#if __is_identifier(
+#endif
+
 #endif
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1053,9 +1053,8 @@
 
 /// HasFeature - Return true if we recognize and implement the feature
 /// specified by the identifier as a standard language feature.
-static bool HasFeature(const Preprocessor &PP, const IdentifierInfo *II) {
+static bool HasFeature(const Preprocessor &PP, StringRef Feature) {
   const LangOptions &LangOpts = PP.getLangOpts();
-  StringRef Feature = II->getName();
 
   // Normalize the feature name, __foo__ becomes foo.
   if (Feature.startswith("__") && Feature.endswith("__") && Feature.size() >= 4)
@@ -1229,8 +1228,8 @@
 /// HasExtension - Return true if we recognize and implement the feature
 /// specified by the identifier, either as an extension or a standard language
 /// feature.
-static bool HasExtension(const P

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

[Andy Gibbs via cfe-commits]

AndyG added inline comments.


Comment at: lib/Lex/PPMacroExpansion.cpp:1456-1457
@@ +1455,4 @@
+
+// Parse next non-comment, non-annotation token.
+do PP.LexUnexpandedNonComment(Tok); while (Tok.isAnnotation());
+

rsmith wrote:
> If we get an annotation token here, we should reject it, not silently ignore 
> it. Also, we shouldn't see comment tokens here (we shouldn't be doing macro 
> expansion with comments enabled); you should call `LexUnexpandedToken` rather 
> than `LexUnexpandedNonComment`.
Ok, annotation tokens are not ignored, but drop down into `Op` and can be 
handled there.


Comment at: lib/Lex/PPMacroExpansion.cpp:1481-1484
@@ +1480,6 @@
+  auto Diag = PP.Diag(Tok.getLocation(), 
diag::err_pp_unexpected_after);
+  if (IdentifierInfo *LastII = LastTok.getIdentifierInfo())
+Diag << LastII;
+  else
+Diag << LastTok.getKind();
+  Diag << tok::l_paren << LastTok.getLocation();

rsmith wrote:
> The only way we can get here without already having a value or producing a 
> diagnostic is if this is the first token inside the parens. So this will 
> always say "unexpected '(' after '('".
> 
> I think it would be better to always `break` here after incrementing 
> `ParenDepth` (even when `!Result.hasValue()`), and let `Op` produce the 
> relevant diagnostic for this case.
I've handled this via a specific diagnostic message: "nested parentheses not 
permitted in %0" where %0 is the macro name.  I would prefer this to bringing 
the error checking into `Op` since this would complicate the implementation of 
`Op` needlessly IMHO.  `Op` should be as slimline as possible.


http://reviews.llvm.org/D17149



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

Re: [PATCH] D17933: Set MaxAtomicInlineWidth properly for i386, i486, and x86-64 cpus without cmpxchg16b.

[Daniel Sanders via cfe-commits]

dsanders added a subscriber: dsanders.


Comment at: test/Preprocessor/init.c:3295
@@ +3294,3 @@
+// MIPSN32BE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
+// MIPSN32BE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1
+// MIPSN32BE-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16

I've just noticed that we only check the '__GCC_*' macros for the 64-bit ABI's 
(N32/N64). I'm not sure why the 32-bit (O32) checks are missing.

The O32 cases are the same as N32 except that `__GCC_ATOMIC_LLONG_LOCK_FREE` is 
1 and `__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8` is not defined.
Could you add:
  // MIPS32LE: #define __GCC_ATOMIC_BOOL_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_CHAR_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_INT_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_LLONG_LOCK_FREE 1
  // MIPS32LE: #define __GCC_ATOMIC_LONG_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_POINTER_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_SHORT_LOCK_FREE 2
  // MIPS32LE: #define __GCC_ATOMIC_TEST_AND_SET_TRUEVAL 1
  // MIPS32LE: #define __GCC_ATOMIC_WCHAR_T_LOCK_FREE 2
  // MIPS32LE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
  // MIPS32LE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
  // MIPS32LE: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
  // MIPS32LE-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8
  // MIPS32LE-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16
and the same for the MIPS32BE case?


http://reviews.llvm.org/D17933



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

r265287 - [OPENMP 4.0] Support for 'inbranch|noinbranch' clauses in 'declare

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Mon Apr  4 05:12:15 2016
New Revision: 265287

URL: http://llvm.org/viewvc/llvm-project?rev=265287&view=rev
Log:
[OPENMP 4.0] Support for 'inbranch|noinbranch' clauses in 'declare
simd'.

Added parsing/semantic analysis for 'inbranch|notinbranch' clauses of
'#pragma omp declare simd' construct.

Modified:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Parse/ParseOpenMP.cpp
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/test/OpenMP/declare_simd_ast_print.c
cfe/trunk/test/OpenMP/declare_simd_ast_print.cpp
cfe/trunk/test/OpenMP/declare_simd_messages.cpp
cfe/trunk/test/OpenMP/dump.cpp

Modified: cfe/trunk/include/clang/Basic/Attr.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=265287&r1=265286&r2=265287&view=diff
==
--- cfe/trunk/include/clang/Basic/Attr.td (original)
+++ cfe/trunk/include/clang/Basic/Attr.td Mon Apr  4 05:12:15 2016
@@ -2265,8 +2265,14 @@ def OMPDeclareSimdDecl : Attr {
   let SemaHandler = 0;
   let HasCustomParsing = 1;
   let Documentation = [OMPDeclareSimdDocs];
+  let Args = [EnumArgument<"BranchState", "BranchStateTy",
+   ["", "inbranch", "notinbranch"],
+   ["BS_Undefined", "BS_Inbranch", "BS_Notinbranch"]>];
   let AdditionalMembers = [{
-  void printPrettyPragma(raw_ostream &OS, const PrintingPolicy &Policy) const 
{}
+void printPrettyPragma(raw_ostream & OS, const PrintingPolicy &Policy)
+const {
+  OS << ' ' << ConvertBranchStateTyToStr(getBranchState());
+}
   }];
 }
 

Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=265287&r1=265286&r2=265287&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Mon Apr  4 05:12:15 
2016
@@ -961,6 +961,8 @@ def err_omp_unknown_map_type_modifier :
   "incorrect map type modifier, expected 'always'">;
 def err_omp_map_type_missing : Error<
   "missing map type">;
+def err_omp_declare_simd_inbranch_notinbranch : Error<
+  "unexpected '%0' clause, '%1' is specified already">;
 
 // Pragma loop support.
 def err_pragma_loop_missing_argument : Error<

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=265287&r1=265286&r2=265287&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Apr  4 05:12:15 2016
@@ -8091,8 +8091,10 @@ public:
 
   /// \brief Called on well-formed '\#pragma omp declare simd' after parsing of
   /// the associated method/function.
-  DeclGroupPtrTy ActOnOpenMPDeclareSimdDirective(DeclGroupPtrTy DG,
- SourceLocation StartLoc);
+  DeclGroupPtrTy
+  ActOnOpenMPDeclareSimdDirective(DeclGroupPtrTy DG,
+  OMPDeclareSimdDeclAttr::BranchStateTy BS,
+  SourceRange SR);
 
   OMPClause *ActOnOpenMPSingleExprClause(OpenMPClauseKind Kind,
  Expr *Expr,

Modified: cfe/trunk/lib/Parse/ParseOpenMP.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseOpenMP.cpp?rev=265287&r1=265286&r2=265287&view=diff
==
--- cfe/trunk/lib/Parse/ParseOpenMP.cpp (original)
+++ cfe/trunk/lib/Parse/ParseOpenMP.cpp Mon Apr  4 05:12:15 2016
@@ -323,6 +323,37 @@ Parser::ParseOpenMPDeclareReductionDirec
  IsCorrect);
 }
 
+/// Parses clauses for 'declare simd' directive.
+///clause:
+///  'inbranch' | 'notinbranch'
+static void parseDeclareSimdClauses(Parser &P,
+OMPDeclareSimdDeclAttr::BranchStateTy &BS) 
{
+  SourceRange BSRange;
+  const Token &Tok = P.getCurToken();
+  while (Tok.isNot(tok::annot_pragma_openmp_end)) {
+if (Tok.isNot(tok::identifier))
+  break;
+OMPDeclareSimdDeclAttr::BranchStateTy Out;
+StringRef TokName = Tok.getIdentifierInfo()->getName();
+// Parse 'inranch|notinbranch' clauses.
+if (OMPDeclareSimdDeclAttr::ConvertStrToBranchStateTy(TokName, Out)) {
+  if (BS != OMPDeclareSimdDeclAttr::BS_Undefined && BS != Out) {
+P.Diag(Tok, diag::err_omp_declare_simd_inbranch_notinbranch)
+<< TokName << OMPDeclareSimdDeclAttr::ConvertBranchStateTyToStr(BS)
+<< BSRange;
+  }
+  BS = Out;
+  BSRange = SourceRange(Tok.getLocation(), Tok.getEndLoc());
+

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Robert Bolter via cfe-commits]

Rob updated this revision to Diff 52539.
Rob added a comment.

adding -std=c++11 and it works as expected. yay


http://reviews.llvm.org/D18396

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-use-override-ms.cpp
  test/clang-tidy/modernize-use-override.cpp

Index: test/clang-tidy/modernize-use-override.cpp
===
--- test/clang-tidy/modernize-use-override.cpp
+++ test/clang-tidy/modernize-use-override.cpp
@@ -21,6 +21,7 @@
   virtual void d2();
   virtual void e() = 0;
   virtual void f() = 0;
+  virtual void f2() const = 0;
   virtual void g() = 0;
 
   virtual void j() const;
@@ -74,7 +75,11 @@
 
   virtual void f()=0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
-  // CHECK-FIXES: {{^}}  void f()override =0;
+  // CHECK-FIXES: {{^}}  void f() override =0;
+
+  virtual void f2() const=0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f2() const override =0;
 
   virtual void g() ABSTRACT;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
Index: test/clang-tidy/modernize-use-override-ms.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-ms.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- -- -fms-extensions -std=c++11
+
+// This test is designed to test ms-extension __declspec(dllexport) attributes.
+#define EXPORT __declspec(dllexport)
+
+class Base {
+  virtual EXPORT void a();
+};
+
+class EXPORT InheritedBase {
+  virtual void a();
+};
+
+class Derived : public Base {
+  virtual EXPORT void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  EXPORT void a() override;
+};
+
+class EXPORT InheritedDerived : public InheritedBase {
+  virtual void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() override;
+};
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 
   This check selectively replaces string literals containing escaped
   characters with raw string literals.
+  
+- Improved ``modernize-use-override`` check
+
+  The fix-its placement around __declspec attributes on windows and other 
+  attributes is improved.
+  
 
 Clang-tidy changes from 3.7 to 3.8
 ^^
Index: clang-tidy/modernize/UseOverrideCheck.cpp
===
--- clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tidy/modernize/UseOverrideCheck.cpp
@@ -95,9 +95,9 @@
: "'override' is";
 StringRef Correct = HasFinal ? "'final'" : "'override'";
 
-Message =
-(llvm::Twine(Redundant) +
- " redundant since the function is already declared " + Correct).str();
+Message = (llvm::Twine(Redundant) +
+   " redundant since the function is already declared " + Correct)
+  .str();
   }
 
   DiagnosticBuilder Diag = diag(Method->getLocation(), Message);
@@ -118,21 +118,24 @@
   if (!HasFinal && !HasOverride) {
 SourceLocation InsertLoc;
 StringRef ReplacementText = "override ";
+SourceLocation MethodLoc = Method->getLocation();
 
 for (Token T : Tokens) {
-  if (T.is(tok::kw___attribute)) {
+  if (T.is(tok::kw___attribute) &&
+  !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) {
 InsertLoc = T.getLocation();
 break;
   }
 }
 
 if (Method->hasAttrs()) {
   for (const clang::Attr *A : Method->getAttrs()) {
-if (!A->isImplicit()) {
+if (!A->isImplicit() && !A->isInherited()) {
   SourceLocation Loc =
   Sources.getExpansionLoc(A->getRange().getBegin());
-  if (!InsertLoc.isValid() ||
-  Sources.isBeforeInTranslationUnit(Loc, InsertLoc))
+  if ((!InsertLoc.isValid() ||
+   Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) &&
+  !Sources.isBeforeInTranslationUnit(Loc, MethodLoc))
 InsertLoc = Loc;
 }
   }
@@ -163,6 +166,9 @@
 Tokens.back().is(tok::kw_delete)) &&
   GetText(Tokens[Tokens.size() - 2], Sources) == "=") {
 InsertLoc = Tokens[Tokens.size() - 2].getLocation();
+// Check if we need to insert a space.
+if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0)
+  ReplacementText = " override ";
   } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
 InsertLoc = Tokens.back().getLocation();
   }
___
cfe-

Re: [PATCH] D18649: [clang-tidy] cppcoreguidelines-interfaces-global-init

[Clement Courbet via cfe-commits]

courbet updated this revision to Diff 52541.
courbet marked 2 inline comments as done.
courbet added a comment.

cosmetics + more documentation


http://reviews.llvm.org/D18649

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp
  clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-interfaces-global-init.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-interfaces-global-init.cpp

Index: test/clang-tidy/cppcoreguidelines-interfaces-global-init.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-interfaces-global-init.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-interfaces-global-init %t
+
+constexpr int makesInt() { return 3; }
+constexpr int takesInt(int i) { return i + 1; }
+constexpr int takesIntPtr(int *i) { return *i; }
+
+extern int ExternGlobal;
+static int GlobalScopeBadInit1 = ExternGlobal;
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with non-const expression depending on static variable 'ExternGlobal'
+static int GlobalScopeBadInit2 = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with non-const expression depending on static variable 'ExternGlobal'
+static int GlobalScopeBadInit3 = takesIntPtr(&ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with non-const expression depending on static variable 'ExternGlobal'
+static int GlobalScopeBadInit4 = 3 * (ExternGlobal + 2);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with non-const expression depending on static variable 'ExternGlobal'
+
+namespace ns {
+static int NamespaceScope = makesInt();
+static int NamespaceScopeBadInit = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with non-const expression depending on static variable 'ExternGlobal'
+
+struct A {
+  static int ClassScope;
+  static int ClassScopeBadInit;
+};
+
+int A::ClassScopeBadInit = takesInt(ExternGlobal);
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: initializing static variable with non-const expression depending on static variable 'ExternGlobal'
+
+static int FromClassBadInit = takesInt(A::ClassScope);
+// CHECK-MESSAGES: [[@LINE-1]]:12: warning: initializing static variable with non-const expression depending on static variable 'ClassScope'
+} // namespace ns
+
+void f() {
+  // This is fine, it's executed after dynamic initialization occurs.
+  static int G = takesInt(ExternGlobal);
+}
+
+// Defined global variables are fine:
+static int GlobalScope = makesInt();
+static int GlobalScopeBadInit = takesInt(GlobalScope);
+static int GlobalScope2 = takesInt(ns::NamespaceScope);
+// Enums are fine.
+enum Enum { kEnumValue = 1 };
+static int GlobalScopeFromEnum = takesInt(kEnumValue);
+
+// Leave constexprs alone.
+extern constexpr int GlobalScopeConstexpr = makesInt();
+static constexpr int GlobalScopeConstexprOk =
+takesInt(GlobalScopeConstexpr);
+
+// This is a pretty common instance: People are usually not using constexpr, but
+// this is what they should write:
+static constexpr const char kValue[] = "value";
+constexpr int Fingerprint(const char *value) { return 0; }
+static int kFingerprint = Fingerprint(kValue);
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -16,6 +16,7 @@
cert-fio38-c (redirects to misc-non-copyable-objects) 
cert-flp30-c
cert-oop11-cpp (redirects to misc-move-constructor-init) 
+   cppcoreguidelines-interfaces-global-init
cppcoreguidelines-pro-bounds-array-to-pointer-decay
cppcoreguidelines-pro-bounds-constant-array-index
cppcoreguidelines-pro-bounds-pointer-arithmetic
Index: docs/clang-tidy/checks/cppcoreguidelines-interfaces-global-init.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-interfaces-global-init.rst
@@ -0,0 +1,14 @@
+.. title:: clang-tidy - cppcoreguidelines-interfaces-global-init
+
+cppcoreguidelines-interfaces-global-init
+
+
+This check flags initializers of globals that access extern objects,
+and therefore can lead to order-of-initialization problems.
+
+This rule is part of the "Interfaces" profile of the C++ Core Guidelines, see
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global-init
+
+Note that currently this does not flag calls to non-constexpr functions, and
+therefore globals could still be accessed from functions themselves.
+
Index: docs/ReleaseNotes.rst
===

Re: [PATCH] D18649: [clang-tidy] cppcoreguidelines-interfaces-global-init

[Clement Courbet via cfe-commits]

courbet added inline comments.


Comment at: 
docs/clang-tidy/checks/cppcoreguidelines-interfaces-global-init.rst:7
@@ +6,3 @@
+This check flags initializers of globals that access extern objects,
+and therefore can lead to order-of-initialization problems.
+

alexfh wrote:
> What about indirect access of other globals, e.g. via function calls? We 
> obviously can't analyze functions defined in a different translation unit, 
> and I'm not sure we need to analyze even those defined in the same TU (since 
> it may be imprecise and quite expensive), but maybe we should use some 
> heuristic here (the most agressive would be to flag all initializers 
> containing function calls, but that might be rather noisy).
The guidelines actually recommend flagging calls non-constexpr functions, but 
this would give too many positives. I've added a note.


http://reviews.llvm.org/D18649



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

Re: [PATCH] D18319: Add a PragmaHandler Registry for plugins to add PragmaHandlers to

[Benjamin Kramer via cfe-commits]

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

> OK to commit then?


Yeah, looks good.


Repository:
  rL LLVM

http://reviews.llvm.org/D18319



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Alexander Kornienko via cfe-commits]

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

Looks good with one nit. Thank you for the patch!

Please tell, if you need someone to commit the patch for you.



Comment at: docs/ReleaseNotes.rst:73
@@ +72,3 @@
+
+  The fix-its placement around __declspec attributes on windows and other 
+  attributes is improved.

Please enclose inline code snippets (`__declspec`) in double backquotes.


http://reviews.llvm.org/D18396



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: test/clang-tidy/modernize-use-override-ms.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- -- -fms-extensions 
-std=c++11
+
+// This test is designed to test ms-extension __declspec(dllexport) attributes.

An old comment sneaked in somehow. Please ignore.


http://reviews.llvm.org/D18396



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

Re: [PATCH] D18649: [clang-tidy] cppcoreguidelines-interfaces-global-init

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D18649#390862, @courbet wrote:

> In http://reviews.llvm.org/D18649#389363, @alexfh wrote:
>
> > Thank you for working on the new clang-tidy check!
> >
> > We usually recommend authors to run their checks on a large code base to 
> > ensure it doesn't crash and doesn't generate obvious false positives. It 
> > would be nice, if you could provide a quick summary of such a run (total 
> > number of hits, number of what seems to be a false positive in a sample of 
> > ~100).
>
>
> The tool generated 20k positives on our codebase. On a sample of 100, there 
> are:
>
> - 8 instances of the same exact code structure that's just wrong: const 
> string var = FLAGS_some_flag + "some_sufix";
> - 8 false positives.
> - 84 possible issues. (interestingly 6 of these are from premature use of 
> variations of "extern char* empty_string;"
>
>   The false positives fall into 3 categories:
> - 3 variations of: ``` extern int i; static const int* pi = &i;  // diag ```


Should we warn at all when only an address of a global variable is used?

> // Then pi is dereferenced later, once i is intialized. 

>  Public example of this: 
> https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

> 

> 2. 3 variations of: ``` // .h class A { static const int i = 42; }; // .cc 
> int A::i;  // diag ```


Looks like we have all information to fix this kind of a false positive.

> 

> 

> 3. 2 variations of: ``` // .h class A { static int i; static int j; }; // .cc 
> int A::i = 0; int A::j = i;  // diag ```


ditto


http://reviews.llvm.org/D18649



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

Re: [PATCH] D18180: [clang-tidy] Add a check to detect static definitions in anonymous namespace.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: 
clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp:52
@@ +51,3 @@
+   "anonymous namespace; static is redundant here")
+  << Def->getName();
+  Token Tok;

`DiagnosticBuilder` handles hella different stuff. I wonder whether this code 
works without `->getName()`?


Comment at: 
docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst:9
@@ +8,3 @@
+Static is redundant in the current translation unit because anonymous namespace
+is only visible in the unit.
+

Still reads a bit strange. A bit more wordsmithing:

  In this case, `static` is redundant, because anonymous namespace limits the 
visibility of definitions to a single translation unit.


Comment at: 
test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp:32
@@ +31,3 @@
+STATIC int h = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'h' is a static definition in 
anonymous namespace
+

Please add a CHECK-FIXES for this case as well. Also move the macro definition 
closer to its usage and add a CHECK-FIXES to ensure it doesn't get changed.

Same below.


http://reviews.llvm.org/D18180



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

Re: [PATCH] D18745: [clang-tidy] Adds misc-use-bool-literals check.

[Alexander Kornienko via cfe-commits]

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

1. Looks like `modernize` would be a better fit for this check, since it 
targets a pattern common for pre-standard C++ (or C code).
2. Please clang-format the code.
3. Please run the check on a large enough codebase (at least, on the whole 
LLVM) to ensure it doesn't crash and doesn't produce obvious false positives.


http://reviews.llvm.org/D18745



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Robert Bolter via cfe-commits]

Rob updated this revision to Diff 52543.
Rob added a comment.

Adding back quotes in the docs


http://reviews.llvm.org/D18396

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-use-override-ms.cpp
  test/clang-tidy/modernize-use-override.cpp

Index: test/clang-tidy/modernize-use-override.cpp
===
--- test/clang-tidy/modernize-use-override.cpp
+++ test/clang-tidy/modernize-use-override.cpp
@@ -21,6 +21,7 @@
   virtual void d2();
   virtual void e() = 0;
   virtual void f() = 0;
+  virtual void f2() const = 0;
   virtual void g() = 0;
 
   virtual void j() const;
@@ -74,7 +75,11 @@
 
   virtual void f()=0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
-  // CHECK-FIXES: {{^}}  void f()override =0;
+  // CHECK-FIXES: {{^}}  void f() override =0;
+
+  virtual void f2() const=0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f2() const override =0;
 
   virtual void g() ABSTRACT;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
Index: test/clang-tidy/modernize-use-override-ms.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-ms.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- -- -fms-extensions -std=c++11
+
+// This test is designed to test ms-extension __declspec(dllexport) attributes.
+#define EXPORT __declspec(dllexport)
+
+class Base {
+  virtual EXPORT void a();
+};
+
+class EXPORT InheritedBase {
+  virtual void a();
+};
+
+class Derived : public Base {
+  virtual EXPORT void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  EXPORT void a() override;
+};
+
+class EXPORT InheritedDerived : public InheritedBase {
+  virtual void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() override;
+};
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -67,6 +67,12 @@
 
   This check selectively replaces string literals containing escaped
   characters with raw string literals.
+  
+- Improved ``modernize-use-override`` check
+
+  The fix-its placement around ``__declspec`` attributes on windows and other 
+  attributes is improved.
+  
 
 Clang-tidy changes from 3.7 to 3.8
 ^^
Index: clang-tidy/modernize/UseOverrideCheck.cpp
===
--- clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tidy/modernize/UseOverrideCheck.cpp
@@ -95,9 +95,9 @@
: "'override' is";
 StringRef Correct = HasFinal ? "'final'" : "'override'";
 
-Message =
-(llvm::Twine(Redundant) +
- " redundant since the function is already declared " + Correct).str();
+Message = (llvm::Twine(Redundant) +
+   " redundant since the function is already declared " + Correct)
+  .str();
   }
 
   DiagnosticBuilder Diag = diag(Method->getLocation(), Message);
@@ -118,21 +118,24 @@
   if (!HasFinal && !HasOverride) {
 SourceLocation InsertLoc;
 StringRef ReplacementText = "override ";
+SourceLocation MethodLoc = Method->getLocation();
 
 for (Token T : Tokens) {
-  if (T.is(tok::kw___attribute)) {
+  if (T.is(tok::kw___attribute) &&
+  !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) {
 InsertLoc = T.getLocation();
 break;
   }
 }
 
 if (Method->hasAttrs()) {
   for (const clang::Attr *A : Method->getAttrs()) {
-if (!A->isImplicit()) {
+if (!A->isImplicit() && !A->isInherited()) {
   SourceLocation Loc =
   Sources.getExpansionLoc(A->getRange().getBegin());
-  if (!InsertLoc.isValid() ||
-  Sources.isBeforeInTranslationUnit(Loc, InsertLoc))
+  if ((!InsertLoc.isValid() ||
+   Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) &&
+  !Sources.isBeforeInTranslationUnit(Loc, MethodLoc))
 InsertLoc = Loc;
 }
   }
@@ -163,6 +166,9 @@
 Tokens.back().is(tok::kw_delete)) &&
   GetText(Tokens[Tokens.size() - 2], Sources) == "=") {
 InsertLoc = Tokens[Tokens.size() - 2].getLocation();
+// Check if we need to insert a space.
+if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0)
+  ReplacementText = " override ";
   } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
 InsertLoc = Tokens.back().getLocation();
   }
___
cfe-commits maili

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Robert Bolter via cfe-commits]

Rob added a comment.

ok, I'm not sure if its worth rolling back the change to 
modernize-use-override-ms.cpp, see ammended comments above.

Apart from this it should be good to go.

I think I am going to need someone to submit the patch for me, I can't use svn 
due to 'error 400' so I've been using the git mirror.



Comment at: test/clang-tidy/modernize-use-override-ms.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t
+

alexfh wrote:
> Does this test pass on linux? If no additional compiler arguments needed and 
> it just works, maybe just merge this test code to the main test file?
That's a good question.  I have been running it with the additional 
-fms-extensions argument, but on windows I find it works with or without that 
option.  I havent tested it on linux.  I could maybe rummage up a virtual 
machine and give it a go.




http://reviews.llvm.org/D18396



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

Re: [PATCH] D18551: Added Fixer implementation and fix() interface in clang-format for removing redundant code.

[Eric Liu via cfe-commits]

ioeric updated this revision to Diff 52547.
ioeric added a comment.

- Added fix for empty namespace.
- Merge multiple continuous token deletions into one big replacement; minor 
code styling.
- refactored code to reduce redundancy.


http://reviews.llvm.org/D18551

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11233,6 +11233,151 @@
 
 #endif // _MSC_VER
 
+class FixTest : public ::testing::Test {
+protected:
+  std::string fix(llvm::StringRef Code,
+  const std::vector &Ranges,
+  const FormatStyle &Style = getLLVMStyle()) {
+DEBUG(llvm::errs() << "---\n");
+DEBUG(llvm::errs() << Code << "\n\n");
+tooling::Replacements Replaces = format::fix(Style, Code, Ranges);
+
+std::string Result = applyAllReplacements(Code, Replaces);
+EXPECT_NE("", Result);
+DEBUG(llvm::errs() << "\n" << Result << "\n\n");
+return Result;
+  }
+};
+
+TEST_F(FixTest, CtorInitializationSimpleRedundantComma) {
+  std::string Code = "class A {\nA() : , {} };";
+  std::string Expected = "class A {\nA()  {} };";
+  std::vector Ranges;
+  Ranges.push_back(tooling::Range(17, 0));
+  Ranges.push_back(tooling::Range(19, 0));
+  std::string Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  Code = "class A {\nA() : x(1), {} };";
+  Expected = "class A {\nA() : x(1) {} };";
+  Ranges.clear();
+  Ranges.push_back(tooling::Range(23, 0));
+  Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(FixTest, CtorInitializationBracesInParens) {
+  std::string Code = "class A {\nA() : x({1}),, {} };";
+  std::string Expected = "class A {\nA() : x({1}) {} };";
+  std::vector Ranges;
+  Ranges.push_back(tooling::Range(24, 0));
+  Ranges.push_back(tooling::Range(26, 0));
+  std::string Result = fix(Code, Ranges);
+  DEBUG(llvm::errs() << "\n" << Result << "\n");
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(FixTest, RedundantCommaNotInAffectedRanges) {
+  std::string Code =
+  "class A {\nA() : x({1}), /* comment */, { int x = 0; } };";
+  std::string Expected =
+  "class A {\nA() : x({1}), /* comment */, { int x = 0; } };";
+  // Set the affected range to be "int x = 0", which does not intercept the
+  // constructor initialization list.
+  std::vector Ranges(1, tooling::Range(42, 9));
+  std::string Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  Code = "class A {\nA() : x(1), {} };";
+  Expected = "class A {\nA() : x(1), {} };";
+  // No range. Fixer should do nothing.
+  Ranges.clear();
+  Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(FixTest, CtorInitializationCommentAroundCommas) {
+  // Remove redundant commas and comment between them.
+  std::string Code = "class A {\nA() : x({1}), /* comment */, {} };";
+  std::string Expected = "class A {\nA() : x({1}) {} };";
+  std::vector Ranges;
+  Ranges.push_back(tooling::Range(25, 0));
+  Ranges.push_back(tooling::Range(40, 0));
+  std::string Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma and comment.
+  Code = "class A {\nA() : x({1}), // comment\n{} };";
+  Expected = "class A {\nA() : x({1})\n{} };";
+  Ranges = std::vector(1, tooling::Range(25, 0));
+  Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma, but leave the comment.
+  Code = "class A {\nA() : x({1}), // comment\n , y(1),{} };";
+  Expected = "class A {\nA() : x({1}), // comment\n  y(1){} };";
+  Ranges = std::vector(1, tooling::Range(38, 0));
+  Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma and the comment before it.
+  Code = "class A {\nA() : x({1}), \n/* comment */, y(1),{} };";
+  Expected = "class A {\nA() : x({1}), \n y(1){} };";
+  Ranges = std::vector(1, tooling::Range(40, 0));
+  Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+
+  // Remove trailing comma and the comment after it.
+  Code = "class A {\nA() : , // comment\n y(1),{} };";
+  Expected = "class A {\nA() : \n y(1){} };";
+  Ranges = std::vector(1, tooling::Range(17, 0));
+  Result = fix(Code, Ranges);
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(FixTest, SkipImbalancedParentheses) {
+  std::string Code = "class A {\nA() : x((),, {} };";
+  std::string Expected = "class A {\nA() : x((),, {} };";
+  std::vector Ranges(1, tooling::Range(0, Code.size()));
+  std::string Result = fix(Code, Ranges);
+  DEBUG(llvm::errs() << "\n" << Result << "\n");
+  EXPECT_EQ(Expected, Result);
+}
+
+TEST_F(FixTest, DeleteEmptyNamespaces) {
+  std::string Code = "namespace A {\n"
+ "namespace B {\n"
+ "} // namespace B\n"
+ "} // namespace A\n\n"
+ "namespace C {\n"
+   

Re: [PATCH] D18694: [ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from.

[Haojian Wu via cfe-commits]

hokein updated this revision to Diff 52549.
hokein marked 7 inline comments as done.
hokein added a comment.

Update.


http://reviews.llvm.org/D18694

Files:
  clang-tidy/ClangTidyOptions.cpp
  clang-tidy/ClangTidyOptions.h
  clang-tidy/tool/ClangTidyMain.cpp
  test/clang-tidy/Inputs/explain-config/.clang-tidy
  test/clang-tidy/explain-checks.cpp

Index: test/clang-tidy/explain-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/explain-checks.cpp
@@ -0,0 +1,12 @@
+// RUN: clang-tidy -checks=-*,modernize-use-nullptr -explain-config | FileCheck --check-prefix=CHECK-MESSAGE1 %s
+// RUN: clang-tidy -config="{Checks: '-*,modernize-use-nullptr'}" -explain-config | FileCheck --check-prefix=CHECK-MESSAGE2 %s
+// RUN: clang-tidy -checks=modernize-use-nullptr -config="{Checks: '-*,modernize-use-nullptr'}" -explain-config | FileCheck --check-prefix=CHECK-MESSAGE3 %s
+// RUN: clang-tidy -checks=modernize-use-nullptr -config="{Checks: '-*,-modernize-use-nullptr'}" %S/Inputs/explain-config/a.cc -explain-config | FileCheck --check-prefix=CHECK-MESSAGE4 %s
+// RUN: clang-tidy -explain-config %S/Inputs/explain-config/a.cc | grep "'modernize-use-nullptr' is enabled in the %S/Inputs/explain-config/.clang-tidy."
+
+// CHECK-MESSAGE1: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
+// CHECK-MESSAGE2: 'modernize-use-nullptr' is enabled in the command-line option '-config'.
+
+// CHECK-MESSAGE3: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
+
+// CHECK-MESSAGE4: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
Index: test/clang-tidy/Inputs/explain-config/.clang-tidy
===
--- /dev/null
+++ test/clang-tidy/Inputs/explain-config/.clang-tidy
@@ -0,0 +1 @@
+Checks: '-*,modernize-use-nullptr'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -128,6 +128,12 @@
 )"),
 cl::init(false), cl::cat(ClangTidyCategory));
 
+static cl::opt ExplainConfig("explain-config", cl::desc(R"(
+for each enabled check explains, where it is enabled, i.e. in clang-tidy binary,
+command line or a specific configuration file.
+)"),
+   cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt Config("config", cl::desc(R"(
 Specifies a configuration in YAML/JSON format:
   -config="{Checks: '*',
@@ -256,6 +262,7 @@
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.insertCheckSource(DefaultChecks, "clang-tidy binary");
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
@@ -266,8 +273,10 @@
 DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
 
   ClangTidyOptions OverrideOptions;
-  if (Checks.getNumOccurrences() > 0)
+  if (Checks.getNumOccurrences() > 0) {
 OverrideOptions.Checks = Checks;
+OverrideOptions.insertCheckSource(Checks, "command-line option '-checks'");
+  }
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
@@ -280,6 +289,10 @@
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {
+  if (ParsedConfig->Checks) {
+ParsedConfig->insertCheckSource(*ParsedConfig->Checks,
+"command-line option '-config'");
+  }
   return llvm::make_unique(
   GlobalOptions, ClangTidyOptions::getDefaults()
  .mergeWith(DefaultOptions)
@@ -311,6 +324,19 @@
   ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FileName);
   std::vector EnabledChecks = getCheckNames(EffectiveOptions);
 
+  if (ExplainConfig) {
+for (const std::string& Check : EnabledChecks) {
+  for (const ClangTidyOptions::StringPair &CheckSource:
+   EffectiveOptions.CheckSources) {
+if (GlobList(CheckSource.first).contains(Check)) {
+  llvm::outs() << "'" << Check << "' is enabled in the "
+   << CheckSource.second << ".\n";
+}
+  }
+}
+return 0;
+  }
+
   if (ListChecks) {
 llvm::outs() << "Enabled checks:";
 for (auto CheckName : EnabledChecks)
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -59,6 +59,10 @@
   /// of this instance overridden by the fields of \p Other that have a value.
   ClangTidyOptions mergeWith(const ClangTidyOptions &Other) const;
 
+  /// \brief Inserts checks' source in CheckSource which is used to record
+  /// relationship between each check and its source.
+  void insertCheckSourc

Re: [PATCH] D4619: [SKX] Enabling SKX target (Skylake server chip) in clang

[Benjamin Kramer via cfe-commits]

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

This landed way back in http://reviews.llvm.org/rL214306.


http://reviews.llvm.org/D4619



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

Re: [PATCH] D18694: [ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from.

[Haojian Wu via cfe-commits]

hokein updated this revision to Diff 52550.
hokein marked an inline comment as done.
hokein added a comment.

Remove redundant code.


http://reviews.llvm.org/D18694

Files:
  clang-tidy/ClangTidyOptions.cpp
  clang-tidy/ClangTidyOptions.h
  clang-tidy/tool/ClangTidyMain.cpp
  test/clang-tidy/Inputs/explain-config/.clang-tidy
  test/clang-tidy/explain-checks.cpp

Index: test/clang-tidy/explain-checks.cpp
===
--- /dev/null
+++ test/clang-tidy/explain-checks.cpp
@@ -0,0 +1,12 @@
+// RUN: clang-tidy -checks=-*,modernize-use-nullptr -explain-config | FileCheck --check-prefix=CHECK-MESSAGE1 %s
+// RUN: clang-tidy -config="{Checks: '-*,modernize-use-nullptr'}" -explain-config | FileCheck --check-prefix=CHECK-MESSAGE2 %s
+// RUN: clang-tidy -checks=modernize-use-nullptr -config="{Checks: '-*,modernize-use-nullptr'}" -explain-config | FileCheck --check-prefix=CHECK-MESSAGE3 %s
+// RUN: clang-tidy -checks=modernize-use-nullptr -config="{Checks: '-*,-modernize-use-nullptr'}" %S/Inputs/explain-config/a.cc -explain-config | FileCheck --check-prefix=CHECK-MESSAGE4 %s
+// RUN: clang-tidy -explain-config %S/Inputs/explain-config/a.cc | grep "'modernize-use-nullptr' is enabled in the %S/Inputs/explain-config/.clang-tidy."
+
+// CHECK-MESSAGE1: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
+// CHECK-MESSAGE2: 'modernize-use-nullptr' is enabled in the command-line option '-config'.
+
+// CHECK-MESSAGE3: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
+
+// CHECK-MESSAGE4: 'modernize-use-nullptr' is enabled in the command-line option '-checks'.
Index: test/clang-tidy/Inputs/explain-config/.clang-tidy
===
--- /dev/null
+++ test/clang-tidy/Inputs/explain-config/.clang-tidy
@@ -0,0 +1 @@
+Checks: '-*,modernize-use-nullptr'
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -128,6 +128,12 @@
 )"),
 cl::init(false), cl::cat(ClangTidyCategory));
 
+static cl::opt ExplainConfig("explain-config", cl::desc(R"(
+for each enabled check explains, where it is enabled, i.e. in clang-tidy binary,
+command line or a specific configuration file.
+)"),
+   cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt Config("config", cl::desc(R"(
 Specifies a configuration in YAML/JSON format:
   -config="{Checks: '*',
@@ -256,6 +262,7 @@
 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.insertCheckSource(DefaultChecks, "clang-tidy binary");
   DefaultOptions.WarningsAsErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
@@ -266,8 +273,10 @@
 DefaultOptions.User = llvm::sys::Process::GetEnv("USERNAME");
 
   ClangTidyOptions OverrideOptions;
-  if (Checks.getNumOccurrences() > 0)
+  if (Checks.getNumOccurrences() > 0) {
 OverrideOptions.Checks = Checks;
+OverrideOptions.insertCheckSource(Checks, "command-line option '-checks'");
+  }
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
@@ -280,6 +289,10 @@
   if (!Config.empty()) {
 if (llvm::ErrorOr ParsedConfig =
 parseConfiguration(Config)) {
+  if (ParsedConfig->Checks) {
+ParsedConfig->insertCheckSource(*ParsedConfig->Checks,
+"command-line option '-config'");
+  }
   return llvm::make_unique(
   GlobalOptions, ClangTidyOptions::getDefaults()
  .mergeWith(DefaultOptions)
@@ -311,6 +324,19 @@
   ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FileName);
   std::vector EnabledChecks = getCheckNames(EffectiveOptions);
 
+  if (ExplainConfig) {
+for (const std::string& Check : EnabledChecks) {
+  for (const ClangTidyOptions::StringPair &CheckSource:
+   EffectiveOptions.CheckSources) {
+if (GlobList(CheckSource.first).contains(Check)) {
+  llvm::outs() << "'" << Check << "' is enabled in the "
+   << CheckSource.second << ".\n";
+}
+  }
+}
+return 0;
+  }
+
   if (ListChecks) {
 llvm::outs() << "Enabled checks:";
 for (auto CheckName : EnabledChecks)
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -59,6 +59,10 @@
   /// of this instance overridden by the fields of \p Other that have a value.
   ClangTidyOptions mergeWith(const ClangTidyOptions &Other) const;
 
+  /// \brief Inserts checks' source in CheckSource which is used to record
+  /// relationship between each check and its source.
+  void i

Re: [PATCH] D18694: [ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from.

[Haojian Wu via cfe-commits]

hokein marked an inline comment as done.


Comment at: clang-tidy/tool/ClangTidyMain.cpp:329
@@ +328,3 @@
+for (const std::string& Check : EnabledChecks) {
+  for (const ClangTidyOptions::StringPair &CheckSource:
+   EffectiveOptions.CheckSources) {

I updated the patch and made sure the behavior keep the order of configuration.

But if the case like:

1. enabled in binary
2. enabled in config file
3. enabled in command-line option `-checks`

Only command-line option `-checks` will be diagnosed, since the processor order 
of configuration is binary -> config file -> command-line option `-checks`.  


http://reviews.llvm.org/D18694



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

The patch doesn't apply cleanly. Please rebase it on top of HEAD.


http://reviews.llvm.org/D18396



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D18396#391031, @Rob wrote:

> ok, I'm not sure if its worth rolling back the change to 
> modernize-use-override-ms.cpp, see ammended comments above.


No, the test is fine as it is.

> Apart from this it should be good to go.

> 

> I think I am going to need someone to submit the patch for me, I can't use 
> svn due to 'error 400' so I've been using the git mirror.


If you don't know for sure, you don't have the commit access. I'll commit the 
patch for you.



Comment at: test/clang-tidy/modernize-use-override-ms.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- -- -fms-extensions 
-std=c++11
+
+// This test is designed to test ms-extension __declspec(dllexport) attributes.

Everything's fine with the test as it is now: both -fms-extensions and 
-std=c++11 are needed on non-windows platforms.


http://reviews.llvm.org/D18396



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

Re: [PATCH] D18180: [clang-tidy] Add a check to detect static definitions in anonymous namespace.

[Haojian Wu via cfe-commits]

hokein updated this revision to Diff 52552.
hokein marked 3 inline comments as done.
hokein added a comment.

Address comments.


http://reviews.llvm.org/D18180

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp
  clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
  docs/clang-tidy/checks/list.rst
  
docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
  test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp

Index: test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp
@@ -0,0 +1,48 @@
+// RUN: %check_clang_tidy %s readability-static-definition-in-anonymous-namespace %t
+
+#define DEFINE_STATIC static
+#define DEFINE_STATIC_VAR(x) static int x = 2
+
+namespace {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'c' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]
+// CHECK-FIXES: {{^}}int c = 1;
+static const int d = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'd' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int d = 1;
+const static int e = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'e' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}const int e = 1;
+
+void f() {
+  int a = 1;
+  static int b = 1;
+}
+
+static int g() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'g' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}int g() {
+  return 1;
+}
+
+DEFINE_STATIC int h = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 'h' is a static definition in anonymous namespace
+// CHECK-FIXES: {{^}}DEFINE_STATIC int h = 1;
+
+DEFINE_STATIC_VAR(i);
+// CHECK-FIXES: {{^}}DEFINE_STATIC_VAR(i);
+
+} // namespace
+
+namespace N {
+
+int a = 1;
+const int b = 1;
+static int c = 1;
+static const int d = 1;
+const static int e = 1;
+
+} // namespace N
Index: docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst
@@ -0,0 +1,17 @@
+.. title:: clang-tidy - readability-static-definition-in-anonymous-namespace
+
+readability-static-definition-in-anonymous-namespace
+
+
+Finds static function and variable definitions in anonymous namespace.
+
+In this case, `static` is redundant, because anonymous namespace limits the
+visibility of definitions to a single translation unit.
+
+.. code:: c++
+  namespace {
+static int a = 1; // Warning.
+static const b = 1; // Warning.
+  }
+
+The check will apply a fix by removing the redundant `static` qualifier.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -101,4 +101,5 @@
readability-redundant-string-cstr
readability-redundant-string-init
readability-simplify-boolean-expr
+   readability-static-definition-in-anonymous-namespace
readability-uniqueptr-delete-release
Index: clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
===
--- /dev/null
+++ clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.h
@@ -0,0 +1,35 @@
+//===--- StaticDefinitionInAnonymousNamespaceCheck.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_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STATIC_DEFINITION_IN_ANONYMOUS_NAMESPACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Finds static function and variable definitions in anonymous namespace.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.html
+class StaticDefinitionInAnonymousNamespaceCheck : public ClangTidyCheck {
+public:
+  StaticDefinitionInAnonymousNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readabili

Re: [PATCH] D18180: [clang-tidy] Add a check to detect static definitions in anonymous namespace.

[Haojian Wu via cfe-commits]

hokein added inline comments.


Comment at: 
clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp:52
@@ +51,3 @@
+   "anonymous namespace; static is redundant here")
+  << Def->getName();
+  Token Tok;

alexfh wrote:
> `DiagnosticBuilder` handles hella different stuff. I wonder whether this code 
> works without `->getName()`?
Using `Def` works!


http://reviews.llvm.org/D18180



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

Re: [PATCH] D18319: Add a PragmaHandler Registry for plugins to add PragmaHandlers to

[John Brawn via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL265295: Add a PragmaHandler Registry for plugins to add 
PragmaHandlers to (authored by john.brawn).

Changed prior to commit:
  http://reviews.llvm.org/D18319?vs=51168&id=52553#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18319

Files:
  cfe/trunk/docs/ClangPlugins.rst
  cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp
  cfe/trunk/include/clang/Lex/Preprocessor.h
  cfe/trunk/lib/Lex/Pragma.cpp
  cfe/trunk/test/Frontend/plugin-annotate-functions.c

Index: cfe/trunk/docs/ClangPlugins.rst
===
--- cfe/trunk/docs/ClangPlugins.rst
+++ cfe/trunk/docs/ClangPlugins.rst
@@ -43,6 +43,26 @@
 
   static FrontendPluginRegistry::Add X("my-plugin-name", "my plugin description");
 
+Defining pragmas
+
+
+Plugins can also define pragmas by declaring a ``PragmaHandler`` and
+registering it using ``PragmaHandlerRegistry::Add<>``:
+
+.. code-block:: c++
+
+  // Define a pragma handler for #pragma example_pragma
+  class ExamplePragmaHandler : public PragmaHandler {
+  public:
+ExamplePragmaHandler() : PragmaHandler("example_pragma") { }
+void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
+  Token &PragmaTok) {
+  // Handle the pragma
+}
+  };
+
+  static PragmaHandlerRegistry::Add Y("example_pragma","example pragma description");
+
 Putting it all together
 ===
 
Index: cfe/trunk/include/clang/Lex/Preprocessor.h
===
--- cfe/trunk/include/clang/Lex/Preprocessor.h
+++ cfe/trunk/include/clang/Lex/Preprocessor.h
@@ -32,6 +32,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/Registry.h"
 #include 
 #include 
 
@@ -1937,6 +1938,9 @@
   virtual bool HandleComment(Preprocessor &PP, SourceRange Comment) = 0;
 };
 
+/// \brief Registry of pragma handlers added by plugins
+typedef llvm::Registry PragmaHandlerRegistry;
+
 }  // end namespace clang
 
 #endif
Index: cfe/trunk/test/Frontend/plugin-annotate-functions.c
===
--- cfe/trunk/test/Frontend/plugin-annotate-functions.c
+++ cfe/trunk/test/Frontend/plugin-annotate-functions.c
@@ -1,7 +1,25 @@
-// RUN: %clang -fplugin=%llvmshlibdir/AnnotateFunctions%pluginext -emit-llvm -S %s -o - | FileCheck %s
+// RUN: %clang -fplugin=%llvmshlibdir/AnnotateFunctions%pluginext -emit-llvm -DPRAGMA_ON -S %s -o - | FileCheck %s --check-prefix=PRAGMA
+// RUN: %clang -fplugin=%llvmshlibdir/AnnotateFunctions%pluginext -emit-llvm -S %s -o - | FileCheck %s --check-prefix=NOPRAGMA
+// RUN: not %clang -fplugin=%llvmshlibdir/AnnotateFunctions%pluginext -emit-llvm -DBAD_PRAGMA -S %s -o - 2>&1 | FileCheck %s --check-prefix=BADPRAGMA
 // REQUIRES: plugins, examples
 
-// CHECK: [[STR_VAR:@.+]] = private unnamed_addr constant [19 x i8] c"example_annotation\00"
-// CHECK: @llvm.global.annotations = {{.*}}@fn1{{.*}}[[STR_VAR]]{{.*}}@fn2{{.*}}[[STR_VAR]]
+#ifdef PRAGMA_ON
+#pragma enable_annotate
+#endif
+
+// BADPRAGMA: warning: extra tokens at end of #pragma directive
+#ifdef BAD_PRAGMA
+#pragma enable_annotate something
+#endif
+
+// PRAGMA: [[STR_VAR:@.+]] = private unnamed_addr constant [19 x i8] c"example_annotation\00"
+// PRAGMA: @llvm.global.annotations = {{.*}}@fn1{{.*}}[[STR_VAR]]{{.*}}@fn2{{.*}}[[STR_VAR]]
+// NOPRAGMA-NOT: [[STR_VAR:@.+]] = private unnamed_addr constant [19 x i8] c"example_annotation\00"
+// NOPRAGMA-NOT: @llvm.global.annotations = {{.*}}@fn1{{.*}}[[STR_VAR]]{{.*}}@fn2{{.*}}[[STR_VAR]]
 void fn1() { }
 void fn2() { }
+
+// BADPRAGMA: error: #pragma enable_annotate not allowed after declarations
+#ifdef BAD_PRAGMA
+#pragma enable_annotate
+#endif
Index: cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp
===
--- cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp
+++ cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp
@@ -7,20 +7,29 @@
 //
 //===--===//
 //
-// Example clang plugin which adds an annotation to every function.
+// Example clang plugin which adds an annotation to every function in
+// translation units that start with #pragma enable_annotate.
 //
 //===--===//
 
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/AST/AST.h"
 #include "clang/AST/ASTConsumer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/LexDiagnostic.h"
 using namespace clang;
 
 namespace {
 
+static bool EnableAnnotate = false;
+static bool HandledDecl = false;
+
 class AnnotateFunctionsConsumer : public ASTConsumer {
 public:
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
+Hand

r265295 - Add a PragmaHandler Registry for plugins to add PragmaHandlers to

[John Brawn via cfe-commits]

Author: john.brawn
Date: Mon Apr  4 09:22:58 2016
New Revision: 265295

URL: http://llvm.org/viewvc/llvm-project?rev=265295&view=rev
Log:
Add a PragmaHandler Registry for plugins to add PragmaHandlers to

This allows plugins which add AST passes to also define pragmas to do things
like only enable certain behaviour of the AST pass in files where a certain
pragma is used.

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

Modified:
cfe/trunk/docs/ClangPlugins.rst
cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp
cfe/trunk/include/clang/Lex/Preprocessor.h
cfe/trunk/lib/Lex/Pragma.cpp
cfe/trunk/test/Frontend/plugin-annotate-functions.c

Modified: cfe/trunk/docs/ClangPlugins.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ClangPlugins.rst?rev=265295&r1=265294&r2=265295&view=diff
==
--- cfe/trunk/docs/ClangPlugins.rst (original)
+++ cfe/trunk/docs/ClangPlugins.rst Mon Apr  4 09:22:58 2016
@@ -43,6 +43,26 @@ register a plugin in a library, use ``Fr
 
   static FrontendPluginRegistry::Add X("my-plugin-name", "my plugin 
description");
 
+Defining pragmas
+
+
+Plugins can also define pragmas by declaring a ``PragmaHandler`` and
+registering it using ``PragmaHandlerRegistry::Add<>``:
+
+.. code-block:: c++
+
+  // Define a pragma handler for #pragma example_pragma
+  class ExamplePragmaHandler : public PragmaHandler {
+  public:
+ExamplePragmaHandler() : PragmaHandler("example_pragma") { }
+void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
+  Token &PragmaTok) {
+  // Handle the pragma
+}
+  };
+
+  static PragmaHandlerRegistry::Add 
Y("example_pragma","example pragma description");
+
 Putting it all together
 ===
 

Modified: cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp?rev=265295&r1=265294&r2=265295&view=diff
==
--- cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp (original)
+++ cfe/trunk/examples/AnnotateFunctions/AnnotateFunctions.cpp Mon Apr  4 
09:22:58 2016
@@ -7,20 +7,29 @@
 //
 
//===--===//
 //
-// Example clang plugin which adds an annotation to every function.
+// Example clang plugin which adds an annotation to every function in
+// translation units that start with #pragma enable_annotate.
 //
 
//===--===//
 
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/AST/AST.h"
 #include "clang/AST/ASTConsumer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/LexDiagnostic.h"
 using namespace clang;
 
 namespace {
 
+static bool EnableAnnotate = false;
+static bool HandledDecl = false;
+
 class AnnotateFunctionsConsumer : public ASTConsumer {
 public:
   bool HandleTopLevelDecl(DeclGroupRef DG) override {
+HandledDecl = true;
+if (!EnableAnnotate)
+  return true;
 for (auto D : DG)
   if (FunctionDecl *FD = dyn_cast(D))
 FD->addAttr(AnnotateAttr::CreateImplicit(FD->getASTContext(),
@@ -46,7 +55,34 @@ public:
   }
 };
 
+class PragmaAnnotateHandler : public PragmaHandler {
+public:
+  PragmaAnnotateHandler() : PragmaHandler("enable_annotate") { }
+
+  void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
+Token &PragmaTok) override {
+
+Token Tok;
+PP.LexUnexpandedToken(Tok);
+if (Tok.isNot(tok::eod))
+  PP.Diag(Tok, diag::ext_pp_extra_tokens_at_eol) << "pragma";
+
+if (HandledDecl) {
+  DiagnosticsEngine &D = PP.getDiagnostics();
+  unsigned ID = D.getCustomDiagID(
+DiagnosticsEngine::Error,
+"#pragma enable_annotate not allowed after declarations");
+  D.Report(PragmaTok.getLocation(), ID);
+}
+
+EnableAnnotate = true;
+  }
+};
+
 }
 
 static FrontendPluginRegistry::Add
 X("annotate-fns", "annotate functions");
+
+static PragmaHandlerRegistry::Add
+Y("enable_annotate","enable annotation");

Modified: cfe/trunk/include/clang/Lex/Preprocessor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/Preprocessor.h?rev=265295&r1=265294&r2=265295&view=diff
==
--- cfe/trunk/include/clang/Lex/Preprocessor.h (original)
+++ cfe/trunk/include/clang/Lex/Preprocessor.h Mon Apr  4 09:22:58 2016
@@ -32,6 +32,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/TinyPtrVector.h"
 #include "llvm/Support/Allocator.h"
+#include "llvm/Support/Registry.h"
 #include 
 #include 
 
@@ -1937,6 +1938,9 @@ public:
   virtual bool HandleComment(Preprocessor &PP, SourceRange Comment) = 0;
 };
 
+/// \brief Registry of pragma handlers added by plugins
+ty

[clang-tools-extra] r265298 - [clang-tidy] fix a couple of modernize-use-override bugs

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Mon Apr  4 09:31:36 2016
New Revision: 265298

URL: http://llvm.org/viewvc/llvm-project?rev=265298&view=rev
Log:
[clang-tidy] fix a couple of modernize-use-override bugs

Fix for __declspec attributes and const=0 without space

This patch is to address 2 problems I found with 
Clang-tidy:modernize-use-override.

1: missing spaces on pure function decls.

Orig:
void pure() const=0
Problem:
void pure() constoverride =0
Fixed:
void pure() const override =0

2: This is ms-extension specific, but possibly applies to other attribute 
types. The override is placed before the attribute which doesn’t work well with 
declspec as this attribute can be inherited or placed before the method 
identifier.

Orig:
class __declspec(dllexport) X : public Y

{
void p();
};
Problem:
class override __declspec(dllexport) class X : public Y

{
void p();
};
Fixed:
class __declspec(dllexport) class X : public Y

{
void p() override;
};

Patch by Robert Bolter!

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

Added:
clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp

Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp?rev=265298&r1=265297&r2=265298&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp Mon Apr  
4 09:31:36 2016
@@ -95,9 +95,9 @@ void UseOverrideCheck::check(const Match
: "'override' is";
 StringRef Correct = HasFinal ? "'final'" : "'override'";
 
-Message =
-(llvm::Twine(Redundant) +
- " redundant since the function is already declared " + Correct).str();
+Message = (llvm::Twine(Redundant) +
+   " redundant since the function is already declared " + Correct)
+  .str();
   }
 
   DiagnosticBuilder Diag = diag(Method->getLocation(), Message);
@@ -118,9 +118,11 @@ void UseOverrideCheck::check(const Match
   if (!HasFinal && !HasOverride) {
 SourceLocation InsertLoc;
 StringRef ReplacementText = "override ";
+SourceLocation MethodLoc = Method->getLocation();
 
 for (Token T : Tokens) {
-  if (T.is(tok::kw___attribute)) {
+  if (T.is(tok::kw___attribute) &&
+  !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) {
 InsertLoc = T.getLocation();
 break;
   }
@@ -128,11 +130,12 @@ void UseOverrideCheck::check(const Match
 
 if (Method->hasAttrs()) {
   for (const clang::Attr *A : Method->getAttrs()) {
-if (!A->isImplicit()) {
+if (!A->isImplicit() && !A->isInherited()) {
   SourceLocation Loc =
   Sources.getExpansionLoc(A->getRange().getBegin());
-  if (!InsertLoc.isValid() ||
-  Sources.isBeforeInTranslationUnit(Loc, InsertLoc))
+  if ((!InsertLoc.isValid() ||
+   Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) &&
+  !Sources.isBeforeInTranslationUnit(Loc, MethodLoc))
 InsertLoc = Loc;
 }
   }
@@ -163,6 +166,9 @@ void UseOverrideCheck::check(const Match
 Tokens.back().is(tok::kw_delete)) &&
   GetText(Tokens[Tokens.size() - 2], Sources) == "=") {
 InsertLoc = Tokens[Tokens.size() - 2].getLocation();
+// Check if we need to insert a space.
+if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0)
+  ReplacementText = " override ";
   } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
 InsertLoc = Tokens.back().getLocation();
   }

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=265298&r1=265297&r2=265298&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Mon Apr  4 09:31:36 2016
@@ -155,9 +155,12 @@ identified.  The improvements since the
 
 Fixed bugs:
 
-  Crash when running on compile database with relative source files paths.
+- Crash when running on compile database with relative source files paths.
 
-  Crash when running with the `-fdelayed-template-parsing` flag.
+- Crash when running with the `-fdelayed-template-parsing` flag.
+
+- The ``modernize-use-override`` check: incorrect fix-its placement around
+  ``__declspec`` and other attributes.
 
 Clang-tidy changes from 3.7 to 3.8
 ^^

Added: clang-tools-extra/trunk

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Alexander Kornienko via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL265298: [clang-tidy] fix a couple of modernize-use-override 
bugs (authored by alexfh).

Changed prior to commit:
  http://reviews.llvm.org/D18396?vs=52543&id=52555#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18396

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp

Index: clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -95,9 +95,9 @@
: "'override' is";
 StringRef Correct = HasFinal ? "'final'" : "'override'";
 
-Message =
-(llvm::Twine(Redundant) +
- " redundant since the function is already declared " + Correct).str();
+Message = (llvm::Twine(Redundant) +
+   " redundant since the function is already declared " + Correct)
+  .str();
   }
 
   DiagnosticBuilder Diag = diag(Method->getLocation(), Message);
@@ -118,21 +118,24 @@
   if (!HasFinal && !HasOverride) {
 SourceLocation InsertLoc;
 StringRef ReplacementText = "override ";
+SourceLocation MethodLoc = Method->getLocation();
 
 for (Token T : Tokens) {
-  if (T.is(tok::kw___attribute)) {
+  if (T.is(tok::kw___attribute) &&
+  !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) {
 InsertLoc = T.getLocation();
 break;
   }
 }
 
 if (Method->hasAttrs()) {
   for (const clang::Attr *A : Method->getAttrs()) {
-if (!A->isImplicit()) {
+if (!A->isImplicit() && !A->isInherited()) {
   SourceLocation Loc =
   Sources.getExpansionLoc(A->getRange().getBegin());
-  if (!InsertLoc.isValid() ||
-  Sources.isBeforeInTranslationUnit(Loc, InsertLoc))
+  if ((!InsertLoc.isValid() ||
+   Sources.isBeforeInTranslationUnit(Loc, InsertLoc)) &&
+  !Sources.isBeforeInTranslationUnit(Loc, MethodLoc))
 InsertLoc = Loc;
 }
   }
@@ -163,6 +166,9 @@
 Tokens.back().is(tok::kw_delete)) &&
   GetText(Tokens[Tokens.size() - 2], Sources) == "=") {
 InsertLoc = Tokens[Tokens.size() - 2].getLocation();
+// Check if we need to insert a space.
+if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0)
+  ReplacementText = " override ";
   } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
 InsertLoc = Tokens.back().getLocation();
   }
Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst
===
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst
@@ -155,9 +155,12 @@
 
 Fixed bugs:
 
-  Crash when running on compile database with relative source files paths.
+- Crash when running on compile database with relative source files paths.
 
-  Crash when running with the `-fdelayed-template-parsing` flag.
+- Crash when running with the `-fdelayed-template-parsing` flag.
+
+- The ``modernize-use-override`` check: incorrect fix-its placement around
+  ``__declspec`` and other attributes.
 
 Clang-tidy changes from 3.7 to 3.8
 ^^
Index: clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-ms.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- -- -fms-extensions -std=c++11
+
+// This test is designed to test ms-extension __declspec(dllexport) attributes.
+#define EXPORT __declspec(dllexport)
+
+class Base {
+  virtual EXPORT void a();
+};
+
+class EXPORT InheritedBase {
+  virtual void a();
+};
+
+class Derived : public Base {
+  virtual EXPORT void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  EXPORT void a() override;
+};
+
+class EXPORT InheritedDerived : public InheritedBase {
+  virtual void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() override;
+};
+
Index: clang-tools-extra/trunk/test/clang-tidy/modernize-use-override.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-overrid

Re: [PATCH] D18649: [clang-tidy] cppcoreguidelines-interfaces-global-init

[Clement Courbet via cfe-commits]

courbet added a comment.

In http://reviews.llvm.org/D18649#391001, @alexfh wrote:

> In http://reviews.llvm.org/D18649#390862, @courbet wrote:
>
> > In http://reviews.llvm.org/D18649#389363, @alexfh wrote:
> >
> > > Thank you for working on the new clang-tidy check!
> > >
> > > We usually recommend authors to run their checks on a large code base to 
> > > ensure it doesn't crash and doesn't generate obvious false positives. It 
> > > would be nice, if you could provide a quick summary of such a run (total 
> > > number of hits, number of what seems to be a false positive in a sample 
> > > of ~100).
> >
> >
> > The tool generated 20k positives on our codebase. On a sample of 100, there 
> > are:
> >
> > - 8 instances of the same exact code structure that's just wrong: const 
> > string var = FLAGS_some_flag + "some_sufix";
> > - 8 false positives.
> > - 84 possible issues. (interestingly 6 of these are from premature use of 
> > variations of "extern char* empty_string;"
> >
> >   The false positives fall into 3 categories:
> > - 3 variations of: ``` extern int i; static const int* pi = &i;  // diag ```
>
>
> Should we warn at all when only an address of a global variable is used?


We could, but it would allow stuff like:

  extern const int i;
  const char c = *(const char*)&i;

No strong opinion here.

> 

> 

> > // Then pi is dereferenced later, once i is intialized. 

> 

> >  Public example of this: 
> > https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027

> 

> > 

> 

> > 2. 3 variations of: ``` // .h class A { static const int i = 42; }; // .cc 
> > int A::i;  // diag ```

> 

> 

> Looks like we have all information to fix this kind of a false positive.

> 

> > 

> 

> > 

> 

> > 3. 2 variations of: ``` // .h class A { static int i; static int j; }; // 
> > .cc int A::i = 0; int A::j = i;  // diag ```

> 

> 

> ditto


It turns out (2) and (3) are just variations of the same - (2) was just:

  // .h
  class A {
static const int i = 0;
static const int j = i;
  };
  // .cc
  int A::i;
  int A::j;

Which has *exactly* the same semantics as (3).

I could not find a way to go from a decl to its (possible) definition. I was 
thinking of matching all global definitions that are not declarations and then 
making sure that the current matches are not referring to one of those 
definitions (in syntactic order because of [3.6.2.3]). Does this  sound like a 
reasonable idea ?


http://reviews.llvm.org/D18649



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

Re: [PATCH] D18398: Compilation for Intel MCU (Part 1/3)

[Andrey Turetskiy via cfe-commits]

aturetsk added a comment.

Ping.


http://reviews.llvm.org/D18398



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

Re: [PATCH] D17180: Fix failing python bindings test

[Jonathan B Coe via cfe-commits]

jbcoe added a comment.

The other python test fix has been applied in r263170 (Differential Revision: 
http://reviews.llvm.org/D17226).

Has anyone had a chance to look at _this_ patch yet?


Repository:
  rL LLVM

http://reviews.llvm.org/D17180



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D18396#391084, @alexfh wrote:

> The patch doesn't apply cleanly. Please rebase it on top of HEAD.


Actually, the conflict was only in the ReleaseNotes.rst file, which I had to 
change anyway to fix the formatting around the place you changed. The patch is 
committed as http://reviews.llvm.org/rL265298. Thank you!


Repository:
  rL LLVM

http://reviews.llvm.org/D18396



___
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] D18649: [clang-tidy] cppcoreguidelines-interfaces-global-init

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/D18649#391112, @courbet wrote:

> In http://reviews.llvm.org/D18649#391001, @alexfh wrote:
>
> > In http://reviews.llvm.org/D18649#390862, @courbet wrote:
> >
> > > In http://reviews.llvm.org/D18649#389363, @alexfh wrote:
> > >
> > > > Thank you for working on the new clang-tidy check!
> > > >
> > > > We usually recommend authors to run their checks on a large code base 
> > > > to ensure it doesn't crash and doesn't generate obvious false 
> > > > positives. It would be nice, if you could provide a quick summary of 
> > > > such a run (total number of hits, number of what seems to be a false 
> > > > positive in a sample of ~100).
> > >
> > >
> > > The tool generated 20k positives on our codebase. On a sample of 100, 
> > > there are:
> > >
> > > - 8 instances of the same exact code structure that's just wrong: const 
> > > string var = FLAGS_some_flag + "some_sufix";
> > > - 8 false positives.
> > > - 84 possible issues. (interestingly 6 of these are from premature use of 
> > > variations of "extern char* empty_string;"
> > >
> > >   The false positives fall into 3 categories:
> > > - 3 variations of: ``` extern int i; static const int* pi = &i;  // diag 
> > > ```
> >
> >
> > Should we warn at all when only an address of a global variable is used?
>
>
> We could, but it would allow stuff like:
>
>   extern const int i;
>   const char c = *(const char*)&i;
>
>
> No strong opinion here.
>
> > 
>
> > 
>
> > > // Then pi is dereferenced later, once i is intialized. 
>
> > 
>
> > >  Public example of this: 
> > > https://github.com/python-git/python/blob/py3k/Objects/dictobject.c#L2027
>
> > 
>
> > > 
>
> > 
>
> > > 2. 3 variations of: ``` // .h class A { static const int i = 42; }; // 
> > > .cc int A::i;  // diag ```
>
> > 
>
> > 
>
> > Looks like we have all information to fix this kind of a false positive.
>
> > 
>
> > > 
>
> > 
>
> > > 
>
> > 
>
> > > 3. 2 variations of: ``` // .h class A { static int i; static int j; }; // 
> > > .cc int A::i = 0; int A::j = i;  // diag ```
>
> > 
>
> > 
>
> > ditto
>
>
> It turns out (2) and (3) are just variations of the same - (2) was just:
>
>   // .h
>   class A {
> static const int i = 0;
> static const int j = i;
>   };
>   // .cc
>   int A::i;
>   int A::j;
>
>
> Which has *exactly* the same semantics as (3).
>
> I could not find a way to go from a decl to its (possible) definition.


`VarDecl` is also `Redeclarable`, which has various ways to iterate 
over all related declarations.

> I was thinking of matching all global definitions that are not declarations 
> and then making sure that the current matches are not referring to one of 
> those definitions (in syntactic order because of [3.6.2.3]). Does this  sound 
> like a reasonable idea ?





http://reviews.llvm.org/D18649



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

Re: [PATCH] D17815: [libc++abi] Use fallback_malloc to allocate __cxa_eh_globals in case of dynamic memory exhaustion.

[Igor Kudrin via cfe-commits]

ikudrin added a comment.

Ping...


http://reviews.llvm.org/D17815



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

Re: [PATCH] D18694: [ClangTidy] Add an 'explain-checks' option to diagnose where each checks comes from.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy/tool/ClangTidyMain.cpp:329
@@ +328,3 @@
+for (const std::string& Check : EnabledChecks) {
+  for (const ClangTidyOptions::StringPair &CheckSource:
+   EffectiveOptions.CheckSources) {

hokein wrote:
> I updated the patch and made sure the behavior keep the order of 
> configuration.
> 
> But if the case like:
> 
> 1. enabled in binary
> 2. enabled in config file
> 3. enabled in command-line option `-checks`
> 
> Only command-line option `-checks` will be diagnosed, since the processor 
> order of configuration is binary -> config file -> command-line option 
> `-checks`.  
As discussed offline, the order of iteration matters here. And it looks like we 
should look at the complete Checks options without splitting them to components 
(in order to handle negative patterns).


http://reviews.llvm.org/D18694



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

Re: [PATCH] D18180: [clang-tidy] Add a check to detect static definitions in anonymous namespace.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: 
clang-tidy/readability/StaticDefinitionInAnonymousNamespaceCheck.cpp:49
@@ +48,3 @@
+
+  DiagnosticBuilder Diag =
+  diag(Def->getLocation(), "%0 is a static definition in "

nit: `auto`?


Comment at: 
docs/clang-tidy/checks/readability-static-definition-in-anonymous-namespace.rst:8
@@ +7,3 @@
+
+In this case, `static` is redundant, because anonymous namespace limits the
+visibility of definitions to a single translation unit.

Double backquotes should be used for inline code snippets (yeah, I know, this 
is confusing: different flavors of markdown and RST are completely inconsistent 
in this regard).


Comment at: 
test/clang-tidy/readability-static-definition-in-anonymous-namespace.cpp:33
@@ +32,3 @@
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 'h' is a static definition in 
anonymous namespace
+// CHECK-FIXES: {{^}}DEFINE_STATIC int h = 1;
+

You missed the "Also move the macro definition closer to its usage and add a 
CHECK-FIXES to ensure it doesn't get changed." part.

Same below.


http://reviews.llvm.org/D18180



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

Re: [PATCH] D18509: clang-tidy: add_new_check.py stubs out release notes

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D18509#388071, @hintonda wrote:

> With this change, won't you need to update 2 different repos: clang and 
> extra?  Is it possible to update both with a single Phabricator patch?


This is updating the release notes in the extra repository.  (The release notes 
are new to this repository; you may not have noticed them being added.)


http://reviews.llvm.org/D18509



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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

As it stands currently, you can't commit a header with `CHECK-MESSAGES` and 
`CHECK-FIXES` lines and have them verified.

That's the whole point of this changeset.

Currently you have to do something very hacky in order to verify changes made 
to headers.


http://reviews.llvm.org/D17482



___
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

[Steve Downey via cfe-commits]

sdowney added a comment.

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.

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.


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] D18110: [OpenMP] Fix SEMA bug in the capture of global variables in template functions.

[Carlo Bertolli via cfe-commits]

carlo.bertolli added a comment.

Hi Alexey

I understand your concerns and share the danger of working on optimizations 
before a correct implementation is actually in place.
I do not think this is the case. My comment was very precise about the 
cause-effect: the OpenMP specification was defined in a certain way, and that 
is what we should implement, and the consequence of that is an optimized 
implementation compared to what we had in OpenMP 4.0.

Here are the passages of the OpenMP specifications where I think this is 
specified (from OpenMP 4.5 specification text):

- Page 197, description of firstprivate: "A list item that appears in a 
firstprivate clause is subject to the private clause semantics described in 
Section 2.15.3.3 on page 192, except as noted. In addition,** the new list item 
is initialized from the original list item existing before the construct**."

- Page 105, restrictions of target: "If an array section is a list item in a 
map clause and the array section is derived from a variable for which the type 
is pointer then the data-sharing attribute for that variable in the construct 
is firstprivate. Prior to the execution of the construct, **the private 
variable is initialized with the address of the storage location of the 
corresponding array section in the device data environment**."

- Page 105, following paragraph: "If a zero-length array section is a list item 
in a map clause, and the array section is derived from a variable for the which 
the type is pointer then** that variable is initialized with the address of the 
corresponding storage location in the device data environment**."

In this I read that whenever we have a map with an array section, or a map on a 
zero-length array section, or a firstprivate on any kind of variable, we need 
to copy the value of the variable (scalar, pointer) into the corresponding 
private variable.

Implementing this with references would work indeed, but it would be like 
implementing pass-by-value using references.

We had long discussion about this with various members of the OpenMP committee 
on how to implement this correctly. This is why I am insisting on this not 
being an optimization.

However, this may not apply here (that is why I conditioned by previous 
comment) or you still think that we should always pass references for any kind 
of argument to target regions. It is your call, but I wanted to make sure that 
this is put in the right light.

Thanks!

- Carlo


http://reviews.llvm.org/D18110



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

r265301 - AnnotateFunctions: Tweak for mingw.

[NAKAMURA Takumi via cfe-commits]

Author: chapuni
Date: Mon Apr  4 10:30:44 2016
New Revision: 265301

URL: http://llvm.org/viewvc/llvm-project?rev=265301&view=rev
Log:
AnnotateFunctions: Tweak for mingw.

  - Externalize the registry.
  - Update libdeps.

Modified:
cfe/trunk/examples/AnnotateFunctions/CMakeLists.txt
cfe/trunk/include/clang/Lex/Preprocessor.h
cfe/trunk/lib/Lex/Preprocessor.cpp

Modified: cfe/trunk/examples/AnnotateFunctions/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/examples/AnnotateFunctions/CMakeLists.txt?rev=265301&r1=265300&r2=265301&view=diff
==
--- cfe/trunk/examples/AnnotateFunctions/CMakeLists.txt (original)
+++ cfe/trunk/examples/AnnotateFunctions/CMakeLists.txt Mon Apr  4 10:30:44 2016
@@ -3,7 +3,9 @@ add_llvm_loadable_module(AnnotateFunctio
 if(LLVM_ENABLE_PLUGINS AND (WIN32 OR CYGWIN))
   target_link_libraries(AnnotateFunctions ${cmake_2_8_12_PRIVATE}
 clangAST
+clangBasic
 clangFrontend
+clangLex
 LLVMSupport
 )
 endif()

Modified: cfe/trunk/include/clang/Lex/Preprocessor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/Preprocessor.h?rev=265301&r1=265300&r2=265301&view=diff
==
--- cfe/trunk/include/clang/Lex/Preprocessor.h (original)
+++ cfe/trunk/include/clang/Lex/Preprocessor.h Mon Apr  4 10:30:44 2016
@@ -1943,4 +1943,6 @@ typedef llvm::Registry Pr
 
 }  // end namespace clang
 
+extern template class llvm::Registry;
+
 #endif

Modified: cfe/trunk/lib/Lex/Preprocessor.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/Preprocessor.cpp?rev=265301&r1=265300&r2=265301&view=diff
==
--- cfe/trunk/lib/Lex/Preprocessor.cpp (original)
+++ cfe/trunk/lib/Lex/Preprocessor.cpp Mon Apr  4 10:30:44 2016
@@ -53,6 +53,8 @@
 #include "llvm/Support/raw_ostream.h"
 using namespace clang;
 
+template class llvm::Registry;
+
 
//===--===//
 ExternalPreprocessorSource::~ExternalPreprocessorSource() { }
 


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

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#390237, @alexfh wrote:

> My main concern is that this change makes the already complicated and 
> unobvious testing mechanism [...]


The complexity and obtuseness of the existing testing mechanism is unrelated to 
this changeset.  This changeset doesn't fundamentally change the testing 
mechanism.

> even more complicated and even less obvious for a very little (as I see it 
> now) benefit.


The benefit is that you would now have a consistent mechanism for verifying 
changes made to headers.

> The added functionality supports a very limited use case (a single header)


You have to start somewhere.  Currently there is no mechanism provided at all.  
Saying that this mechanism is not acceptable because it doesn't handle 
arbitrarily complex generalized checking is making the perfect the enemy of the 
good and isn't a reason for preventing this change from being accepted IMO.

> and it does it in a rather hacky way (the use of the new --header-filter flag 
> is not self-documenting and the overall behavior seems "magical" at the first 
> glance).


None of the mechanisms in the testing of these files is self-documenting.  If 
that is the criteria by which all changes are to be measured, then the entire 
existing system has to be thrown out.  Again, this feels like making the 
perfect the enemy of the good.  The behavior for validating header files is the 
same as the existing behavior for validating source files.  They are copied, 
filtered, processed and the processed results are analyzed.  Discrepencies are 
shown as diffs against the original source files.

> There's also an outstanding comment that you didn't seem to have addressed 
> yet.


Which specific comment are you referring to?  Because, just like this comment, 
there's a bunch of discussion in very general terms without specific complaints 
against specific pieces of code that are preventing it from being committed.

> In http://reviews.llvm.org/D17482#378685, @LegalizeAdulthood wrote:

> 

> > In the context of a clang-tidy check, what would be gained by verifying 
> > changes made to multiple headers that isn't already tested by verifying 
> > changes made to a single header?

> 


You haven't answered this question yet.  The existing test mechanism verifies 
only a single source file at a time; I see no reason why verifying a single 
header is such a great imposition of constraints that would prevent existing 
checks from being enhanced to verify their changes on header files.  However, 
should that arise in the future it is a relatively small change to add 
additional header processing to the script.  Again, let's not make the perfect 
the enemy of the good.  There is no reason we cannot improve the code in steps, 
on demand, as the need arises.  This is the essence of agile development.  
YAGNI 

> > At least two of the checks I've already added have complaints that they 
> > don't work on headers.

> 

> 

> That's because you used `isExpansionInMainFile`, which is just not needed 
> there


...and that was because I couldn't write an automated test against header files 
to verify the changes made there.  The whole point of THIS changeset was to 
give me a mechanism for verifying the fixes in a header so that I could address 
those issues in the bug database.

But instead of getting on with fixing it, I've spent 6 weeks waiting for this 
changeset to be reviewed and that discussion has prevented an advancement of 
functionality in the testing framework.

> > While this change was waiting to be reviewed, another check was added that 
> > made changes to headers but had no way to verify them.

> 

> 

> Which check do you mean? Does it need a special treatment of headers or is it 
> just able to make fixes in headers as most other checks?


I already quoted the check in question earlier in this thread; please review 
those messages.


http://reviews.llvm.org/D17482



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

RE: r265218 - [test] Don't use "UNSUPPORTED" in FileCheck prefixes

[Robinson, Paul via cfe-commits]

Is it worth teaching FileCheck about lit-defined keywords, and
reject check-prefixes that end in one of them?
Offhand that would be UNSUPPORTED, RUN, REQUIRES, XFAIL
(I haven't actually gone to look at what lit knows).
--paulr

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Greg Parker via cfe-commits
> Sent: Friday, April 01, 2016 10:29 PM
> To: cfe-commits@lists.llvm.org
> Subject: r265218 - [test] Don't use "UNSUPPORTED" in FileCheck prefixes
> 
> Author: gparker
> Date: Sat Apr  2 00:29:00 2016
> New Revision: 265218
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=265218&view=rev
> Log:
> [test] Don't use "UNSUPPORTED" in FileCheck prefixes
> 
> lit uses "UNSUPPORTED:" for its own purposes and may be
> confused if that text appears elsewhere in the test file.
> 
> Modified:
> cfe/trunk/test/Driver/arc.c
> cfe/trunk/test/Driver/objc-weak.m
> 
> Modified: cfe/trunk/test/Driver/arc.c
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/Driver/arc.c?rev=265218&r1=265217&r2=265218&view=di
> ff
> ==
> 
> --- cfe/trunk/test/Driver/arc.c (original)
> +++ cfe/trunk/test/Driver/arc.c Sat Apr  2 00:29:00 2016
> @@ -3,7 +3,7 @@
>  // RUN: not %clang -x objective-c++ -target i386-apple-darwin10 -m32 -
> fobjc-arc %s -fsyntax-only 2>&1 | FileCheck %s
>  // RUN: not %clang -x c -target i386-apple-darwin10 -m32 -fobjc-arc %s -
> fsyntax-only 2>&1 | FileCheck -check-prefix NOTOBJC %s
>  // RUN: not %clang -x c++ -target i386-apple-darwin10 -m32 -fobjc-arc %s
> -fsyntax-only 2>&1 | FileCheck -check-prefix NOTOBJC %s
> -// RUN: not %clang -x objective-c -target x86_64-apple-darwin11 -mmacosx-
> version-min=10.5 -fobjc-arc %s -fsyntax-only 2>&1 | FileCheck -check-
> prefix UNSUPPORTED %s
> +// RUN: not %clang -x objective-c -target x86_64-apple-darwin11 -mmacosx-
> version-min=10.5 -fobjc-arc %s -fsyntax-only 2>&1 | FileCheck -check-
> prefix NOTSUPPORTED %s
> 
>  // Just to test clang is working.
>  # foo
> @@ -14,4 +14,4 @@
>  // NOTOBJC-NOT: error: -fobjc-arc is not supported on platforms using the
> legacy runtime
>  // NOTOBJC: invalid preprocessing directive
> 
> -// UNSUPPORTED: error: -fobjc-arc is not supported on versions of OS X
> prior to 10.6
> +// NOTSUPPORTED: error: -fobjc-arc is not supported on versions of OS X
> prior to 10.6
> 
> Modified: cfe/trunk/test/Driver/objc-weak.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/objc-
> weak.m?rev=265218&r1=265217&r2=265218&view=diff
> ==
> 
> --- cfe/trunk/test/Driver/objc-weak.m (original)
> +++ cfe/trunk/test/Driver/objc-weak.m Sat Apr  2 00:29:00 2016
> @@ -10,9 +10,9 @@
>  // ARC-NO-WEAK: -fobjc-arc
>  // ARC-NO-WEAK: -fno-objc-weak
> 
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-arc -fobjc-weak 2>&1 | FileCheck %s --check-prefix ARC-WEAK-
> UNSUPPORTED
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fobjc-weak -fobjc-arc  2>&1 | FileCheck %s --check-
> prefix ARC-WEAK-UNSUPPORTED
> -// ARC-WEAK-UNSUPPORTED: error: -fobjc-weak is not supported on the
> current deployment target
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-arc -fobjc-weak 2>&1 | FileCheck %s --check-prefix ARC-WEAK-
> NOTSUPPORTED
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fobjc-weak -fobjc-arc  2>&1 | FileCheck %s --check-
> prefix ARC-WEAK-NOTSUPPORTED
> +// ARC-WEAK-NOTSUPPORTED: error: -fobjc-weak is not supported on the
> current deployment target
> 
>  // RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.7 -S -
> ### %s -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-WEAK
>  // RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.7 -S -
> ### %s -fno-objc-weak -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-
> WEAK
> @@ -22,6 +22,6 @@
>  // RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.7 -S -
> ### %s -fobjc-weak -fno-objc-weak 2>&1 | FileCheck %s --check-prefix MRC-
> NO-WEAK
>  // MRC-NO-WEAK: -fno-objc-weak
> 
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-WEAK-UNSUPPORTED
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-
> WEAK-UNSUPPORTED
> -// MRC-WEAK-UNSUPPORTED: error: -fobjc-weak is not supported on the
> current deployment target
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-WEAK-
> NOTSUPPORTED
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fob

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

[clang-tools-extra] r265303 - [clang-tidy] Reduce false-positive ratio in misc-suspicious-missing-comma check.

[Etienne Bergeron via cfe-commits]

Author: etienneb
Date: Mon Apr  4 10:46:38 2016
New Revision: 265303

URL: http://llvm.org/viewvc/llvm-project?rev=265303&view=rev
Log:
[clang-tidy] Reduce false-positive ratio in misc-suspicious-missing-comma check.

Summary:
This patch is adding detection of common string literal patterns
that should not trigger warnings.

  [*] Add a limit on the number of concatenated token,
  [*] Add support for parenthese sequence of tokens,
  [*] Add detection of valid indentation.

As an example, this code will no longer trigger a warning:
```
const char* Array[] = {
  "first literal"
"indented literal"
"indented literal",
  "second literal",
  [...]
```

Reviewers: alexfh

Subscribers: cfe-commits

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

Modified:
clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.h
clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-missing-comma.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp?rev=265303&r1=265302&r2=265303&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp Mon 
Apr  4 10:46:38 2016
@@ -19,8 +19,51 @@ namespace misc {
 
 namespace {
 
-AST_MATCHER(StringLiteral, isConcatenatedLiteral) {
-  return Node.getNumConcatenated() > 1;
+bool isConcatenatedLiteralsOnPurpose(ASTContext* Ctx,
+ const StringLiteral* Lit) {
+  // String literals surrounded by parentheses are assumed to be on purpose.
+  //i.e.:  const char* Array[] = { ("a" "b" "c"), "d", [...] };
+  auto Parents = Ctx->getParents(*Lit);
+  if (Parents.size() == 1 && Parents[0].get() != nullptr)
+return true;
+
+  // Appropriately indented string literals are assumed to be on purpose.
+  // The following frequent indentation is accepted:
+  // const char* Array[] = {
+  //   "first literal"
+  //   "indented literal"
+  //   "indented literal",
+  //   "second literal",
+  //   [...]
+  // };
+  const SourceManager& SM = Ctx->getSourceManager();
+  bool IndentedCorrectly = true;
+  SourceLocation FirstToken = Lit->getStrTokenLoc(0);
+  FileID BaseFID = SM.getFileID(FirstToken);
+  unsigned int BaseIndent = SM.getSpellingColumnNumber(FirstToken);
+  unsigned int BaseLine = SM.getSpellingLineNumber(FirstToken);
+  for (unsigned int TokNum = 1; TokNum < Lit->getNumConcatenated(); ++ TokNum) 
{
+SourceLocation Token = Lit->getStrTokenLoc(TokNum);
+FileID FID = SM.getFileID(Token);
+unsigned int Indent = SM.getSpellingColumnNumber(Token);
+unsigned int Line = SM.getSpellingLineNumber(Token);
+if (FID != BaseFID || Line != BaseLine + TokNum || Indent <= BaseIndent) {
+  IndentedCorrectly = false;
+  break;
+}
+  }
+  if (IndentedCorrectly)
+return true;
+
+  // There is no pattern recognized by the checker, assume it's not on purpose.
+  return false;
+}
+
+AST_MATCHER_P(StringLiteral, isConcatenatedLiteral,
+  unsigned, MaxConcatenatedTokens) {
+  return Node.getNumConcatenated() > 1 &&
+ Node.getNumConcatenated() < MaxConcatenatedTokens &&
+ !isConcatenatedLiteralsOnPurpose(&Finder->getASTContext(), &Node);
 }
 
 }  // namespace
@@ -29,17 +72,19 @@ SuspiciousMissingCommaCheck::SuspiciousM
 StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   SizeThreshold(Options.get("SizeThreshold", 5U)),
-  RatioThreshold(std::stod(Options.get("RatioThreshold", ".2"))) {}
+  RatioThreshold(std::stod(Options.get("RatioThreshold", ".2"))),
+  MaxConcatenatedTokens(Options.get("MaxConcatenatedTokens", 5U)) {}
 
 void SuspiciousMissingCommaCheck::storeOptions(
 ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "SizeThreshold", SizeThreshold);
   Options.store(Opts, "RatioThreshold", std::to_string(RatioThreshold));
+  Options.store(Opts, "MaxConcatenatedTokens", MaxConcatenatedTokens);
 }
 
 void SuspiciousMissingCommaCheck::registerMatchers(MatchFinder *Finder) {
   const auto ConcatenatedStringLiteral =
-  stringLiteral(isConcatenatedLiteral()).bind("str");
+  stringLiteral(isConcatenatedLiteral(MaxConcatenatedTokens)).bind("str");
 
   const auto StringsInitializerList =
   initListExpr(hasType(constantArrayType()),
@@ -51,9 +96,10 @@ void SuspiciousMissingCommaCheck::regist
 void SuspiciousMissingCommaCheck::check(
 const MatchFinder::MatchResult &Result) {
   const auto *InitializerList = Result.Nodes.getNodeAs("list");
-  const auto *ConcatenatedLiteral = Result.Nodes.getNodeAs("str");
+  const auto *ConcatenatedLiteral =
+  Result.Nodes.getNod

Re: [PATCH] D18695: [clang-tidy] Reduce false-positive ratio in misc-suspicious-missing-comma check.

[Etienne Bergeron via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL265303: [clang-tidy] Reduce false-positive ratio in 
misc-suspicious-missing-comma check. (authored by etienneb).

Changed prior to commit:
  http://reviews.llvm.org/D18695?vs=52365&id=52563#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18695

Files:
  clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
  clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.h
  clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-missing-comma.cpp

Index: clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.h
+++ clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.h
@@ -32,6 +32,8 @@
   const unsigned SizeThreshold;
   // Maximal threshold ratio of suspicious string literals to be considered.
   const double RatioThreshold;
+  // Maximal number of concatenated tokens.
+  const unsigned MaxConcatenatedTokens;
 };
 
 } // namespace misc
Index: clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
@@ -19,27 +19,72 @@
 
 namespace {
 
-AST_MATCHER(StringLiteral, isConcatenatedLiteral) {
-  return Node.getNumConcatenated() > 1;
+bool isConcatenatedLiteralsOnPurpose(ASTContext* Ctx,
+ const StringLiteral* Lit) {
+  // String literals surrounded by parentheses are assumed to be on purpose.
+  //i.e.:  const char* Array[] = { ("a" "b" "c"), "d", [...] };
+  auto Parents = Ctx->getParents(*Lit);
+  if (Parents.size() == 1 && Parents[0].get() != nullptr)
+return true;
+
+  // Appropriately indented string literals are assumed to be on purpose.
+  // The following frequent indentation is accepted:
+  // const char* Array[] = {
+  //   "first literal"
+  //   "indented literal"
+  //   "indented literal",
+  //   "second literal",
+  //   [...]
+  // };
+  const SourceManager& SM = Ctx->getSourceManager();
+  bool IndentedCorrectly = true;
+  SourceLocation FirstToken = Lit->getStrTokenLoc(0);
+  FileID BaseFID = SM.getFileID(FirstToken);
+  unsigned int BaseIndent = SM.getSpellingColumnNumber(FirstToken);
+  unsigned int BaseLine = SM.getSpellingLineNumber(FirstToken);
+  for (unsigned int TokNum = 1; TokNum < Lit->getNumConcatenated(); ++ TokNum) {
+SourceLocation Token = Lit->getStrTokenLoc(TokNum);
+FileID FID = SM.getFileID(Token);
+unsigned int Indent = SM.getSpellingColumnNumber(Token);
+unsigned int Line = SM.getSpellingLineNumber(Token);
+if (FID != BaseFID || Line != BaseLine + TokNum || Indent <= BaseIndent) {
+  IndentedCorrectly = false;
+  break;
+}
+  }
+  if (IndentedCorrectly)
+return true;
+
+  // There is no pattern recognized by the checker, assume it's not on purpose.
+  return false;
+}
+
+AST_MATCHER_P(StringLiteral, isConcatenatedLiteral,
+  unsigned, MaxConcatenatedTokens) {
+  return Node.getNumConcatenated() > 1 &&
+ Node.getNumConcatenated() < MaxConcatenatedTokens &&
+ !isConcatenatedLiteralsOnPurpose(&Finder->getASTContext(), &Node);
 }
 
 }  // namespace
 
 SuspiciousMissingCommaCheck::SuspiciousMissingCommaCheck(
 StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   SizeThreshold(Options.get("SizeThreshold", 5U)),
-  RatioThreshold(std::stod(Options.get("RatioThreshold", ".2"))) {}
+  RatioThreshold(std::stod(Options.get("RatioThreshold", ".2"))),
+  MaxConcatenatedTokens(Options.get("MaxConcatenatedTokens", 5U)) {}
 
 void SuspiciousMissingCommaCheck::storeOptions(
 ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "SizeThreshold", SizeThreshold);
   Options.store(Opts, "RatioThreshold", std::to_string(RatioThreshold));
+  Options.store(Opts, "MaxConcatenatedTokens", MaxConcatenatedTokens);
 }
 
 void SuspiciousMissingCommaCheck::registerMatchers(MatchFinder *Finder) {
   const auto ConcatenatedStringLiteral =
-  stringLiteral(isConcatenatedLiteral()).bind("str");
+  stringLiteral(isConcatenatedLiteral(MaxConcatenatedTokens)).bind("str");
 
   const auto StringsInitializerList =
   initListExpr(hasType(constantArrayType()),
@@ -51,9 +96,10 @@
 void SuspiciousMissingCommaCheck::check(
 const MatchFinder::MatchResult &Result) {
   const auto *InitializerList = Result.Nodes.getNodeAs("list");
-  const auto *ConcatenatedLiteral = Result.Nodes.getNodeAs("str");
+  const auto *ConcatenatedLiteral =
+  Result.Nodes.getNodeAs("str");
   assert(InitializerList && ConcatenatedLiteral);
-
+  
   // Skip small arrays as they often generate false-posit

r265304 - [OPENMP] Codegen for teams directive for NVPTX

[Carlo Bertolli via cfe-commits]

Author: cbertol
Date: Mon Apr  4 10:55:02 2016
New Revision: 265304

URL: http://llvm.org/viewvc/llvm-project?rev=265304&view=rev
Log:
[OPENMP] Codegen for teams directive for NVPTX

This patch implements the teams directive for the NVPTX backend. It is 
different from the host code generation path as it:

Does not call kmpc_fork_teams. All necessary teams and threads are started upon 
touching the target region, when launching a CUDA kernel, and their execution 
is coordinated through sequential and parallel regions within the target region.
Does not call kmpc_push_num_teams even if a num_teams of thread_limit clause is 
present. Setting the number of teams and the thread limit is implemented by the 
nvptx-related runtime.
Please note that I am now passing a Clang Expr * to emitPushNumTeams instead of 
the originally chosen llvm::Value * type. The reason for that is that I want to 
avoid emitting expressions for num_teams and thread_limit if they are not 
needed in the target region.

http://reviews.llvm.org/D17963



Added:
cfe/trunk/test/OpenMP/nvptx_teams_codegen.cpp
Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.h
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp?rev=265304&r1=265303&r2=265304&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp Mon Apr  4 10:55:02 2016
@@ -4832,17 +4832,29 @@ void CGOpenMPRuntime::emitTeamsCall(Code
 }
 
 void CGOpenMPRuntime::emitNumTeamsClause(CodeGenFunction &CGF,
- llvm::Value *NumTeams,
- llvm::Value *ThreadLimit,
+ const Expr *NumTeams,
+ const Expr *ThreadLimit,
  SourceLocation Loc) {
   if (!CGF.HaveInsertPoint())
 return;
 
   auto *RTLoc = emitUpdateLocation(CGF, Loc);
 
+  llvm::Value *NumTeamsVal =
+  (NumTeams)
+  ? CGF.Builder.CreateIntCast(CGF.EmitScalarExpr(NumTeams),
+  CGF.CGM.Int32Ty, /* isSigned = */ true)
+  : CGF.Builder.getInt32(0);
+
+  llvm::Value *ThreadLimitVal =
+  (ThreadLimit)
+  ? CGF.Builder.CreateIntCast(CGF.EmitScalarExpr(ThreadLimit),
+  CGF.CGM.Int32Ty, /* isSigned = */ true)
+  : CGF.Builder.getInt32(0);
+
   // Build call __kmpc_push_num_teamss(&loc, global_tid, num_teams, 
thread_limit)
-  llvm::Value *PushNumTeamsArgs[] = {
-  RTLoc, getThreadID(CGF, Loc), NumTeams, ThreadLimit};
+  llvm::Value *PushNumTeamsArgs[] = {RTLoc, getThreadID(CGF, Loc), NumTeamsVal,
+ ThreadLimitVal};
   CGF.EmitRuntimeCall(createRuntimeFunction(OMPRTL__kmpc_push_num_teams),
   PushNumTeamsArgs);
 }

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h?rev=265304&r1=265303&r2=265304&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h Mon Apr  4 10:55:02 2016
@@ -912,11 +912,10 @@ public:
   /// \brief Emits call to void __kmpc_push_num_teams(ident_t *loc, kmp_int32
   /// global_tid, kmp_int32 num_teams, kmp_int32 thread_limit) to generate code
   /// for num_teams clause.
-  /// \param NumTeams An integer value of teams.
-  /// \param ThreadLimit An integer value of threads.
-  virtual void emitNumTeamsClause(CodeGenFunction &CGF, llvm::Value *NumTeams,
-  llvm::Value *ThreadLimit, SourceLocation 
Loc);
-
+  /// \param NumTeams An integer expression of teams.
+  /// \param ThreadLimit An integer expression of threads.
+  virtual void emitNumTeamsClause(CodeGenFunction &CGF, const Expr *NumTeams,
+  const Expr *ThreadLimit, SourceLocation Loc);
 };
 
 } // namespace CodeGen

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp?rev=265304&r1=265303&r2=265304&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp Mon Apr  4 10:55:02 2016
@@ -14,6 +14,8 @@
 
 #include "CGOpenMPRuntimeNVPTX.h"
 #include "clang/AST/DeclOpenMP.h"
+#include "CodeGenFunction.h"
+#include "clang/AST/StmtOpenMP.h"
 
 using namespace clang;
 using namespace CodeGen;
@@ -350,3 +352,45 

Re: [PATCH] D17963: [OPENMP] Codegen for teams directive for NVPTX

[Carlo Bertolli via cfe-commits]

carlo.bertolli closed this revision.
carlo.bertolli added a comment.

Committed revision 265304.


Repository:
  rL LLVM

http://reviews.llvm.org/D17963



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

Re: [PATCH] D18752: Documented standard substitutions defined by lit

[Paul Robinson via cfe-commits]

probinson added a subscriber: probinson.
probinson added a comment.

In http://reviews.llvm.org/D18752#390906, @RedX2501 wrote:

> Somebody else needs to commit this as I don't have any rights for it.


When someone else commits for you, our practice is to attribute the correct 
author in the commit message.
This is more than a courtesy, because you retain copyright of any changes you 
have made.  Therefore we prefer to use your actual name rather than a handle 
such as RedX2501, guibufolo, or Guilherme.
What name would you like us to use?


http://reviews.llvm.org/D18752



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

Re: [PATCH] D18396: Clang-tidy:modernize-use-override. Fix for __declspec attributes and const=0 without spacse

[Robert Bolter via cfe-commits]

Rob added a comment.

Thanks :)


Repository:
  rL LLVM

http://reviews.llvm.org/D18396



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

Re: [PATCH] D18752: Documented standard substitutions defined by lit

[guibufolo+l...@gmail.com via cfe-commits]

RedX2501 added a comment.

Thanks for the advice.

Please use Guilherme Bufolo as the name for the commit.
Am 04.04.2016 21:33 schrieb "Paul Robinson" <
paul_robin...@playstation.sony.com>:

> probinson added a subscriber: probinson.

>  probinson added a comment.

> 

> In http://reviews.llvm.org/D18752#390906, @RedX2501 wrote:

> 

> > Somebody else needs to commit this as I don't have any rights for it.

> 

> When someone else commits for you, our practice is to attribute the

>  correct author in the commit message.

>  This is more than a courtesy, because you retain copyright of any changes

>  you have made.  Therefore we prefer to use your actual name rather than a

>  handle such as RedX2501, guibufolo, or Guilherme.

>  What name would you like us to use?

> 

> http://reviews.llvm.org/D18752



http://reviews.llvm.org/D18752



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

[libcxx] r265306 - Fix for Bug #27193; 'std::acos on complex does not agree with C'. Tests need work; so the bug will stay open.

[Marshall Clow via cfe-commits]

Author: marshall
Date: Mon Apr  4 11:08:54 2016
New Revision: 265306

URL: http://llvm.org/viewvc/llvm-project?rev=265306&view=rev
Log:
Fix for Bug #27193; 'std::acos on complex does not agree with C'. Tests need 
work; so the bug will stay open.


Modified:
libcxx/trunk/include/complex

Modified: libcxx/trunk/include/complex
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/complex?rev=265306&r1=265305&r2=265306&view=diff
==
--- libcxx/trunk/include/complex (original)
+++ libcxx/trunk/include/complex Mon Apr  4 11:08:54 2016
@@ -1399,7 +1399,7 @@ acos(const complex<_Tp>& __x)
 }
 if (isinf(__x.imag()))
 return complex<_Tp>(__pi/_Tp(2), -__x.imag());
-if (__x.real() == 0)
+if (__x.real() == 0 && (__x.imag() == 0 || isnan(__x.imag(
 return complex<_Tp>(__pi/_Tp(2), -__x.imag());
 complex<_Tp> __z = log(__x + sqrt(pow(__x, _Tp(2)) - _Tp(1)));
 if (signbit(__x.imag()))


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

Re: [PATCH] D18745: [clang-tidy] Adds misc-use-bool-literals check.

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In http://reviews.llvm.org/D18745#390739, @Eugene.Zelenko wrote:

> Isn't readability-implicit-bool-cast¶ should catch such issues? If not, I 
> think will be good idea to improve that check instead of introducing new one.


I wouldn't add this functionality there. I see that 
readability-implicit-bool-cast aims much different problem.


http://reviews.llvm.org/D18745



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

Re: [PATCH] D18698: [C11] PR24451: Allow restrict _Atomic pointers

[Denis Zobnin via cfe-commits]

d.zobnin.bugzilla added a comment.

Richard, thank you for the review and explanation!
I don't have any examples of useful code using this extension, I was trying to 
fix the PR... So, do you think we should add this GCC extension to Clang (with 
a proper warning, of course) for compatibility reasons, or it will be better to 
close PR24451 and abandon this review?

Thank you,
Denis Zobnin


http://reviews.llvm.org/D18698



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

[PATCH] D18761: [mips] Enable IAS by default for 32-bit MIPS targets (O32).

[Daniel Sanders via cfe-commits]

dsanders created this revision.
dsanders added a reviewer: vkalintiris.
dsanders added a subscriber: cfe-commits.
dsanders added a dependency: D18753: [mips][sanitizer_common] Don't use `ld` in 
internal_clone() on 32-bit MIPS..

The MIPS IAS can now pass 'ninja check-all' and recurse now that the immediates
are all range checked properly.

Unfortunately we can't enable it by default for 64-bit targets yet since the N32
ABI is still very buggy and this also means we can't enable it for N64 either
because we can't distinguish between N32 and N64 in the relevant code.

Depends on D18753

http://reviews.llvm.org/D18761

Files:
  lib/Driver/ToolChains.cpp

Index: lib/Driver/ToolChains.cpp
===
--- lib/Driver/ToolChains.cpp
+++ lib/Driver/ToolChains.cpp
@@ -2396,6 +2396,8 @@
   case llvm::Triple::ppc64:
   case llvm::Triple::ppc64le:
   case llvm::Triple::systemz:
+  case llvm::Triple::mips:
+  case llvm::Triple::mipsel:
 return true;
   default:
 return false;


Index: lib/Driver/ToolChains.cpp
===
--- lib/Driver/ToolChains.cpp
+++ lib/Driver/ToolChains.cpp
@@ -2396,6 +2396,8 @@
   case llvm::Triple::ppc64:
   case llvm::Triple::ppc64le:
   case llvm::Triple::systemz:
+  case llvm::Triple::mips:
+  case llvm::Triple::mipsel:
 return true;
   default:
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D18764: [clang-tidy] Fix documentation of misc-suspicious-missing-comma

[Etienne Bergeron via cfe-commits]

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

The clang-tidy documentation generation was broken since commit : 
http://reviews.llvm.org/D18457

I ran locally the documentation generation and I fixed errors related to that 
specific check.

http://reviews.llvm.org/D18764

Files:
  docs/clang-tidy/checks/misc-suspicious-missing-comma.rst

Index: docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
===
--- docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
+++ docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
@@ -7,14 +7,19 @@
 (after the preprocessor). This feature is used to represent long string
 literal on multiple lines.
 
-For instance, these declarations are equivalent:
+For instance, the following declarations are equivalent:
+
+.. code:: c++
+
   const char* A[] = "This is a test";
-  const char* B[] = "This" " is a "
-"test";
+  const char* B[] = "This" " is a ""test";
 
+
 A common mistake done by programmers is to forget a comma between two string
 literals in an array initializer list.
 
+.. code:: c++
+
   const char* Test[] = {
 "line 1",
 "line 2" // Missing comma!
@@ -23,13 +28,17 @@
 "line 5"
   };
 
+
 The array contains the string "line 2line3" at offset 1 (i.e. Test[1]). Clang
 won't generate warnings at compile time.
 
 This checker may warn incorrectly on cases like:
 
+.. code:: c++
+
   const char* SupportedFormat[] = {
 "Error %s",
 "Code " PRIu64,   // May warn here.
 "Warning %s",
   };
+


Index: docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
===
--- docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
+++ docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
@@ -7,14 +7,19 @@
 (after the preprocessor). This feature is used to represent long string
 literal on multiple lines.
 
-For instance, these declarations are equivalent:
+For instance, the following declarations are equivalent:
+
+.. code:: c++
+
   const char* A[] = "This is a test";
-  const char* B[] = "This" " is a "
-"test";
+  const char* B[] = "This" " is a ""test";
 
+
 A common mistake done by programmers is to forget a comma between two string
 literals in an array initializer list.
 
+.. code:: c++
+
   const char* Test[] = {
 "line 1",
 "line 2" // Missing comma!
@@ -23,13 +28,17 @@
 "line 5"
   };
 
+
 The array contains the string "line 2line3" at offset 1 (i.e. Test[1]). Clang
 won't generate warnings at compile time.
 
 This checker may warn incorrectly on cases like:
 
+.. code:: c++
+
   const char* SupportedFormat[] = {
 "Error %s",
 "Code " PRIu64,   // May warn here.
 "Warning %s",
   };
+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18747: [Support] Fix an invalid character escaping in string literal (unittest).

[Reid Kleckner via cfe-commits]

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

Lgtm I suppose the test worked fine because it still found the right magic 
bytes.


http://reviews.llvm.org/D18747



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

Re: [PATCH] D18752: Documented standard substitutions defined by lit

[Paul Robinson via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL265314: Document standard substitutions defined by lit. 
(authored by probinson).

Changed prior to commit:
  http://reviews.llvm.org/D18752?vs=52531&id=52578#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18752

Files:
  llvm/trunk/docs/CommandGuide/lit.rst

Index: llvm/trunk/docs/CommandGuide/lit.rst
===
--- llvm/trunk/docs/CommandGuide/lit.rst
+++ llvm/trunk/docs/CommandGuide/lit.rst
@@ -355,6 +355,35 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various patterns that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t temporary file name unique to the test
+ %T temporary directory unique to the test
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
+More information on the testing infrastucture can be found in the
+:doc:`../TestingGuide`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 


Index: llvm/trunk/docs/CommandGuide/lit.rst
===
--- llvm/trunk/docs/CommandGuide/lit.rst
+++ llvm/trunk/docs/CommandGuide/lit.rst
@@ -355,6 +355,35 @@
 configuration parameters --- for example, to change the test format, or the
 suffixes which identify test files.
 
+PRE-DEFINED SUBSTITUTIONS
+~~
+
+:program:`lit` provides various patterns that can be used with the RUN command.
+These are defined in TestRunner.py.
+
+ == ==
+  Macro  Substitution
+ == ==
+ %s source path (path to the file currently being run)
+ %S source dir (directory of the file currently being run)
+ %p same as %S
+ %{pathsep} path separator
+ %t temporary file name unique to the test
+ %T temporary directory unique to the test
+ %% %
+ %/ssame as %s but replace all / with \\
+ %/Ssame as %S but replace all / with \\
+ %/psame as %p but replace all / with \\
+ %/tsame as %t but replace all / with \\
+ %/Tsame as %T but replace all / with \\
+ == ==
+
+Further substitution patterns might be defined by each test module.
+See the modules :ref:`local-configuration-files`.
+
+More information on the testing infrastucture can be found in the
+:doc:`../TestingGuide`.
+
 TEST RUN OUTPUT FORMAT
 ~~
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18425: [Sema] Make enable_if act correctly with value dependent conditions/arguments

[George Burgess IV via cfe-commits]

george.burgess.iv added a comment.

Ping ;)


http://reviews.llvm.org/D18425



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

Re: [PATCH] D18540: [Sema] Note when we've actually encountered a failure in ExprConstant, and take that into account when looking up objects.

[George Burgess IV via cfe-commits]

george.burgess.iv added a comment.

Ping :)


http://reviews.llvm.org/D18540



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

Re: [PATCH] D18398: Compilation for Intel MCU (Part 1/3)

[Bruno Cardoso Lopes via cfe-commits]

bruno added a subscriber: bruno.
bruno added a comment.

Hi Andrey,



Comment at: include/clang/Driver/Options.td:1281
@@ -1280,1 +1280,3 @@
+def miamcu : Flag<["-"], "miamcu">, Group, Flags<[DriverOption, 
CoreOption]>,
+  HelpText<"Use Intel MCU ABI.">;
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, 
Group;

You can remove the "." to be consistent with the other flags help text.


Comment at: lib/Driver/Driver.cpp:283
@@ +282,3 @@
+  continue;
+}
+

You probably don't need this - turning -static on as you do some lines below 
should be enough to output a warning if -dynamic is specified - see 
CheckCodeGenerationOptions.


Comment at: lib/Driver/Driver.cpp:387
@@ +386,3 @@
+  D.Diag(diag::err_drv_argument_not_allowed_with)
+  << "-miamcu" << A->getBaseArg().getAsString(Args);
+

What happens if -m32 isn't specified and the target is != x86?


Comment at: lib/Driver/Tools.cpp:2130
@@ -2122,3 +2129,3 @@
 }
 
 void Clang::AddHexagonTargetArgs(const ArgList &Args,

One potential place you could be missing: maybe getX86TargetCPU should return 
Lakemont when OPT_miamcu is on? or is this coming in another patch?


http://reviews.llvm.org/D18398



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

Re: [PATCH] D18713: [OpenCL] Generate bitcast when target address space does not change.

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

LG, apart from renaming the test file.

Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with 
CreateAddrSpaceCast call too. I am wondering if that could have the same 
issue... if yes, may be we should fix it already now.



Comment at: test/CodeGenOpenCL/2016-04-01-addrcast.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 %s -triple x86_64-unknown-linux-gnu -O0 -cl-std=CL2.0 
-emit-llvm -o - | FileCheck %s
+

Can we rename the file to just contain the name i.e. something like 
addrspacecast.cl would be fine.

Actually, we also have similar functionality in 
test/CodeGenOpenCL/address-spaces-conversions.cl. Do you think it might make 
sense to integrate? Let's say to have two RUN lines with and w/o 
-ffake-address-space-map.


http://reviews.llvm.org/D18713



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

clang-format: fix for 19986, extra spacing around c++14 lambda capture with initializer

[Jacek Sieka via cfe-commits]

Hello,

Here's a little one-off patch that fixes
https://llvm.org/bugs/show_bug.cgi?id=19986 for me, by allowing a few
more things inside [] when trying to match a lambda capture section.

I hope someone can help it find its way into the main repo.

Cheers,
Jacek
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp  (revision 263181)
+++ lib/Format/UnwrappedLineParser.cpp  (working copy)
@@ -1080,8 +1080,25 @@
 if (!FormatTok->isOneOf(tok::identifier, tok::kw_this))
   return false;
 nextToken();
-if (FormatTok->is(tok::ellipsis))
+if (FormatTok->is(tok::equal)) {
   nextToken();
+  int parens = 0;
+  while (!eof()) {
+if (FormatTok->isOneOf(tok::l_paren, tok::l_square)) {
+  ++parens;
+} else if (parens && FormatTok->isOneOf(tok::r_paren, tok::r_square)) {
+  --parens;
+} else if (FormatTok->isOneOf(tok::comma, tok::r_square)) {
+  break;
+} else if (!FormatTok->isOneOf(tok::identifier, tok::coloncolon)) {
+  return false;
+}
+
+nextToken();
+  }
+} else if (FormatTok->is(tok::ellipsis)) {
+  nextToken();
+}
 if (FormatTok->is(tok::comma)) {
   nextToken();
 } else if (FormatTok->is(tok::r_square)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator

[Anastasia Stulova via cfe-commits]

Anastasia added a subscriber: Matt.
Anastasia added a comment.

Cool, thanks! I think we should commit this ASAP.

@Xiuli/@Matt, do you have any more comments here?


http://reviews.llvm.org/D17412



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

Re: [PATCH] D17412: PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator

[Matt D. via cfe-commits]

Matt added a comment.

In http://reviews.llvm.org/D17412#391322, @Anastasia wrote:

> Cool, thanks! I think we should commit this ASAP.
>
> @Xiuli/@Matt, do you have any more comments here?


Hi! I think that "Matt" for this one would be @arsenm :-)


http://reviews.llvm.org/D17412



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

[PATCH] D18765: [clang-tidy] Don't complain about const pass_object_size params in declarations

[George Burgess IV via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added a reviewer: alexfh.
george.burgess.iv added a subscriber: cfe-commits.

This patch seems trivial, but I've never touched clang-tidy before, so I'm just 
making sure I didn't miss something obvious. :)

--

Clang has a parameter attribute called `pass_object_size`, which requires that 
its parameter's type be `const`. This restriction only applies at function 
definitions. e.g.

```
void foo(void *p __attribute__((pass_object_size(0))); // this is a decl; const 
isn't required.
void foo(void *const p __attribute__((pass_object_size(0))) {} // const is 
required; this is a def.
```

readability-avoid-const-params-in-decls will complain if you put `const` in the 
decl. Given that clang gives you an error unless you use `const` in the def, I 
don't think it's clearly a bad thing to use `const` in the decl. This patch 
makes said checker not complain about `const` params with the 
`pass_object_size` attribute.

http://reviews.llvm.org/D18765

Files:
  clang-tidy/readability/AvoidConstParamsInDecls.cpp
  test/clang-tidy/readability-avoid-const-params-in-decls.cpp

Index: test/clang-tidy/readability-avoid-const-params-in-decls.cpp
===
--- test/clang-tidy/readability-avoid-const-params-in-decls.cpp
+++ test/clang-tidy/readability-avoid-const-params-in-decls.cpp
@@ -65,6 +65,8 @@
 void NF6(const int *const) {}
 void NF7(int, const int) {}
 void NF8(const int, const int) {}
+// pass_object_size requires a const pointer param in definitions.
+void NF9(const char *const __attribute__((pass_object_size(0 {}
 
 // Do not match on other stuff
 void NF(const alias_type& i);
@@ -76,3 +78,6 @@
 void NF(const int*);
 void NF(const int* const*);
 void NF(alias_const_type);
+// While pass_object_size only requires a const pointer param in definitions,
+// it's probably best to not complain at users for using it in a decl.
+void NF(const char *const __attribute__((pass_object_size(0;
Index: clang-tidy/readability/AvoidConstParamsInDecls.cpp
===
--- clang-tidy/readability/AvoidConstParamsInDecls.cpp
+++ clang-tidy/readability/AvoidConstParamsInDecls.cpp
@@ -31,7 +31,8 @@
 
 void AvoidConstParamsInDecls::registerMatchers(MatchFinder *Finder) {
   const auto ConstParamDecl =
-  parmVarDecl(hasType(qualType(isConstQualified(.bind("param");
+  parmVarDecl(hasType(qualType(isConstQualified())),
+  unless(hasAttr(clang::attr::PassObjectSize))).bind("param");
   Finder->addMatcher(functionDecl(unless(isDefinition()),
   has(typeLoc(forEach(ConstParamDecl
  .bind("func"),


Index: test/clang-tidy/readability-avoid-const-params-in-decls.cpp
===
--- test/clang-tidy/readability-avoid-const-params-in-decls.cpp
+++ test/clang-tidy/readability-avoid-const-params-in-decls.cpp
@@ -65,6 +65,8 @@
 void NF6(const int *const) {}
 void NF7(int, const int) {}
 void NF8(const int, const int) {}
+// pass_object_size requires a const pointer param in definitions.
+void NF9(const char *const __attribute__((pass_object_size(0 {}
 
 // Do not match on other stuff
 void NF(const alias_type& i);
@@ -76,3 +78,6 @@
 void NF(const int*);
 void NF(const int* const*);
 void NF(alias_const_type);
+// While pass_object_size only requires a const pointer param in definitions,
+// it's probably best to not complain at users for using it in a decl.
+void NF(const char *const __attribute__((pass_object_size(0;
Index: clang-tidy/readability/AvoidConstParamsInDecls.cpp
===
--- clang-tidy/readability/AvoidConstParamsInDecls.cpp
+++ clang-tidy/readability/AvoidConstParamsInDecls.cpp
@@ -31,7 +31,8 @@
 
 void AvoidConstParamsInDecls::registerMatchers(MatchFinder *Finder) {
   const auto ConstParamDecl =
-  parmVarDecl(hasType(qualType(isConstQualified(.bind("param");
+  parmVarDecl(hasType(qualType(isConstQualified())),
+  unless(hasAttr(clang::attr::PassObjectSize))).bind("param");
   Finder->addMatcher(functionDecl(unless(isDefinition()),
   has(typeLoc(forEach(ConstParamDecl
  .bind("func"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 {
+publi

Re: [PATCH] D17933: Set MaxAtomicInlineWidth properly for i386, i486, and x86-64 cpus without cmpxchg16b.

[James Y Knight via cfe-commits]

jyknight added a comment.

Done and done.


http://reviews.llvm.org/D17933



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

Re: [PATCH] D17933: Set MaxAtomicInlineWidth properly for i386, i486, and x86-64 cpus without cmpxchg16b.

[James Y Knight via cfe-commits]

jyknight updated this revision to Diff 52587.
jyknight marked 2 inline comments as done.
jyknight added a comment.

review fixes


http://reviews.llvm.org/D17933

Files:
  lib/Basic/Targets.cpp
  lib/Frontend/InitPreprocessor.cpp
  test/CodeGen/atomic-ops.c
  test/CodeGen/ms-volatile.c
  test/CodeGenCXX/atomicinit.cpp
  test/OpenMP/atomic_capture_codegen.cpp
  test/OpenMP/atomic_read_codegen.c
  test/OpenMP/atomic_update_codegen.cpp
  test/OpenMP/atomic_write_codegen.c
  test/Preprocessor/arm-target-features.c
  test/Preprocessor/init.c
  test/Preprocessor/predefined-arch-macros.c
  test/Preprocessor/predefined-macros.c
  test/Preprocessor/x86_target_features.c
  test/Sema/atomic-ops.c

Index: test/Sema/atomic-ops.c
===
--- test/Sema/atomic-ops.c
+++ test/Sema/atomic-ops.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -std=c11
+// RUN: %clang_cc1 %s -verify -ffreestanding -fsyntax-only -triple=i686-linux-gnu -target-cpu i686 -std=c11
 
 // Basic parsing/Sema tests for __c11_atomic_*
 
@@ -502,5 +502,3 @@
   (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_acq_rel, memory_order_relaxed);
   (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_relaxed);
 }
-
-
Index: test/Preprocessor/x86_target_features.c
===
--- test/Preprocessor/x86_target_features.c
+++ test/Preprocessor/x86_target_features.c
@@ -291,8 +291,13 @@
 
 // NOTBM-NOT: #define __TBM__ 1
 
-// RUN: %clang -target i386-unknown-unknown -march=pentiumpro -mcx16 -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=MCX16 %s
+// RUN: %clang -target i386-unknown-unknown -march=pentiumpro -mcx16 -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=MCX16-386 %s
 
+// MCX16-386-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16
+
+// RUN: %clang -target x86_64-unknown-unknown -mcx16 -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=MCX16 %s
+
+// MCX16: #define __GCC_ATOMIC_LLONG_LOCK_FREE 2
 // MCX16: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 1
 
 // RUN: %clang -target i386-unknown-unknown -march=atom -mprfchw -x c -E -dM -o - %s | FileCheck -match-full-lines --check-prefix=PRFCHW %s
Index: test/Preprocessor/predefined-macros.c
===
--- test/Preprocessor/predefined-macros.c
+++ test/Preprocessor/predefined-macros.c
@@ -102,47 +102,117 @@
 // RUN: %clang_cc1 %s -E -dM -o - -triple i686 -target-cpu i386 \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-SYNC_CAS_I386
 // CHECK-SYNC_CAS_I386-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP
+// CHECK-SYNC_CAS_I386-NOT: __GCC_ATOMIC_{{.*}}_LOCK_FREE 2
 //
 // RUN: %clang_cc1 %s -E -dM -o - -triple i686 -target-cpu i486 \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-SYNC_CAS_I486
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_BOOL_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_CHAR_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_INT_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_LLONG_LOCK_FREE 1
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_LONG_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_POINTER_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_SHORT_LOCK_FREE 2
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_TEST_AND_SET_TRUEVAL 1
+// CHECK-SYNC_CAS_I486: #define __GCC_ATOMIC_WCHAR_T_LOCK_FREE 2
 // CHECK-SYNC_CAS_I486: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
 // CHECK-SYNC_CAS_I486: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
 // CHECK-SYNC_CAS_I486: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
 // CHECK-SYNC_CAS_I486-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8
+// CHECK-SYNC_CAS_I486-NOT: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16
 //
+
+// FIXME: The value of LLONG_LOCK_FREE on i586 is wrong: clang is
+// using the alignment of "long long" (only 32bit for a 64bit value)
+// to disqualify it from being lock free. But the semantics of this
+// define are supposed to tell you if "_Atomic long long" is lock free
+// -- and it is.
+
 // RUN: %clang_cc1 %s -E -dM -o - -triple i686 -target-cpu i586 \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-SYNC_CAS_I586
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_BOOL_LOCK_FREE 2
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_CHAR_LOCK_FREE 2
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_INT_LOCK_FREE 2
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_LLONG_LOCK_FREE 1
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_LONG_LOCK_FREE 2
+// CHECK-SYNC_CAS_I586: #define __GCC_ATOMIC_POINTER_LOCK_FREE 2
+// CHECK-SYNC

Re: [PATCH] D18540: [Sema] Note when we've actually encountered a failure in ExprConstant, and take that into account when looking up objects.

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: lib/AST/ExprConstant.cpp:782
@@ +781,3 @@
+LLVM_ATTRIBUTE_UNUSED_RESULT bool noteUnrecoverableFailure() {
+  // This is a little bit icky. We leave HasSideEffects unchange if we
+  // aren't going to keep running, because eval modes that don't allow

unchange -> unchanged


Comment at: lib/AST/ExprConstant.cpp:784-785
@@ +783,4 @@
+  // aren't going to keep running, because eval modes that don't allow
+  // evaluation after side-effects/failure sometimes depend on the value of
+  // HasSideEffects. This is why this function should only be called if we
+  // intend to keep evaluating.

I think this is the right behavior regardless of the current expectations of 
any particular caller. After this patch:

  * `HasSideEffects == false` indicates that the evaluation (if evaluation was 
successful, then up to the current point, otherwise up to the point where 
evaluation failed) has definitely not skipped any side-effects
  * `HasSideEffects == true` means nothing is known about side-effects (in 
particular, it does not guarantee that a side-effect has definitely happened, 
just that one /might/ have happened)

(If any caller expects these flags to mean something else, they are wrong -- 
and note that `HasSideEffects` is meaningless if evaluation fails.) Therefore, 
if we transition from an "evaluation has failed" to an "evaluation was 
successful" state, we must set `HasSideEffects` to `true` to model the 
subexpressions we might have skipped. And if we just unwind (because 
`keepEvaluatingAfterFailure()` returns `false`), then we don't need to set 
`HasSideEffects` at all.

Perhaps you could say something like this:

"Failure when evaluating some expression often means there is some 
subexpression whose evaluation was skipped. Therefore, (because we don't track 
whether we skipped an expression when unwinding after an evaluation failure) 
every evaluation failure that bubbles up from a subexpression implies that a 
side-effect has potentially happened. We skip setting the HasSideEffects flag 
to true until we decide to continue evaluating after that point, which happens 
here."


Comment at: lib/AST/ExprConstant.cpp:853-854
@@ -825,5 +852,4 @@
   Info.EvalStatus.Diag = NewDiag;
   // If we're speculatively evaluating, we may have skipped over some
   // evaluations and missed out a side effect.
 }

I think this comment is now redundant / incorrect.


Comment at: lib/AST/ExprConstant.cpp:2643
@@ +2642,3 @@
+  // Don't allow the speculative evaluation of writes.
+  if (AK != AK_Read && Info.IsSpeculativelyEvaluating)
+return CompleteObject();

`IsSpeculativelyEvaluating` is always `false`. Presumably it should be set to 
`true` in the `SpeculativeEvaluationRAII` constructor. (Please also add a 
testcase to catch this...)


Comment at: lib/AST/ExprConstant.cpp:3280
@@ -3249,3 +3279,3 @@
   if (!EvaluateObjectArgument(Info, BO->getLHS(), LV)) {
-if (Info.keepEvaluatingAfterFailure()) {
+if (Info.noteUnrecoverableFailure()) {
   MemberPtr MemPtr;

Can we give this function a name that reads more naturally at the call site? 
It's locally unclear what this condition is testing. Maybe keep the old name?


Comment at: lib/AST/ExprConstant.cpp:4074-4076
@@ -4043,2 +4073,5 @@
 assert(Info.checkingPotentialConstantExpression());
+assert(
+Info.EvalStatus.HasSideEffects &&
+"Need multiple speculative eval RAIIs below if we haven't failed 
yet.");
 

These RAII objects are cheap; maybe just create two of them regardless?


Comment at: lib/AST/ExprConstant.cpp:4104-4105
@@ -4069,1 +4103,4 @@
+(void)KeepGoing;
+assert(KeepGoing && "We can't evaluate after a failure in potential "
+"constant expressions?");
 CheckPotentialConstantConditional(E);

I think this should be an `if` rather than an `assert`. If we hit something 
that's known to be non-constant while evaluating the condition of a `?:`, we 
should bail out of evaluation in `checkingPotentialConstantExpression` mode. 
`keepEvaluatingAfterFailure` should return `false` in that case (even though it 
doesn't do so today).


Comment at: lib/AST/ExprConstant.cpp:7374-7375
@@ -7335,4 +7373,4 @@
 bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
-  if (!Info.keepEvaluatingAfterFailure() && E->isAssignmentOp())
+  if (E->isAssignmentOp() && !Info.noteUnrecoverableFailure())
 return Error(E);
 

The side-effect here should be delayed until after we've evaluated the left and 
right hand sides of the assignment.


http://reviews.llvm.org/D18540



___
cfe-commits mailing list
cfe-commit

[PATCH] D18768: Refactoring attribute subject diagnostics

[Aaron Ballman via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, doug.gregor.
aaron.ballman added a subscriber: cfe-commits.

Currently, attribute subject lists attempt to automatically determine what 
diagnostic to emit when the attribute does not appertain to a given 
declaration. It does so by the attribute tablegen emitter looking at the 
subject list and attempting to match it up with a viable enumerator in 
AttributeList.h. This approach is starting to show signs of wear now that we 
are getting more complex attribute subject lists. It also runs into problems 
where users don't want to add a one-off enumerator and instead use an existing 
close-but-not-quite-accurate diagnostic.

This patch attempts to improve the situation by removing the need for the 
enumeration and complex selection logic in the diagnostics tablegen. Instead, 
each declaration and statement node tablegen description includes a 
comma-separated list of subjects for diagnostics that the attribute tablegen 
emitter uses to create a single string to pass to a new diagnostic. This 
approach is not ideal -- ideally we would have a "list" notion for the 
diagnostics engine that could handle properly formatting a list so that it 
handles multiple elements for us. However, I think my current approach is an 
incremental improvement that gets us a step closer to being able to use such a 
list mechanism were it to be implemented in the future.

Before I correct all of the test cases for minor diagnostic differences, I 
wanted to point out those differences explicitly:

* The following were renamed to reduce confusion:
  * methods -> Objective-C methods
  * interfaces -> Objective-C interfaces
  * protocols -> Objective-C protocols
  * fields -> non-static data members
  * types -> typedefs
* We used to consider CXXRecordDecl to only be "classes" while RecordDecl was 
"struct, union, or class"; we would drop the "or class" part when not compiling 
in C++ mode. Now that we've switched to a string literal, dropping (or adding) 
"classes" is more challenging and so RecordDecl always reports "classes" under 
the assumption that it won't be overly confusing to C users. I am also banking 
on there being more C++ users than C users, but an alternative may be to drop 
"classes" from RecordDecl under the assumption that users understand "struct" 
is the effectively same thing as a "class" in C++.

Note that the actual declarations the attribute appertains to haven't changed, 
just the spelling of the diagnostics.

If the general approach taken here is reasonable, then I will update the patch 
with corrected test cases and other polish-related fixes. But since it affects 
~30 test cases (due to the updated terminology, consistent use of 'and' instead 
of 'or', etc), I didn't want to go through the effort unless the patch is going 
in a reasonable direction.

http://reviews.llvm.org/D18768

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/DeclNodes.td
  include/clang/Basic/DiagnosticSemaKinds.td
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2419,120 +2419,72 @@
   OS << "}\n\n";
 }
 
+static std::string GetDiagnosticSpelling(const Record &R) {
+  std::string Ret = R.getValueAsString("DiagSpelling");
+  if (!Ret.empty())
+return Ret;
+
+  // If we couldn't find the DiagSpelling in this object, we can check to see
+  // if the object is one that has a base, and if it is, loop up to the Base
+  // member recursively.
+  std::string Super = R.getSuperClasses().back().first->getName();
+  if (Super == "DDecl" || Super == "DStmt")
+return GetDiagnosticSpelling(*R.getValueAsDef("Base"));
+
+  return "";
+}
+
 static std::string CalculateDiagnostic(const Record &S) {
   // If the SubjectList object has a custom diagnostic associated with it,
   // return that directly.
   std::string CustomDiag = S.getValueAsString("CustomDiag");
   if (!CustomDiag.empty())
-return CustomDiag;
+return '"' + CustomDiag + '"';
 
-  // Given the list of subjects, determine what diagnostic best fits.
-  enum {
-Func = 1U << 0,
-Var = 1U << 1,
-ObjCMethod = 1U << 2,
-Param = 1U << 3,
-Class = 1U << 4,
-GenericRecord = 1U << 5,
-Type = 1U << 6,
-ObjCIVar = 1U << 7,
-ObjCProp = 1U << 8,
-ObjCInterface = 1U << 9,
-Block = 1U << 10,
-Namespace = 1U << 11,
-Field = 1U << 12,
-CXXMethod = 1U << 13,
-ObjCProtocol = 1U << 14,
-Enum = 1U << 15
-  };
-  uint32_t SubMask = 0;
-
+  std::vector DiagList;
   std::vector Subjects = S.getValueAsListOfDefs("Subjects");
   for (const auto *Subject : Subjects) {
 const Record &R = *Subject;
-std::string Name;
-
-if (R.isSubClassOf("SubsetSubject")) {
-  PrintError(R.getLoc(), "SubsetSubjects should use a custom diagnostic");
-  // As a fallback, lo

r265324 - IRGen-level lowering for the Swift calling convention.

[John McCall via cfe-commits]

Author: rjmccall
Date: Mon Apr  4 13:33:08 2016
New Revision: 265324

URL: http://llvm.org/viewvc/llvm-project?rev=265324&view=rev
Log:
IRGen-level lowering for the Swift calling convention.

Added:
cfe/trunk/include/clang/CodeGen/SwiftCallingConv.h
cfe/trunk/lib/CodeGen/SwiftCallingConv.cpp
cfe/trunk/test/CodeGen/arm-swiftcall.c
cfe/trunk/test/CodeGenCXX/arm-swiftcall.cpp
Modified:
cfe/trunk/lib/CodeGen/ABIInfo.h
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/CodeGen/CMakeLists.txt
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/CodeGenTypes.h
cfe/trunk/lib/CodeGen/TargetInfo.cpp
cfe/trunk/lib/CodeGen/TargetInfo.h

Added: cfe/trunk/include/clang/CodeGen/SwiftCallingConv.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/CodeGen/SwiftCallingConv.h?rev=265324&view=auto
==
--- cfe/trunk/include/clang/CodeGen/SwiftCallingConv.h (added)
+++ cfe/trunk/include/clang/CodeGen/SwiftCallingConv.h Mon Apr  4 13:33:08 2016
@@ -0,0 +1,168 @@
+//==-- SwiftCallingConv.h - Swift ABI lowering 
-==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// Defines constants and types related to Swift ABI lowering.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_CODEGEN_SWIFTCALLINGCONV_H
+#define LLVM_CLANG_CODEGEN_SWIFTCALLINGCONV_H
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/CharUnits.h"
+#include "clang/AST/Type.h"
+#include "llvm/ADT/FoldingSet.h"
+#include "llvm/Support/TrailingObjects.h"
+#include 
+
+namespace llvm {
+  class IntegerType;
+  class Type;
+  class StructType;
+  class VectorType;
+}
+
+namespace clang {
+class Decl;
+class FieldDecl;
+class ASTRecordLayout;
+
+namespace CodeGen {
+class ABIArgInfo;
+class CodeGenModule;
+class CGFunctionInfo;
+
+namespace swiftcall {
+
+class SwiftAggLowering {
+  CodeGenModule &CGM;
+
+  struct StorageEntry {
+CharUnits Begin;
+CharUnits End;
+llvm::Type *Type;
+
+CharUnits getWidth() const {
+  return End - Begin;
+}
+  };
+  SmallVector Entries;
+  bool Finished = false;
+
+public:
+  SwiftAggLowering(CodeGenModule &CGM) : CGM(CGM) {}
+
+  void addOpaqueData(CharUnits begin, CharUnits end) {
+addEntry(nullptr, begin, end);
+  }
+
+  void addTypedData(QualType type, CharUnits begin);
+  void addTypedData(const RecordDecl *record, CharUnits begin);
+  void addTypedData(const RecordDecl *record, CharUnits begin,
+const ASTRecordLayout &layout);
+  void addTypedData(llvm::Type *type, CharUnits begin);
+  void addTypedData(llvm::Type *type, CharUnits begin, CharUnits end);
+
+  void finish();
+
+  /// Does this lowering require passing any data?
+  bool empty() const {
+assert(Finished && "didn't finish lowering before calling empty()");
+return Entries.empty();
+  }
+
+  /// According to the target Swift ABI, should a value with this lowering
+  /// be passed indirectly?
+  ///
+  /// Note that this decision is based purely on the data layout of the
+  /// value and does not consider whether the type is address-only,
+  /// must be passed indirectly to match a function abstraction pattern, or
+  /// anything else that is expected to be handled by high-level lowering.
+  ///
+  /// \param asReturnValue - if true, answer whether it should be passed
+  ///   indirectly as a return value; if false, answer whether it should be
+  ///   passed indirectly as an argument
+  bool shouldPassIndirectly(bool asReturnValue) const;
+
+  using EnumerationCallback =
+llvm::function_ref;
+
+  /// Enumerate the expanded components of this type.
+  ///
+  /// The component types will always be legal vector, floating-point,
+  /// integer, or pointer types.
+  void enumerateComponents(EnumerationCallback callback) const;
+
+  /// Return the types for a coerce-and-expand operation.
+  ///
+  /// The first type matches the memory layout of the data that's been
+  /// added to this structure, including explicit [N x i8] arrays for any
+  /// internal padding.
+  ///
+  /// The second type removes any internal padding members and, if only
+  /// one element remains, is simply that element type.
+  std::pair getCoerceAndExpandTypes() const;
+
+private:
+  void addBitFieldData(const FieldDecl *field, CharUnits begin,
+   uint64_t bitOffset);
+  void addLegalTypedData(llvm::Type *type, CharUnits begin, CharUnits end);
+  void addEntry(llvm::Type *type, CharUnits begin, CharUnits end);
+  void splitVectorEntry(unsigned index);
+};
+
+/// Return the maximum voluntary integer size for the current target.
+CharUnits

r265323 - Add a couple of convenience operations to CharUnits.

[John McCall via cfe-commits]

Author: rjmccall
Date: Mon Apr  4 13:33:00 2016
New Revision: 265323

URL: http://llvm.org/viewvc/llvm-project?rev=265323&view=rev
Log:
Add a couple of convenience operations to CharUnits.

Modified:
cfe/trunk/include/clang/AST/CharUnits.h

Modified: cfe/trunk/include/clang/AST/CharUnits.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CharUnits.h?rev=265323&r1=265322&r2=265323&view=diff
==
--- cfe/trunk/include/clang/AST/CharUnits.h (original)
+++ cfe/trunk/include/clang/AST/CharUnits.h Mon Apr  4 13:33:00 2016
@@ -142,9 +142,17 @@ namespace clang {
   CharUnits operator* (QuantityType N) const {
 return CharUnits(Quantity * N);
   }
+  CharUnits &operator*= (QuantityType N) {
+Quantity *= N;
+return *this;
+  }
   CharUnits operator/ (QuantityType N) const {
 return CharUnits(Quantity / N);
   }
+  CharUnits operator/= (QuantityType N) {
+Quantity /= N;
+return *this;
+  }
   QuantityType operator/ (const CharUnits &Other) const {
 return Quantity / Other.Quantity;
   }


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

Re: [PATCH] D18713: [OpenCL] Generate bitcast when target address space does not change.

[Yaxun Liu via cfe-commits]

yaxunl added a comment.

> Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with 
> CreateAddrSpaceCast call too. I am wondering if that could have the same 
> issue... if yes, may be we should fix it already now.


I cannot find another CreateAddrSpaceCast in lib/CodeGen/CGExprScalar.cpp with 
the latest commit 885217218dd21498430d50c5a2f6dd8e329add4b. Do you have the 
commit hash and line number?

Thanks.


http://reviews.llvm.org/D18713



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

Re: [PATCH] D3635: DebugInfo: Emit any enum with a referenced enum constant.

[David Blaikie via cfe-commits]

dblaikie abandoned this revision.
dblaikie added a subscriber: rsmith.
dblaikie added a comment.

In http://reviews.llvm.org/D3635#44720, @rsmith wrote:

> Implementation LGTM, if you decide that you want to go in this direction.


Just looking over some old patches/git branches I had lying around.

It seems what I might need to make this closer to GCC's size/fidelity tradeoff, 
is the ability to detect if the use of the enumerator was an ODR use. So that 
this:

  enum X { Y };
  int i = Y;

was a use, but this:

  int i[Y];

was not.

Is that reasnable? Can I practically detect that property in 
MarkAnyDeclReferenced?

(is "int i = Y;" an ODR use? Perhaps I'm getting the terminology wrong... maybe 
there is no nice linguistic property that distinguishes these two cases?)

& just for the record, the patch as it stands grows the .dwo size for the dwos 
build for the clang binary by a little over 18% :/

& using the MightBeOdrUse flag to MarkAnyDeclReferenced didn't seem to change 
the array case behavior above - I didn't get aggregate numbers to see if it had 
any impact.


http://reviews.llvm.org/D3635



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

Re: r265323 - Add a couple of convenience operations to CharUnits.

[Sean Silva via cfe-commits]

On Mon, Apr 4, 2016 at 11:33 AM, John McCall via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rjmccall
> Date: Mon Apr  4 13:33:00 2016
> New Revision: 265323
>
> URL: http://llvm.org/viewvc/llvm-project?rev=265323&view=rev
> Log:
> Add a couple of convenience operations to CharUnits.
>
> Modified:
> cfe/trunk/include/clang/AST/CharUnits.h
>
> Modified: cfe/trunk/include/clang/AST/CharUnits.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CharUnits.h?rev=265323&r1=265322&r2=265323&view=diff
>
> ==
> --- cfe/trunk/include/clang/AST/CharUnits.h (original)
> +++ cfe/trunk/include/clang/AST/CharUnits.h Mon Apr  4 13:33:00 2016
> @@ -142,9 +142,17 @@ namespace clang {
>CharUnits operator* (QuantityType N) const {
>  return CharUnits(Quantity * N);
>}
> +  CharUnits &operator*= (QuantityType N) {
> +Quantity *= N;
> +return *this;
> +  }
>CharUnits operator/ (QuantityType N) const {
>  return CharUnits(Quantity / N);
>}
> +  CharUnits operator/= (QuantityType N) {
>

Should this return type be a reference like the one for operator*=?

-- Sean Silva


> +Quantity /= N;
> +return *this;
> +  }
>QuantityType operator/ (const CharUnits &Other) const {
>  return Quantity / Other.Quantity;
>}
>
>
> ___
> 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] D18709: Add copyright notice to modulemap files

[Yunzhong Gao via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL265325: Add copyright notice to the modulemap file. 
(authored by ygao).

Changed prior to commit:
  http://reviews.llvm.org/D18709?vs=52414&id=52590#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18709

Files:
  cfe/trunk/lib/Headers/module.modulemap

Index: cfe/trunk/lib/Headers/module.modulemap
===
--- cfe/trunk/lib/Headers/module.modulemap
+++ cfe/trunk/lib/Headers/module.modulemap
@@ -1,3 +1,26 @@
+/*=== module.modulemap - intrinsics module map -===
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ *===---===
+ */
+
 module _Builtin_intrinsics [system] [extern_c] {
   explicit module altivec {
 requires altivec


Index: cfe/trunk/lib/Headers/module.modulemap
===
--- cfe/trunk/lib/Headers/module.modulemap
+++ cfe/trunk/lib/Headers/module.modulemap
@@ -1,3 +1,26 @@
+/*=== module.modulemap - intrinsics module map -===
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ *===---===
+ */
+
 module _Builtin_intrinsics [system] [extern_c] {
   explicit module altivec {
 requires altivec
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r265325 - Add copyright notice to the modulemap file.

[Yunzhong Gao via cfe-commits]

Author: ygao
Date: Mon Apr  4 13:46:09 2016
New Revision: 265325

URL: http://llvm.org/viewvc/llvm-project?rev=265325&view=rev
Log:
Add copyright notice to the modulemap file.

The module.modulemap file in the lib/Headers directory was missing the LLVM
copyright notice. This patch adds the copyright notice just like the rest of
the files in this directory.

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


Modified:
cfe/trunk/lib/Headers/module.modulemap

Modified: cfe/trunk/lib/Headers/module.modulemap
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/module.modulemap?rev=265325&r1=265324&r2=265325&view=diff
==
--- cfe/trunk/lib/Headers/module.modulemap (original)
+++ cfe/trunk/lib/Headers/module.modulemap Mon Apr  4 13:46:09 2016
@@ -1,3 +1,26 @@
+/*=== module.modulemap - intrinsics module map -===
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ *===---===
+ */
+
 module _Builtin_intrinsics [system] [extern_c] {
   explicit module altivec {
 requires altivec


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

Re: r263051 - [PPC] FE support for generating VSX [negated] absolute value instructions

[Kit Barton via cfe-commits]

I forgot to edit the message.
Do you think it's worth going back and fixing the log message at this point?
 
Kit Barton, Ph.D.LLVM Development on POWERIBM Toronto Lab, D2/929/8200/MKM8200 Warden Ave, Markham, L6G 1C7(905) 413-3452kbar...@ca.ibm.com
 
 
- Original message -From: Eric Christopher To: Kit Barton/Toronto/IBM@IBMCA, cfe-commits@lists.llvm.orgCc:Subject: Re: r263051 - [PPC] FE support for generating VSX [negated] absolute value instructionsDate: Wed, Mar 9, 2016 5:36 PM 
 
On Wed, Mar 9, 2016 at 11:33 AM Kit Barton via cfe-commits  wrote:
Author: kbartonDate: Wed Mar  9 13:28:31 2016New Revision: 263051URL: http://llvm.org/viewvc/llvm-project?rev=263051&view=revLog:[PPC] FE support for generating VSX [negated] absolute value instructionsIncludes new built-in, conversion of built-in to target-independent intrinsicand update in the header file. Tests are also updated. There is a second part inthe backend for which I will post a separate code-review. BACKEND PART SHOULD BECOMMITTED FIRST. 
 
Did you mean to commit this? Or just forget to edit your commit message?
 
-eric
 
Phabricator: http://reviews.llvm.org/D17816Modified:    cfe/trunk/include/clang/Basic/BuiltinsPPC.def    cfe/trunk/lib/CodeGen/CGBuiltin.cpp    cfe/trunk/lib/Headers/altivec.h    cfe/trunk/test/CodeGen/builtins-ppc-altivec.c    cfe/trunk/test/CodeGen/builtins-ppc-p8vector.c    cfe/trunk/test/CodeGen/builtins-ppc-vsx.cModified: cfe/trunk/include/clang/Basic/BuiltinsPPC.defURL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/BuiltinsPPC.def?rev=263051&r1=263050&r2=263051&view=diff==--- cfe/trunk/include/clang/Basic/BuiltinsPPC.def (original)+++ cfe/trunk/include/clang/Basic/BuiltinsPPC.def Wed Mar  9 13:28:31 2016@@ -336,6 +336,9 @@ BUILTIN(__builtin_vsx_xxleqv, "V4UiV4UiV BUILTIN(__builtin_vsx_xvcpsgndp, "V2dV2dV2d", "") BUILTIN(__builtin_vsx_xvcpsgnsp, "V4fV4fV4f", "")+BUILTIN(__builtin_vsx_xvabssp, "V4fV4f", "")+BUILTIN(__builtin_vsx_xvabsdp, "V2dV2d", "")+ // HTM builtins BUILTIN(__builtin_tbegin, "UiUIi", "") BUILTIN(__builtin_tend, "UiUIi", "")Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cppURL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=263051&r1=263050&r2=263051&view=diff==--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Wed Mar  9 13:28:31 2016@@ -6992,6 +6992,16 @@ Value *CodeGenFunction::EmitPPCBuiltinEx     llvm::Function *F = CGM.getIntrinsic(ID, ResultType);     return Builder.CreateCall(F, X);   }++  // Absolute value+  case PPC::BI__builtin_vsx_xvabsdp:+  case PPC::BI__builtin_vsx_xvabssp: {+    llvm::Type *ResultType = ConvertType(E->getType());+    Value *X = EmitScalarExpr(E->getArg(0));+    llvm::Function *F = CGM.getIntrinsic(Intrinsic::fabs, ResultType);+    return Builder.CreateCall(F, X);+  }+   // FMA variations   case PPC::BI__builtin_vsx_xvmaddadp:   case PPC::BI__builtin_vsx_xvmaddasp:Modified: cfe/trunk/lib/Headers/altivec.hURL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/altivec.h?rev=263051&r1=263050&r2=263051&view=diff==--- cfe/trunk/lib/Headers/altivec.h (original)+++ cfe/trunk/lib/Headers/altivec.h Wed Mar  9 13:28:31 2016@@ -124,16 +124,18 @@ vec_abs(vector signed long long __a) { #endif static vector float __ATTRS_o_ai vec_abs(vector float __a) {+#ifdef __VSX__+  return __builtin_vsx_xvabssp(__a);+#else   vector unsigned int __res =       (vector unsigned int)__a & (vector unsigned int)(0x7FFF);   return (vector float)__res;+#endif } #if defined(__POWER8_VECTOR__) && defined(__powerpc64__) static vector double __ATTRS_o_ai vec_abs(vector double __a) {-  vector unsigned long long __res = { 0x7FFF, 0x7FFF };-  __res &= (vector unsigned int)__a;-  return (vector double)__res;+  return __builtin_vsx_xvabsdp(__a); } #endifModified: cfe/trunk/test/CodeGen/builtins-ppc-altivec.cURL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtins-ppc-altivec.c?rev=263051&r1=263050&r2=263051&view=diff==--- cfe/trunk/test/CodeGen/builtins-ppc-altivec.c (original)+++ cfe/trunk/test/CodeGen/builtins-ppc-altivec.c Wed Mar  9 13:28:31 2016@@ -1,7 +1,13 @@ // REQUIRES: powerpc-registered-target-// RUN: %clang_cc1 -faltivec -triple powerpc-unknown-unknown -emit-llvm %s -o - | FileCheck %s-// RUN: %clang_cc1 -faltivec -triple powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s-// RUN: %clang_cc1 -faltivec -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-LE+// RUN: %clang_cc1 -faltivec -triple powerpc-unknown-unknown -emit-llvm %s \+// RUN:            -o - | FileCheck %s+// RUN: %clang_cc1 -faltivec 

Re: r265323 - Add a couple of convenience operations to CharUnits.

[John McCall via cfe-commits]

> On Apr 4, 2016, at 11:49 AM, Sean Silva  wrote:
> On Mon, Apr 4, 2016 at 11:33 AM, John McCall via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: rjmccall
> Date: Mon Apr  4 13:33:00 2016
> New Revision: 265323
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=265323&view=rev 
> 
> Log:
> Add a couple of convenience operations to CharUnits.
> 
> Modified:
> cfe/trunk/include/clang/AST/CharUnits.h
> 
> Modified: cfe/trunk/include/clang/AST/CharUnits.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CharUnits.h?rev=265323&r1=265322&r2=265323&view=diff
>  
> 
> ==
> --- cfe/trunk/include/clang/AST/CharUnits.h (original)
> +++ cfe/trunk/include/clang/AST/CharUnits.h Mon Apr  4 13:33:00 2016
> @@ -142,9 +142,17 @@ namespace clang {
>CharUnits operator* (QuantityType N) const {
>  return CharUnits(Quantity * N);
>}
> +  CharUnits &operator*= (QuantityType N) {
> +Quantity *= N;
> +return *this;
> +  }
>CharUnits operator/ (QuantityType N) const {
>  return CharUnits(Quantity / N);
>}
> +  CharUnits operator/= (QuantityType N) {
> 
> Should this return type be a reference like the one for operator*=?

Yes, absolutely.  Fixed in r265328, thanks.

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

r265328 - Assignment operators should return by reference.

[John McCall via cfe-commits]

Author: rjmccall
Date: Mon Apr  4 13:53:01 2016
New Revision: 265328

URL: http://llvm.org/viewvc/llvm-project?rev=265328&view=rev
Log:
Assignment operators should return by reference.

Thanks to Sean Silva for pointing this out.

Modified:
cfe/trunk/include/clang/AST/CharUnits.h

Modified: cfe/trunk/include/clang/AST/CharUnits.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CharUnits.h?rev=265328&r1=265327&r2=265328&view=diff
==
--- cfe/trunk/include/clang/AST/CharUnits.h (original)
+++ cfe/trunk/include/clang/AST/CharUnits.h Mon Apr  4 13:53:01 2016
@@ -149,7 +149,7 @@ namespace clang {
   CharUnits operator/ (QuantityType N) const {
 return CharUnits(Quantity / N);
   }
-  CharUnits operator/= (QuantityType N) {
+  CharUnits &operator/= (QuantityType N) {
 Quantity /= N;
 return *this;
   }


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

Re: [PATCH] D18425: [Sema] Make enable_if act correctly with value dependent conditions/arguments

[Richard Smith via cfe-commits]

rsmith added inline comments.


Comment at: include/clang/Sema/Sema.h:2489
@@ -2485,1 +2488,3 @@
+  bool MissingImplicitThis = false,
+  bool *FailedDueToValueDependentExpr = nullptr);
 

I'd just call this `Dependent` -- that is, "here is a dependent `enable_if` 
attribute, so I couldn't tell whether this is enabled". The details of why it's 
dependent shouldn't be relevant to the caller.


Comment at: lib/Sema/SemaExpr.cpp:5047-5098
@@ -5046,57 +5046,54 @@
 
+CallExpr *Sema::buildDependentCallExpr(Expr *ExecConfig, Expr *Fn,
+   MultiExprArg ArgExprs,
+   SourceLocation RParenLoc) {
+  if (ExecConfig)
+return new (Context)
+CUDAKernelCallExpr(Context, Fn, cast(ExecConfig), ArgExprs,
+   Context.DependentTy, VK_RValue, RParenLoc);
+  return new (Context) CallExpr(Context, Fn, ArgExprs, Context.DependentTy,
+VK_RValue, RParenLoc);
+}
+
 /// ActOnCallExpr - Handle a call to Fn with the specified array of arguments.
 /// This provides the location of the left/right parens and a list of comma
 /// locations.
 ExprResult
 Sema::ActOnCallExpr(Scope *S, Expr *Fn, SourceLocation LParenLoc,
 MultiExprArg ArgExprs, SourceLocation RParenLoc,
 Expr *ExecConfig, bool IsExecConfig) {
   // Since this might be a postfix expression, get rid of ParenListExprs.
   ExprResult Result = MaybeConvertParenListExprToParenExpr(S, Fn);
   if (Result.isInvalid()) return ExprError();
   Fn = Result.get();
 
   if (checkArgsForPlaceholders(*this, ArgExprs))
 return ExprError();
 
   if (getLangOpts().CPlusPlus) {
 // If this is a pseudo-destructor expression, build the call immediately.
 if (isa(Fn)) {
   if (!ArgExprs.empty()) {
 // Pseudo-destructor calls should not have any arguments.
 Diag(Fn->getLocStart(), diag::err_pseudo_dtor_call_with_args)
   << FixItHint::CreateRemoval(
 
SourceRange(ArgExprs.front()->getLocStart(),
 ArgExprs.back()->getLocEnd()));
   }
 
   return new (Context)
   CallExpr(Context, Fn, None, Context.VoidTy, VK_RValue, RParenLoc);
 }
 if (Fn->getType() == Context.PseudoObjectTy) {
   ExprResult result = CheckPlaceholderExpr(Fn);
   if (result.isInvalid()) return ExprError();
   Fn = result.get();
 }
 
 // Determine whether this is a dependent call inside a C++ template,
 // in which case we won't do any semantic analysis now.
 // FIXME: Will need to cache the results of name lookup (including ADL) in
 // Fn.
-bool Dependent = false;
-if (Fn->isTypeDependent())
-  Dependent = true;
-else if (Expr::hasAnyTypeDependentArguments(ArgExprs))
-  Dependent = true;
-
-if (Dependent) {
-  if (ExecConfig) {
-return new (Context) CUDAKernelCallExpr(
-Context, Fn, cast(ExecConfig), ArgExprs,
-Context.DependentTy, VK_RValue, RParenLoc);
-  } else {
-return new (Context) CallExpr(
-Context, Fn, ArgExprs, Context.DependentTy, VK_RValue, RParenLoc);
-  }
-}
+if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))
+  return buildDependentCallExpr(ExecConfig, Fn, ArgExprs, RParenLoc);
 

You don't seem to make use of this refactoring in the functional change below. 
Either revert or commit separately, please (and if you commit it, fix the 
comment in Sema.h to reflect what this is used for).


Comment at: lib/Sema/SemaExpr.cpp:5095-5096
@@ -5083,3 +5094,4 @@
 // in which case we won't do any semantic analysis now.
 // FIXME: Will need to cache the results of name lookup (including ADL) in
 // Fn.
+if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))

Can you delete this FIXME? I'm pretty sure we've got that right for years :)


Comment at: lib/Sema/SemaExpr.cpp:5202-5205
@@ +5201,6 @@
+  Fn, NDecl, LParenLoc, ArgExprs, RParenLoc, ExecConfig, IsExecConfig);
+  if (MakeCallValueDependent) {
+BuiltCall.get()->setValueDependent(true);
+BuiltCall.get()->setInstantiationDependent(true);
+  }
+

Does this need to be value-dependent? The only possible outcomes here are 
either that we call the designated function or that instantiation fails, which 
seems like instantiation-dependence is enough. (In practice, the 
`setValueDependent` call will be a no-op, because the only way the flag gets 
set is if one of the `ArgExprs` is value-dependent, which results in the call 
being value-dependent regardless, so there should be no functional change in 
removing the `setValueDependent` call.)


Comment at: lib/Sema/SemaOverload

Re: r263191 - Preserve ExtParameterInfos into CGFunctionInfo.

[Nico Weber via cfe-commits]

Thanks, r265324 fixed the assert.

On Fri, Apr 1, 2016 at 8:25 PM, John McCall  wrote:

> On Apr 1, 2016, at 3:50 PM, Nico Weber  wrote:
> On Thu, Mar 10, 2016 at 8:30 PM, John McCall via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: rjmccall
>> Date: Thu Mar 10 22:30:31 2016
>> New Revision: 263191
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=263191&view=rev
>> Log:
>> Preserve ExtParameterInfos into CGFunctionInfo.
>>
>> As part of this, make the function-arrangement interfaces
>> a little simpler and more semantic.
>>
>> NFC.
>>
>> Modified:
>> cfe/trunk/include/clang/AST/CanonicalType.h
>> cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h
>> cfe/trunk/lib/CodeGen/CGAtomic.cpp
>> cfe/trunk/lib/CodeGen/CGBlocks.cpp
>> cfe/trunk/lib/CodeGen/CGBuiltin.cpp
>> cfe/trunk/lib/CodeGen/CGCall.cpp
>> cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
>> cfe/trunk/lib/CodeGen/CGException.cpp
>> cfe/trunk/lib/CodeGen/CGExpr.cpp
>> cfe/trunk/lib/CodeGen/CGObjC.cpp
>> cfe/trunk/lib/CodeGen/CGObjCMac.cpp
>> cfe/trunk/lib/CodeGen/CGObjCRuntime.cpp
>> cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
>> cfe/trunk/lib/CodeGen/CGStmt.cpp
>> cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
>> cfe/trunk/lib/CodeGen/CodeGenABITypes.cpp
>> cfe/trunk/lib/CodeGen/CodeGenTypes.h
>> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
>>
>> Modified: cfe/trunk/include/clang/AST/CanonicalType.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/CanonicalType.h?rev=263191&r1=263190&r2=263191&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/AST/CanonicalType.h (original)
>> +++ cfe/trunk/include/clang/AST/CanonicalType.h Thu Mar 10 22:30:31 2016
>> @@ -484,6 +484,9 @@ struct CanProxyAdaptor>LLVM_CLANG_CANPROXY_TYPE_ACCESSOR(getReturnType)
>>LLVM_CLANG_CANPROXY_SIMPLE_ACCESSOR(FunctionType::ExtInfo, getExtInfo)
>>LLVM_CLANG_CANPROXY_SIMPLE_ACCESSOR(unsigned, getNumParams)
>> +  LLVM_CLANG_CANPROXY_SIMPLE_ACCESSOR(bool, hasExtParameterInfos)
>> +  LLVM_CLANG_CANPROXY_SIMPLE_ACCESSOR(
>> +ArrayRef,
>> getExtParameterInfos)
>>CanQualType getParamType(unsigned i) const {
>>  return
>> CanQualType::CreateUnsafe(this->getTypePtr()->getParamType(i));
>>}
>>
>> Modified: cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h?rev=263191&r1=263190&r2=263191&view=diff
>>
>> ==
>> --- cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h (original)
>> +++ cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h Thu Mar 10 22:30:31
>> 2016
>> @@ -343,8 +343,10 @@ struct CGFunctionInfoArgInfo {
>>  /// function definition.
>>  class CGFunctionInfo final
>>  : public llvm::FoldingSetNode,
>> -  private llvm::TrailingObjects> CGFunctionInfoArgInfo> {
>> +  private llvm::TrailingObjects> CGFunctionInfoArgInfo,
>> +FunctionProtoType::ExtParameterInfo>
>> {
>>typedef CGFunctionInfoArgInfo ArgInfo;
>> +  typedef FunctionProtoType::ExtParameterInfo ExtParameterInfo;
>>
>>/// The LLVM::CallingConv to use for this function (as specified by the
>>/// user).
>> @@ -378,7 +380,8 @@ class CGFunctionInfo final
>>/// The struct representing all arguments passed in memory.  Only used
>> when
>>/// passing non-trivial types with inalloca.  Not part of the profile.
>>llvm::StructType *ArgStruct;
>> -  unsigned ArgStructAlign;
>> +  unsigned ArgStructAlign : 31;
>> +  unsigned HasExtParameterInfos : 1;
>>
>>unsigned NumArgs;
>>
>> @@ -389,7 +392,19 @@ class CGFunctionInfo final
>>  return getTrailingObjects();
>>}
>>
>> -  size_t numTrailingObjects(OverloadToken) { return NumArgs +
>> 1; }
>> +  ExtParameterInfo *getExtParameterInfosBuffer() {
>> +return getTrailingObjects();
>> +  }
>> +  const ExtParameterInfo *getExtParameterInfosBuffer() const{
>> +return getTrailingObjects();
>> +  }
>> +
>> +  size_t numTrailingObjects(OverloadToken) const {
>> +return NumArgs + 1;
>> +  }
>> +  size_t numTrailingObjects(OverloadToken) const {
>> +return (HasExtParameterInfos ? NumArgs : 0);
>> +  }
>>friend class TrailingObjects;
>>
>>CGFunctionInfo() : Required(RequiredArgs::All) {}
>> @@ -399,6 +414,7 @@ public:
>>  bool instanceMethod,
>>  bool chainCall,
>>  const FunctionType::ExtInfo &extInfo,
>> +ArrayRef paramInfos,
>>  CanQualType resultType,
>>  ArrayRef argTypes,
>>  RequiredArgs required);
>> @@ -472,6 +488,16 @@ public:
>>ABIArgInfo &getReturnInfo() { return getArgsBuffer()[0].info; }
>>const ABIA

[PATCH] Windows support for ObjC DLLIMPORT

[Wes Witt via cfe-commits]

Please accept this diff as a change to support dllimport of objective c 
interfaces on windows. I've included a new test and changes to an existing test 
as well. All clang tests pass on Linux & Windows. This change is required for 
Microsoft's use of the clang compiler in support of objective c on windows.

Thanks,
Wes Witt



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

Re: [PATCH] D18703: [clang-tidy] Add new checker for comparison with runtime string functions.

[Etienne Bergeron via cfe-commits]

etienneb updated this revision to Diff 52601.
etienneb marked 3 inline comments as done.
etienneb added a comment.

add options, configurable list of function names, support for ! pattern.


http://reviews.llvm.org/D18703

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/SuspiciousStringCompareCheck.cpp
  clang-tidy/misc/SuspiciousStringCompareCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-suspicious-string-compare.rst
  test/clang-tidy/misc-suspicious-string-compare.c
  test/clang-tidy/misc-suspicious-string-compare.cpp

Index: test/clang-tidy/misc-suspicious-string-compare.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-string-compare.cpp
@@ -0,0 +1,299 @@
+// RUN: %check_clang_tidy %s misc-suspicious-string-compare %t -- \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: misc-suspicious-string-compare.WarnOnImplicitComparison, value: 1}, \
+// RUN:   {key: misc-suspicious-string-compare.WarnOnLogicalNotComparison, value: 1}]}' \
+// RUN: --
+
+typedef __SIZE_TYPE__ size;
+
+struct locale_t {
+  void* dummy;
+} locale;
+
+static const char A[] = "abc";
+static const unsigned char U[] = "abc";
+static const unsigned char V[] = "xyz";
+static const wchar_t W[] = L"abc";
+
+int memcmp(const void *, const void *, size);
+int wmemcmp(const wchar_t *, const wchar_t *, size);
+int memicmp(const void *, const void *, size);
+int _memicmp(const void *, const void *, size);
+int _memicmp_l(const void *, const void *, size, locale_t);
+
+int strcmp(const char *, const char *);
+int strncmp(const char *, const char *, size);
+int strcasecmp(const char *, const char *);
+int strncasecmp(const char *, const char *, size);
+int stricmp(const char *, const char *);
+int strcmpi(const char *, const char *);
+int strnicmp(const char *, const char *, size);
+int _stricmp(const char *, const char * );
+int _strnicmp(const char *, const char *, size);
+int _stricmp_l(const char *, const char *, locale_t);
+int _strnicmp_l(const char *, const char *, size, locale_t);
+
+int wcscmp(const wchar_t *, const wchar_t *);
+int wcsncmp(const wchar_t *, const wchar_t *, size);
+int wcscasecmp(const wchar_t *, const wchar_t *);
+int wcsicmp(const wchar_t *, const wchar_t *);
+int wcsnicmp(const wchar_t *, const wchar_t *, size);
+int _wcsicmp(const wchar_t *, const wchar_t *);
+int _wcsnicmp(const wchar_t *, const wchar_t *, size);
+int _wcsicmp_l(const wchar_t *, const wchar_t *, locale_t);
+int _wcsnicmp_l(const wchar_t *, const wchar_t *, size, locale_t);
+
+int _mbscmp(const unsigned char *, const unsigned char *);
+int _mbsncmp(const unsigned char *, const unsigned char *, size);
+int _mbsnbcmp(const unsigned char *, const unsigned char *, size);
+int _mbsnbicmp(const unsigned char *, const unsigned char *, size);
+int _mbsicmp(const unsigned char *, const unsigned char *);
+int _mbsnicmp(const unsigned char *, const unsigned char *, size);
+int _mbscmp_l(const unsigned char *, const unsigned char *, locale_t);
+int _mbsncmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+int _mbsicmp_l(const unsigned char *, const unsigned char *, locale_t);
+int _mbsnicmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+int _mbsnbcmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+int _mbsnbicmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+
+int test_warning_patterns() {
+  if (strcmp(A, "a"))
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is called without explicitly comparing result [misc-suspicious-string-compare]
+  // CHECK-FIXES: if (strcmp(A, "a") != 0)
+
+  if (strcmp(A, "a") == 0 ||
+  strcmp(A, "b"))
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is called without explicitly comparing result
+  // CHECK-FIXES: strcmp(A, "b") != 0)
+
+  if (strcmp(A, "a") == 1)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") == -1)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") == true)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") < '0')
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") < 0.)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' has suspicious implicit cast
+}
+
+int test_valid_patterns() {
+  // The following cases are valid.
+  if (strcmp(A, "a") < 0)
+return 0;
+  if (strcmp(A, "a") == 0)
+return 0;
+  if (strcmp(A, "a") <= 0)
+return 0;
+
+  if (wcscmp(W, L"a") < 0)
+return 0;
+  if (wcscmp(W, L"a") == 0)
+return 0;
+  if (wcscmp(W, L"a") <=

Re: [PATCH] D18551: Added Fixer implementation and fix() interface in clang-format for removing redundant code.

[Daniel Jasper via cfe-commits]

djasper added inline comments.


Comment at: include/clang/Format/Format.h:770
@@ +769,3 @@
+/// \brief Returns the replacements corresponding to applying, fixing, and
+/// reformatting \p Replaces.
+tooling::Replacements fixReplacements(StringRef Code,

I think we should not automatically format the replacements after fixing. This 
function can easily be combined with formatReplacements if that is desired.


Comment at: include/clang/Format/Format.h:802
@@ +801,3 @@
+///
+/// Otherwise identical to the reformat() function using a file ID.
+tooling::Replacements fix(const FormatStyle &Style, StringRef Code,

This sentence doesn't make sense.


Comment at: include/clang/Format/Format.h:803
@@ +802,3 @@
+/// Otherwise identical to the reformat() function using a file ID.
+tooling::Replacements fix(const FormatStyle &Style, StringRef Code,
+  ArrayRef Ranges,

I believe the style will become important as certain aspects of fixing are 
going to be controlled via the style. E.g. some styles might want to turn:

  if (a)
return;

into:

  if (a) {
return;
  }


Comment at: lib/Format/Format.cpp:1446
@@ -1445,1 +1445,3 @@
 
+// Returns true if 'Range' intersects with one of the input ranges.
+static bool affectsCharSourceRange(SourceManager &SourceMgr,

I'd probably move all of these out into a class so you don't need to pass the 
same set of parameters between all of them and so that you can make those 
private that aren't meant to be called directly.


Comment at: lib/Format/Format.cpp:1596
@@ +1595,3 @@
+
+class Fixer : public UnwrappedLineConsumer {
+public:

I am not sure, this is the right class to pull out. It still has a lot of 
overlap with formatter. Maybe it is instead a better idea to pull out each 
fixer into its own class and pull them into the formatter.


Comment at: lib/Format/Format.cpp:1651
@@ +1650,3 @@
+
+  tooling::Replacements fix(FormatTokenLexer &Tokens, bool *IncompleteFormat) {
+reset();

The Tokens parameter is unused!?


Comment at: lib/Format/Format.cpp:1654
@@ +1653,3 @@
+
+while (CurrentToken->isNot(tok::eof)) {
+  switch (CurrentToken->Tok.getKind()) {

I am not (yet) convinced that the fundamental design here makes sense. So far 
it seems to me that all fixers will either operate within a single line (ctor 
fixer and probably most others) or that they do something specific on multiple 
lines (namespace fixer and empty public/private section fixer and maybe 
others). Creating a new way to iterate over all tokens of all lines doesn't 
really add useful functionality here AFAICT.


Comment at: lib/Format/Format.cpp:1657
@@ +1656,3 @@
+  case tok::colon:
+if (CurrentToken->Type == TT_CtorInitializerColon) {
+  checkConstructorInitList();

No braces. Here and at all other single-statement ifs.


Comment at: lib/Format/Format.cpp:1690
@@ +1689,3 @@
+
+  void reset() {
+CurrentLine = 0;

Having a function called "reset" that is only called once is weird. Just do 
this in the constructor?


Comment at: lib/Format/Format.cpp:1748
@@ +1747,3 @@
+unsigned Idx = 0;
+while (Idx < Tokens.size()) {
+  unsigned St = Idx, Ed = Idx;

Comment on what this is doing.


Comment at: lib/Format/Format.cpp:1749
@@ +1748,3 @@
+while (Idx < Tokens.size()) {
+  unsigned St = Idx, Ed = Idx;
+  while ((Ed + 1) < Tokens.size() && Tokens[Ed]->Next == Tokens[Ed + 1]) {

Ed? Either use E or End.


Comment at: lib/Format/Format.cpp:1891
@@ +1890,3 @@
+
+  struct FormatTokenLess {
+FormatTokenLess(SourceManager &SM) : SM(SM) {}

Why is this needed?


Comment at: lib/Format/Format.cpp:1909
@@ +1908,3 @@
+  SmallVector AnnotatedLines;
+  size_t CurrentLine;
+  FormatToken DummyToken;

A lot of these need documentation. What's the current line? What is the last 
token (last token that was read? last token of line? last token of file?). What 
are RedundantTokens?


Comment at: lib/Format/Format.cpp:2356
@@ +2355,3 @@
+
+  std::string NewCode = applyAllReplacements(Code, Replaces);
+  std::vector ChangedRanges =

Too much code duplication. Find a way to avoid that (passing in the calls to 
either reformat() or fix() as lambdas to a shared internal function).


Comment at: unittests/Format/FormatTest.cpp:11236
@@ -11235,1 +11235,3 @@
 
+class FixTest : public ::testing::Test {
+protected:

All of this should go into a different file.


http://reviews.llvm.org/D18551



___
cfe-commits mailing list
cfe-commits@lis

Re: [PATCH] D18703: [clang-tidy] Add new checker for comparison with runtime string functions.

[Etienne Bergeron via cfe-commits]

etienneb updated this revision to Diff 52602.
etienneb added a comment.

nits


http://reviews.llvm.org/D18703

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/SuspiciousStringCompareCheck.cpp
  clang-tidy/misc/SuspiciousStringCompareCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-suspicious-string-compare.rst
  test/clang-tidy/misc-suspicious-string-compare.c
  test/clang-tidy/misc-suspicious-string-compare.cpp

Index: test/clang-tidy/misc-suspicious-string-compare.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-suspicious-string-compare.cpp
@@ -0,0 +1,299 @@
+// RUN: %check_clang_tidy %s misc-suspicious-string-compare %t -- \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: misc-suspicious-string-compare.WarnOnImplicitComparison, value: 1}, \
+// RUN:   {key: misc-suspicious-string-compare.WarnOnLogicalNotComparison, value: 1}]}' \
+// RUN: --
+
+typedef __SIZE_TYPE__ size;
+
+struct locale_t {
+  void* dummy;
+} locale;
+
+static const char A[] = "abc";
+static const unsigned char U[] = "abc";
+static const unsigned char V[] = "xyz";
+static const wchar_t W[] = L"abc";
+
+int memcmp(const void *, const void *, size);
+int wmemcmp(const wchar_t *, const wchar_t *, size);
+int memicmp(const void *, const void *, size);
+int _memicmp(const void *, const void *, size);
+int _memicmp_l(const void *, const void *, size, locale_t);
+
+int strcmp(const char *, const char *);
+int strncmp(const char *, const char *, size);
+int strcasecmp(const char *, const char *);
+int strncasecmp(const char *, const char *, size);
+int stricmp(const char *, const char *);
+int strcmpi(const char *, const char *);
+int strnicmp(const char *, const char *, size);
+int _stricmp(const char *, const char * );
+int _strnicmp(const char *, const char *, size);
+int _stricmp_l(const char *, const char *, locale_t);
+int _strnicmp_l(const char *, const char *, size, locale_t);
+
+int wcscmp(const wchar_t *, const wchar_t *);
+int wcsncmp(const wchar_t *, const wchar_t *, size);
+int wcscasecmp(const wchar_t *, const wchar_t *);
+int wcsicmp(const wchar_t *, const wchar_t *);
+int wcsnicmp(const wchar_t *, const wchar_t *, size);
+int _wcsicmp(const wchar_t *, const wchar_t *);
+int _wcsnicmp(const wchar_t *, const wchar_t *, size);
+int _wcsicmp_l(const wchar_t *, const wchar_t *, locale_t);
+int _wcsnicmp_l(const wchar_t *, const wchar_t *, size, locale_t);
+
+int _mbscmp(const unsigned char *, const unsigned char *);
+int _mbsncmp(const unsigned char *, const unsigned char *, size);
+int _mbsnbcmp(const unsigned char *, const unsigned char *, size);
+int _mbsnbicmp(const unsigned char *, const unsigned char *, size);
+int _mbsicmp(const unsigned char *, const unsigned char *);
+int _mbsnicmp(const unsigned char *, const unsigned char *, size);
+int _mbscmp_l(const unsigned char *, const unsigned char *, locale_t);
+int _mbsncmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+int _mbsicmp_l(const unsigned char *, const unsigned char *, locale_t);
+int _mbsnicmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+int _mbsnbcmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+int _mbsnbicmp_l(const unsigned char *, const unsigned char *, size, locale_t);
+
+int test_warning_patterns() {
+  if (strcmp(A, "a"))
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is called without explicitly comparing result [misc-suspicious-string-compare]
+  // CHECK-FIXES: if (strcmp(A, "a") != 0)
+
+  if (strcmp(A, "a") == 0 ||
+  strcmp(A, "b"))
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is called without explicitly comparing result
+  // CHECK-FIXES: strcmp(A, "b") != 0)
+
+  if (strcmp(A, "a") == 1)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") == -1)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") == true)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") < '0')
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (strcmp(A, "a") < 0.)
+return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' has suspicious implicit cast
+}
+
+int test_valid_patterns() {
+  // The following cases are valid.
+  if (strcmp(A, "a") < 0)
+return 0;
+  if (strcmp(A, "a") == 0)
+return 0;
+  if (strcmp(A, "a") <= 0)
+return 0;
+
+  if (wcscmp(W, L"a") < 0)
+return 0;
+  if (wcscmp(W, L"a") == 0)
+return 0;
+  if (wcscmp(W, L"a") <= 0)
+return 0;
+
+  return 1;
+}
+
+int test_implicit_compare_with_functions() {
+
+  if (memcmp(A, "a", 1)