[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

If the current listed reviewers on this patch are not the best people for 
reviewing this, would one of them please suggest a more appropriate reviewer so 
we can get some traction on this?


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

https://reviews.llvm.org/D69585



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

[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Adding a ping since it's been a week with no additional feedback for the author.


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

https://reviews.llvm.org/D69585



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

[PATCH] D70553: [clang-apply-replacements] Add command line option to overwrite readonly files.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D70553#1757862 , @aaron.ballman 
wrote:

> Can you add a test case for this functionality?


The reason there's no test case is because it seems like that wouldn't really 
be any different than testing the functionality of `llvm::sys::fs`, which 
should already be handled at the llvm/Support layer.  There's not really any 
interesting logic here.  I can still do one if you think it's important but 
that was my reasoning anyway.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70553



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

[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Zachary Turner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG64f74bf72eb4: [clang-tidy] Rewrite modernize-avoid-bind 
check. (authored by zturner).

Changed prior to commit:
  https://reviews.llvm.org/D70368?vs=230661&id=231793#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70368

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -8,75 +8,62 @@
 template 
 bind_rt bind(Fp &&, Arguments &&...);
 }
+
+template 
+T ref(T &t);
 }
 
-int add(int x, int y) { return x + y; }
+namespace boost {
+template 
+class bind_rt {};
 
-void f() {
-  auto clj = std::bind(add, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
-  // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template 
+bind_rt bind(const Fp &, Arguments...);
 
-void g() {
-  int x = 2;
-  int y = 2;
-  auto clj = std::bind(add, x, y);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=] { return add(x, y); };
+template 
+struct reference_wrapper {
+  explicit reference_wrapper(T &t) {}
+};
+
+template 
+reference_wrapper const ref(T &t) {
+  return reference_wrapper(t);
 }
 
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+} // namespace boost
 
-void h() {
-  int x = 2;
-  auto clj = std::bind(add, x, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
 
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+struct Foo {
+  static int add(int x, int y) { return x + y; }
+};
 
-void i() {
-  auto BATest = std::bind(ABTest, _2, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+struct D {
+  D() = default;
+  void operator()(int x, int y) const {}
 
-void j() {
-  auto clj = std::bind(add, 2, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for argument mismatches.
-  // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+  void MemberFunction(int x) {}
 
-void k() {
-  auto clj = std::bind(add, _1, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for reused placeholders.
-  // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+  static D *create();
+};
 
-void m() {
-  auto clj = std::bind(add, 1, add(2, 5));
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for nested calls.
-  // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+struct F {
+  F(int x) {}
+  ~F() {}
 
-namespace C {
-  int add(int x, int y){ return x + y; }
-}
+  int get() { return 42; }
+};
 
-void n() {
-  auto clj = std::bind(C::add, 1, 1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+void UseF(F);
+
+struct placeholder {};
+placeholder _1;
+placeholder _2;
+
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -114,10 +101,213 @@
   void Reset(std::function);
 };
 
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+  int MemberVariable;
+  static int StaticMemberVariable;
+  F MemberStruct;
+
+  void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through

[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Zachary Turner via Phabricator via cfe-commits]

zturner updated this revision to Diff 230661.
zturner added a comment.

Addressed suggestions from @Eugene.Zelenko


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

https://reviews.llvm.org/D70368

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -8,75 +8,62 @@
 template 
 bind_rt bind(Fp &&, Arguments &&...);
 }
+
+template 
+T ref(T &t);
 }
 
-int add(int x, int y) { return x + y; }
+namespace boost {
+template 
+class bind_rt {};
 
-void f() {
-  auto clj = std::bind(add, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
-  // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template 
+bind_rt bind(const Fp &, Arguments...);
 
-void g() {
-  int x = 2;
-  int y = 2;
-  auto clj = std::bind(add, x, y);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=] { return add(x, y); };
+template 
+struct reference_wrapper {
+  explicit reference_wrapper(T &t) {}
+};
+
+template 
+reference_wrapper const ref(T &t) {
+  return reference_wrapper(t);
 }
 
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+} // namespace boost
 
-void h() {
-  int x = 2;
-  auto clj = std::bind(add, x, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
 
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+struct Foo {
+  static int add(int x, int y) { return x + y; }
+};
 
-void i() {
-  auto BATest = std::bind(ABTest, _2, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+struct D {
+  D() = default;
+  void operator()(int x, int y) const {}
 
-void j() {
-  auto clj = std::bind(add, 2, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for argument mismatches.
-  // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+  void MemberFunction(int x) {}
 
-void k() {
-  auto clj = std::bind(add, _1, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for reused placeholders.
-  // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+  static D *create();
+};
 
-void m() {
-  auto clj = std::bind(add, 1, add(2, 5));
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for nested calls.
-  // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+struct F {
+  F(int x) {}
+  ~F() {}
 
-namespace C {
-  int add(int x, int y){ return x + y; }
-}
+  int get() { return 42; }
+};
 
-void n() {
-  auto clj = std::bind(C::add, 1, 1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+void UseF(F);
+
+struct placeholder {};
+placeholder _1;
+placeholder _2;
+
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -114,10 +101,213 @@
   void Reset(std::function);
 };
 
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+  int MemberVariable;
+  static int StaticMemberVariable;
+  F MemberStruct;
+
+  void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through this.
+auto CCC = std::bind(add, MemberVariable, GlobalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-

[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Zachary Turner via Phabricator via cfe-commits]

zturner updated this revision to Diff 230495.
zturner added a comment.

- Updated documentation for this check
- Incorporated additional suggestions from @aaron.ballman
- Fixed an invalid transformation that was generated when binding a member 
function and the second argument of `bind` (the object pointer) was a 
placeholder.  Test is added for this case as well.
- Fixed an invalid transformation that was generated when a placeholder index 
was entirely skipped, as in the call `std::bind(add, 0, _2);`  In this case we 
need to generate an unused placeholder in the first position of the resulting 
lambda's parameter list.
- Added a clang-tidy option `PermissiveParameterList` which appends `auto&&...` 
to the end of every lambda's placeholder list.  This is necessary in some 
situations to prevent clang-tidy from applying a fixit that causes the code to 
no longer compile.


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

https://reviews.llvm.org/D70368

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
  clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -8,75 +8,62 @@
 template 
 bind_rt bind(Fp &&, Arguments &&...);
 }
+
+template 
+T ref(T &t);
 }
 
-int add(int x, int y) { return x + y; }
+namespace boost {
+template 
+class bind_rt {};
 
-void f() {
-  auto clj = std::bind(add, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
-  // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template 
+bind_rt bind(const Fp &, Arguments...);
 
-void g() {
-  int x = 2;
-  int y = 2;
-  auto clj = std::bind(add, x, y);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=] { return add(x, y); };
+template 
+struct reference_wrapper {
+  explicit reference_wrapper(T &t) {}
+};
+
+template 
+reference_wrapper const ref(T &t) {
+  return reference_wrapper(t);
 }
 
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+} // namespace boost
 
-void h() {
-  int x = 2;
-  auto clj = std::bind(add, x, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
 
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+struct Foo {
+  static int add(int x, int y) { return x + y; }
+};
 
-void i() {
-  auto BATest = std::bind(ABTest, _2, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+struct D {
+  D() = default;
+  void operator()(int x, int y) const {}
 
-void j() {
-  auto clj = std::bind(add, 2, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for argument mismatches.
-  // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+  void MemberFunction(int x) {}
 
-void k() {
-  auto clj = std::bind(add, _1, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for reused placeholders.
-  // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+  static D *create();
+};
 
-void m() {
-  auto clj = std::bind(add, 1, add(2, 5));
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for nested calls.
-  // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+struct F {
+  F(int x) {}
+  ~F() {}
 
-namespace C {
-  int add(int x, int y){ return x + y; }
-}
+  int get() { return 42; }
+};
 
-void n() {
-  auto clj = std::bind(C::add, 1, 1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+void UseF(F);
+
+struct placeholder {};
+placeholder _1;
+placeholder _2;
+
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -114,10 +101,213 @@
   void Reset(std::function);
 };
 
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+  int MemberVariable;
+  static int StaticMemberVariable;
+  F MemberStruct;
+
+  void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16

[PATCH] D70553: [clang-apply-replacements] Add command line option to overwrite readonly files.

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
zturner added reviewers: aaron.ballman, alexfh, hokein.
zturner added a project: clang-tools-extra.
Herald added a project: clang.

Some source code control systems attempt to prevent you from editing files 
unless you explicitly check them out.  This makes it impossible to use certain 
refactoring tools such as this, since only the tool itself is able to determine 
the set of files that need to be modified.  This patch adds a `--force` option 
which clears the read-only bit of the file so that it can be modified.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D70553

Files:
  clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp


Index: 
clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- 
clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ 
clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -48,6 +48,10 @@
  "Use -style to choose formatting style.\n"),
 cl::cat(FormattingCategory));
 
+static cl::opt ForceOverwriteReadOnly(
+"force", cl::desc("Overwrite read-only files when applying 
replacements\n"),
+cl::init(false), cl::cat(ReplacementCategory));
+
 // FIXME: Consider making the default behaviour for finding a style
 // configuration file to start the search anew for every file being changed to
 // handle situations where the style is different for different parts of a
@@ -152,6 +156,13 @@
 
 // Write new file to disk
 std::error_code EC;
+if (ForceOverwriteReadOnly) {
+  using namespace llvm::sys::fs;
+  if (auto ErrorOrPerms = getPermissions(FileName)) {
+perms P = ErrorOrPerms.get();
+setPermissions(FileName, P | all_write);
+  }
+}
 llvm::raw_fd_ostream FileStream(FileName, EC, llvm::sys::fs::OF_None);
 if (EC) {
   llvm::errs() << "Could not open " << FileName << " for writing\n";


Index: clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -48,6 +48,10 @@
  "Use -style to choose formatting style.\n"),
 cl::cat(FormattingCategory));
 
+static cl::opt ForceOverwriteReadOnly(
+"force", cl::desc("Overwrite read-only files when applying replacements\n"),
+cl::init(false), cl::cat(ReplacementCategory));
+
 // FIXME: Consider making the default behaviour for finding a style
 // configuration file to start the search anew for every file being changed to
 // handle situations where the style is different for different parts of a
@@ -152,6 +156,13 @@
 
 // Write new file to disk
 std::error_code EC;
+if (ForceOverwriteReadOnly) {
+  using namespace llvm::sys::fs;
+  if (auto ErrorOrPerms = getPermissions(FileName)) {
+perms P = ErrorOrPerms.get();
+setPermissions(FileName, P | all_write);
+  }
+}
 llvm::raw_fd_ostream FileStream(FileName, EC, llvm::sys::fs::OF_None);
 if (EC) {
   llvm::errs() << "Could not open " << FileName << " for writing\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Zachary Turner via Phabricator via cfe-commits]

zturner updated this revision to Diff 230162.
zturner added a comment.

Forgot to remove spurious `llvm::` namespace qualifications.


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

https://reviews.llvm.org/D70368

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -8,75 +8,51 @@
 template 
 bind_rt bind(Fp &&, Arguments &&...);
 }
+
+template 
+T ref(T &t);
 }
 
-int add(int x, int y) { return x + y; }
+namespace boost {
+template 
+class bind_rt {};
 
-void f() {
-  auto clj = std::bind(add, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
-  // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template 
+bind_rt bind(const Fp &, Arguments...);
+} // namespace boost
 
-void g() {
-  int x = 2;
-  int y = 2;
-  auto clj = std::bind(add, x, y);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=] { return add(x, y); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
 
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+struct Foo {
+  static int add(int x, int y) { return x + y; }
+};
 
-void h() {
-  int x = 2;
-  auto clj = std::bind(add, x, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+struct D {
+  D() = default;
+  void operator()(int x, int y) const {}
 
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+  void MemberFunction(int x) {}
 
-void i() {
-  auto BATest = std::bind(ABTest, _2, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+  static D *create();
+};
 
-void j() {
-  auto clj = std::bind(add, 2, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for argument mismatches.
-  // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+struct F {
+  F(int x) {}
+  ~F() {}
 
-void k() {
-  auto clj = std::bind(add, _1, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for reused placeholders.
-  // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+  int get() { return 42; }
+};
 
-void m() {
-  auto clj = std::bind(add, 1, add(2, 5));
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for nested calls.
-  // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+void UseF(F);
 
-namespace C {
-  int add(int x, int y){ return x + y; }
-}
+struct placeholder {};
+placeholder _1;
+placeholder _2;
 
-void n() {
-  auto clj = std::bind(C::add, 1, 1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -114,10 +90,188 @@
   void Reset(std::function);
 };
 
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+  int MemberVariable;
+  static int StaticMemberVariable;
+  F MemberStruct;
+
+  void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through this.
+auto CCC = std::bind(add, MemberVariable, GlobalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto CCC = [this] { return add(MemberVariable, GlobalVariable); };
+
+// Static member variables shouldn't be captured, but locals should
+auto DDD = std::bind(add, TestCaptureByValueStruct::StaticMemberVariable, LocalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHEC

[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Zachary Turner via Phabricator via cfe-commits]

zturner updated this revision to Diff 230161.
zturner added a comment.

Updated with suggestions from @aaron.ballman


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

https://reviews.llvm.org/D70368

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -8,75 +8,51 @@
 template 
 bind_rt bind(Fp &&, Arguments &&...);
 }
+
+template 
+T ref(T &t);
 }
 
-int add(int x, int y) { return x + y; }
+namespace boost {
+template 
+class bind_rt {};
 
-void f() {
-  auto clj = std::bind(add, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
-  // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template 
+bind_rt bind(const Fp &, Arguments...);
+} // namespace boost
 
-void g() {
-  int x = 2;
-  int y = 2;
-  auto clj = std::bind(add, x, y);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=] { return add(x, y); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
 
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+struct Foo {
+  static int add(int x, int y) { return x + y; }
+};
 
-void h() {
-  int x = 2;
-  auto clj = std::bind(add, x, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+struct D {
+  D() = default;
+  void operator()(int x, int y) const {}
 
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+  void MemberFunction(int x) {}
 
-void i() {
-  auto BATest = std::bind(ABTest, _2, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+  static D *create();
+};
 
-void j() {
-  auto clj = std::bind(add, 2, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for argument mismatches.
-  // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+struct F {
+  F(int x) {}
+  ~F() {}
 
-void k() {
-  auto clj = std::bind(add, _1, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for reused placeholders.
-  // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+  int get() { return 42; }
+};
 
-void m() {
-  auto clj = std::bind(add, 1, add(2, 5));
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for nested calls.
-  // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+void UseF(F);
 
-namespace C {
-  int add(int x, int y){ return x + y; }
-}
+struct placeholder {};
+placeholder _1;
+placeholder _2;
 
-void n() {
-  auto clj = std::bind(C::add, 1, 1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -114,10 +90,188 @@
   void Reset(std::function);
 };
 
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+  int MemberVariable;
+  static int StaticMemberVariable;
+  F MemberStruct;
+
+  void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto BBB = [x] { return add(x, x); };
+
+int LocalVariable;
+// Global variables shouldn't be captured at all, and members should be captured through this.
+auto CCC = std::bind(add, MemberVariable, GlobalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto CCC = [this] { return add(MemberVariable, GlobalVariable); };
+
+// Static member variables shouldn't be captured, but locals should
+auto DDD = std::bind(add, TestCaptureByValueStruct::StaticMemberVariable, LocalVariable);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto DD

[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Zachary Turner via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:116
+static const Expr *ignoreTemporariesAndImplicitCasts(const Expr *E) {
+  if (const auto *T = dyn_cast(E))
+return ignoreTemporariesAndImplicitCasts(T->GetTemporaryExpr());

aaron.ballman wrote:
> What about `CXXBindTemporaryExpr`?
> 
> Would `Expr::IgnoreImplicit()` do too much stripping because it removes 
> `FullExpr` as well?
I don't actually know.  This patch is literally my first exposure to clang's 
frontend and AST manipulations, so I was kind of just trying different things 
until something worked here.  I didn't even know about `IgnoreImplicit()` or 
`FullExpr`.  I'll play around with them and report back.



Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:153
+
+  return HNM.matchesNode(*dyn_cast(D));
+}

aaron.ballman wrote:
> This should probably use `cast<>` if it's going to assume the returned value 
> is never null.
Good point.  Since we're talking about this code anyway, it felt super hacky to 
instantiate an AST matcher just to check for the qualified name of a Decl.  Is 
there a better way to do this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70368



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

[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
zturner added reviewers: aaron.ballman, dblaikie, jbcoe, NoQ.
Herald added a subscriber: xazax.hun.

This represents largely a full re-write of modernize-avoid-bind, adding 
significant new functionality in the process.  In particular:

1. Both boost::bind and std::bind are now supported
2. Function objects are supported in addition to functions
3. Member functions are supported
4. Nested calls are supported using capture-init syntax
5. `std::ref()` and `boost::ref()` are now recognized, and will capture by 
reference.
6. Rather than capturing with a global `=`, we now build up an individual 
capture list that is both necessary and sufficient for the call.
7. Fixits are supported in a much larger variety of scenarios than before.

All previous tests pass under the re-write, but a large number of new tests 
have been added as well.

I don't know who the best person to review this is, so I've put a couple of 
people on here.  Feel free to re-direct if there's someone better.


https://reviews.llvm.org/D70368

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -8,75 +8,51 @@
 template 
 bind_rt bind(Fp &&, Arguments &&...);
 }
+
+template 
+T ref(T &t);
 }
 
-int add(int x, int y) { return x + y; }
+namespace boost {
+template 
+class bind_rt {};
 
-void f() {
-  auto clj = std::bind(add, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
-  // CHECK-FIXES: auto clj = [] { return add(2, 2); };
-}
+template 
+bind_rt bind(const Fp &, Arguments...);
+} // namespace boost
 
-void g() {
-  int x = 2;
-  int y = 2;
-  auto clj = std::bind(add, x, y);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=] { return add(x, y); };
-}
+namespace C {
+int add(int x, int y) { return x + y; }
+} // namespace C
 
-struct placeholder {};
-placeholder _1;
-placeholder _2;
+struct Foo {
+  static int add(int x, int y) { return x + y; }
+};
 
-void h() {
-  int x = 2;
-  auto clj = std::bind(add, x, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
-}
+struct D {
+  D() = default;
+  void operator()(int x, int y) const {}
 
-struct A;
-struct B;
-bool ABTest(const A &, const B &);
+  void MemberFunction(int x) {}
 
-void i() {
-  auto BATest = std::bind(ABTest, _2, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
-}
+  static D *create();
+};
 
-void j() {
-  auto clj = std::bind(add, 2, 2, 2);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for argument mismatches.
-  // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
-}
+struct F {
+  F(int x) {}
+  ~F() {}
 
-void k() {
-  auto clj = std::bind(add, _1, _1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for reused placeholders.
-  // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
-}
+  int get() { return 42; }
+};
 
-void m() {
-  auto clj = std::bind(add, 1, add(2, 5));
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // No fix is applied for nested calls.
-  // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
-}
+void UseF(F);
 
-namespace C {
-  int add(int x, int y){ return x + y; }
-}
+struct placeholder {};
+placeholder _1;
+placeholder _2;
 
-void n() {
-  auto clj = std::bind(C::add, 1, 1);
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
-}
+int add(int x, int y) { return x + y; }
+int addThree(int x, int y, int z) { return x + y + z; }
 
 // Let's fake a minimal std::function-like facility.
 namespace std {
@@ -114,10 +90,188 @@
   void Reset(std::function);
 };
 
-void test(Thing *t) {
+int GlobalVariable = 42;
+
+struct TestCaptureByValueStruct {
+  int MemberVariable;
+  static int StaticMemberVariable;
+  F MemberStruct;
+
+  void testCaptureByValue(int Param, F f) {
+int x = 3;
+int y = 4;
+auto AAA = std::bind(add, x, y);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+// CHECK-FIXES: auto AAA = [x, y] { return add(x, y); };
+
+// When the captured variable is repeated, it should only appear in the capture list once.
+auto BBB = std::bind(add, x, x);
+

[PATCH] D69872: Improve modernize-avoid-bind to support more types of expressions

[Zachary Turner via Phabricator via cfe-commits]

zturner abandoned this revision.
zturner added a comment.

I have a more comprehensive version of this patch that I'll upload separately.


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

https://reviews.llvm.org/D69872



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

[PATCH] D69825: [Clang][Driver] Bypass cc1 process and re-enter driver main

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Does this change crash recovery semantics in any meaningful way?  Will we still 
be able to get stack traces on all platforms when the compiler crashes?




Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:207
+  // FIXME error: cannot compile this 'this' captured by SEH yet
+  CrashRecoveryContext *This = this;
   __try {

You can fix this by writing:

```
static bool wrap_function_call(function_ref Fn, bool 
EnableExceptionHandler, unsigned &RetCode)
{
   __try {
 Fn();
 return true;
  } __except (EnableExceptionHandler
  ? LLVMUnhandledExceptionFilter(GetExceptionInformation())
  : 1) {
RetCode = GetExceptionCode();
return false;
  }
}

bool CrashRecoveryContext::RunSafely(function_ref Fn) {
...
bool Result = wrap_function_call(EnableExceptionHandler, Fn, RetCode);
...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D69872: Improve modernize-avoid-bind to support more types of expressions

[Zachary Turner via Phabricator via cfe-commits]

zturner updated this revision to Diff 228093.
zturner added a comment.

Minor cleanup -- moved `isFixitSupported` logic to its own function.


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

https://reviews.llvm.org/D69872

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -78,6 +78,39 @@
   // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
 }
 
+struct D {
+  void operator()(int x, int y) {}
+
+  void MemberFunction(int x) {}
+};
+
+void o() {
+  D d;
+  auto clj = std::bind(d, 1, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(d, 1, 2);
+}
+
+void p() {
+  auto clj = std::bind(D{}, 1, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(D{}, 1, 2);
+}
+
+void q() {
+  D *d;
+  auto clj = std::bind(*d, 1, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(*d, 1, 2);
+}
+
+void r() {
+  D *d;
+  auto clj = std::bind(&D::MemberFunction, d, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(&D::MemberFunction, d, 1);
+}
+
 // Let's fake a minimal std::function-like facility.
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
@@ -120,41 +120,49 @@
   if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas.
 return;
 
-  Finder->addMatcher(
-  callExpr(
-  callee(namedDecl(hasName("::std::bind"))),
-  hasArgument(0, declRefExpr(to(functionDecl().bind("f"))).bind("ref")))
-  .bind("bind"),
-  this);
+  Finder->addMatcher(callExpr(callee(namedDecl(hasName("::std::bind"))),
+  hasArgument(0, expr().bind("ref")))
+ .bind("bind"),
+ this);
 }
 
-void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *MatchedDecl = Result.Nodes.getNodeAs("bind");
-  auto Diag = diag(MatchedDecl->getBeginLoc(), "prefer a lambda to std::bind");
-
-  const auto Args = buildBindArguments(Result, MatchedDecl);
-
+static bool isFixitSupported(const Expr *Expr, ArrayRef Args) {
   // Do not attempt to create fixits for nested call expressions.
   // FIXME: Create lambda capture variables to capture output of calls.
   // NOTE: Supporting nested std::bind will be more difficult due to placeholder
   // sharing between outer and inner std:bind invocations.
   if (llvm::any_of(Args,
[](const BindArgument &B) { return B.Kind == BK_CallExpr; }))
-return;
+return false;
 
   // Do not attempt to create fixits when placeholders are reused.
   // Unused placeholders are supported by requiring C++14 generic lambdas.
   // FIXME: Support this case by deducing the common type.
   if (isPlaceHolderIndexRepeated(Args))
-return;
+return false;
+
+  if (const auto *DRE = llvm::dyn_cast(Expr)) {
+const auto *FD = llvm::dyn_cast_or_null(DRE->getDecl());
+// std::bind can support argument count mismatch between its arguments and
+// the bound function's arguments. Do not attempt to generate a fixit for
+// such cases.
+// FIXME: Support this case by creating unused lambda capture variables.
+if (FD && (FD->getNumParams() == Args.size()))
+  return true;
+  }
 
-  const auto *F = Result.Nodes.getNodeAs("f");
+  // Do not attempt to create fixits for generalized call expressions.
+  return false;
+}
+
+void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs("bind");
+  auto Diag = diag(MatchedDecl->getBeginLoc(), "prefer a lambda to std::bind");
+
+  const auto Args = buildBindArguments(Result, MatchedDecl);
 
-  // std::bind can support argument count mismatch between its arguments and the
-  // bound function's arguments. Do not attempt to generate a fixit for such
-  // cases.
-  // FIXME: Support this case by creating unused lambda capture variables.
-  if (F->getNumParams() != Args.size())
+  const auto *Ref = Result.Nodes.getNodeAs("ref");
+  if (!isFixitSupported(Ref, Args))
 return;
 
   std::string Buffer;
@@ -162,7 +170,6 @@
 
   bool HasCapturedArgument = llvm::any_of(
   Args, [](const BindArgument &B) { return B.Kind == BK_Other; });
-  const auto *Ref = Result.Nodes.ge

[PATCH] D69872: Improve modernize-avoid-bind to support more types of expressions

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
zturner added reviewers: aaron.ballman, jbcoe.

Previously modernize-avoid-bind only supported the case where the function to 
call was a FunctionDecl.  This patch makes it support arbitrary expressions, 
including functors, member functions, and combinations thereof.

This change actually *simplifies* the code rather than complicates it, because 
it assumes that the first argument to std::bind() is always a callable, 
otherwise it wouldn't even compile.  So rather than limiting ourselves to 
DeclRefExprs, we just accept any kind of expression for the first argument.  
Fixits are still only applied in the same set of limited cases as before, 
although I plan to improve this in followup patches.


https://reviews.llvm.org/D69872

Files:
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp
@@ -78,6 +78,39 @@
   // CHECK-FIXES: auto clj = [] { return C::add(1, 1); };
 }
 
+struct D {
+  void operator()(int x, int y) {}
+
+  void MemberFunction(int x) {}
+};
+
+void o() {
+  D d;
+  auto clj = std::bind(d, 1, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(d, 1, 2);
+}
+
+void p() {
+  auto clj = std::bind(D{}, 1, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(D{}, 1, 2);
+}
+
+void q() {
+  D *d;
+  auto clj = std::bind(*d, 1, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(*d, 1, 2);
+}
+
+void r() {
+  D *d;
+  auto clj = std::bind(&D::MemberFunction, d, 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
+  // CHECK-FIXES: auto clj = std::bind(&D::MemberFunction, d, 1);
+}
+
 // Let's fake a minimal std::function-like facility.
 namespace std {
 template 
Index: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
@@ -120,12 +120,10 @@
   if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas.
 return;
 
-  Finder->addMatcher(
-  callExpr(
-  callee(namedDecl(hasName("::std::bind"))),
-  hasArgument(0, declRefExpr(to(functionDecl().bind("f"))).bind("ref")))
-  .bind("bind"),
-  this);
+  Finder->addMatcher(callExpr(callee(namedDecl(hasName("::std::bind"))),
+  hasArgument(0, expr().bind("ref")))
+ .bind("bind"),
+ this);
 }
 
 void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
@@ -148,21 +146,27 @@
   if (isPlaceHolderIndexRepeated(Args))
 return;
 
-  const auto *F = Result.Nodes.getNodeAs("f");
-
-  // std::bind can support argument count mismatch between its arguments and the
-  // bound function's arguments. Do not attempt to generate a fixit for such
-  // cases.
-  // FIXME: Support this case by creating unused lambda capture variables.
-  if (F->getNumParams() != Args.size())
+  const auto *Ref = Result.Nodes.getNodeAs("ref");
+  if (Ref) {
+const auto *FD = llvm::dyn_cast_or_null(Ref->getFoundDecl());
+
+// std::bind can support argument count mismatch between its arguments and
+// the bound function's arguments. Do not attempt to generate a fixit for
+// such cases.
+// FIXME: Support this case by creating unused lambda capture variables.
+if (!FD || (FD->getNumParams() != Args.size()))
+  return;
+  } else {
+// Don't support fixits for generalized call expressions.
+// FIXME: Support fixits for member function pointers as well as call objects.
 return;
+  }
 
   std::string Buffer;
   llvm::raw_string_ostream Stream(Buffer);
 
   bool HasCapturedArgument = llvm::any_of(
   Args, [](const BindArgument &B) { return B.Kind == BK_Other; });
-  const auto *Ref = Result.Nodes.getNodeAs("ref");
   Stream << "[" << (HasCapturedArgument ? "=" : "") << "]";
   addPlaceholderArgs(Args, Stream);
   Stream << " { return ";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68736: [MSVC] Automatically add atlmfc include and lib directories as system paths.

[Zachary Turner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG79f243296654: [MSVC] Automatically add atlmfc folder to 
include and libpath. (authored by zturner).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68736

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MSVC.h


Index: clang/lib/Driver/ToolChains/MSVC.h
===
--- clang/lib/Driver/ToolChains/MSVC.h
+++ clang/lib/Driver/ToolChains/MSVC.h
@@ -98,12 +98,14 @@
 Lib,
   };
   std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent,
   llvm::Triple::ArchType TargetArch) const;
 
   // Convenience overload.
   // Uses the current target arch.
-  std::string getSubDirectoryPath(SubDirectoryType Type) const {
-return getSubDirectoryPath(Type, getArch());
+  std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent = "") const {
+return getSubDirectoryPath(Type, SubdirParent, getArch());
   }
 
   enum class ToolsetLayout {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -331,6 +331,11 @@
 TC.getSubDirectoryPath(
 toolchains::MSVCToolChain::SubDirectoryType::Lib)));
 
+CmdArgs.push_back(Args.MakeArgString(
+Twine("-libpath:") +
+
TC.getSubDirectoryPath(toolchains::MSVCToolChain::SubDirectoryType::Lib,
+   "atlmfc")));
+
 if (TC.useUniversalCRT()) {
   std::string UniversalCRTLibPath;
   if (TC.getUniversalCRTLibraryPath(UniversalCRTLibPath))
@@ -548,7 +553,7 @@
   EnvVar.substr(0, PrefixLen) +
   TC.getSubDirectoryPath(SubDirectoryType::Bin) +
   llvm::Twine(llvm::sys::EnvPathSeparator) +
-  TC.getSubDirectoryPath(SubDirectoryType::Bin, HostArch) +
+  TC.getSubDirectoryPath(SubDirectoryType::Bin, "", HostArch) +
   (EnvVar.size() > PrefixLen
? llvm::Twine(llvm::sys::EnvPathSeparator) +
  EnvVar.substr(PrefixLen)
@@ -824,6 +829,7 @@
 // of hardcoding paths.
 std::string
 MSVCToolChain::getSubDirectoryPath(SubDirectoryType Type,
+   llvm::StringRef SubdirParent,
llvm::Triple::ArchType TargetArch) const {
   const char *SubdirName;
   const char *IncludeName;
@@ -843,6 +849,9 @@
   }
 
   llvm::SmallString<256> Path(VCToolChainPath);
+  if (!SubdirParent.empty())
+llvm::sys::path::append(Path, SubdirParent);
+
   switch (Type) {
   case SubDirectoryType::Bin:
 if (VSLayout == ToolsetLayout::VS2017OrNewer) {
@@ -1228,6 +1237,8 @@
   if (!VCToolChainPath.empty()) {
 addSystemInclude(DriverArgs, CC1Args,
  getSubDirectoryPath(SubDirectoryType::Include));
+addSystemInclude(DriverArgs, CC1Args,
+ getSubDirectoryPath(SubDirectoryType::Include, "atlmfc"));
 
 if (useUniversalCRT()) {
   std::string UniversalCRTSdkPath;


Index: clang/lib/Driver/ToolChains/MSVC.h
===
--- clang/lib/Driver/ToolChains/MSVC.h
+++ clang/lib/Driver/ToolChains/MSVC.h
@@ -98,12 +98,14 @@
 Lib,
   };
   std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent,
   llvm::Triple::ArchType TargetArch) const;
 
   // Convenience overload.
   // Uses the current target arch.
-  std::string getSubDirectoryPath(SubDirectoryType Type) const {
-return getSubDirectoryPath(Type, getArch());
+  std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent = "") const {
+return getSubDirectoryPath(Type, SubdirParent, getArch());
   }
 
   enum class ToolsetLayout {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -331,6 +331,11 @@
 TC.getSubDirectoryPath(
 toolchains::MSVCToolChain::SubDirectoryType::Lib)));
 
+CmdArgs.push_back(Args.MakeArgString(
+Twine("-libpath:") +
+TC.getSubDirectoryPath(toolchains::MSVCToolChain::SubDirectoryType::Lib,
+   "atlmfc")));
+
 if (TC.useUniversalCRT()) {
   std::string UniversalCRTLibPath;
   if (TC.getUniversalCRTLibraryPath(UniversalCRTLibPath))
@@ -548,7 +553,7 @@
   EnvVar.substr(0, PrefixLen) +
   TC.getSubDirectoryPath(SubDirector

[PATCH] D29039: [clang-format] Proposal for clang-format -r option

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In Windows you just write a Python script to do this, and this works 
everywhere, not just on one platform or the other, so bash isn't even necessary 
and Python is easy to write so I wouldn't really say it's "even harder".  If 
you google for `run-clang-format.py` you'll find a script that actually 
branches out and does this in parallel.  There's a lot of logic and smarts you 
could bake into an external tool which can then be used for many different 
programs, not just clang-format.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D29039



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

[PATCH] D68736: [MSVC] Automatically add atlmfc include and lib directories as system paths.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D68736#1702482 , @amccarth wrote:

> > This matches the behavior of cl.
>
> Are you sure?  In a bare environment, cl.exe doesn't include any system 
> paths, not even to the standard library.  It actually uses the INCLUDE 
> environment variable for those paths.  Granted, VCVARSALL sets that (and 
> other environment variables), but it's not innate behavior of cl.exe.


Right, sorry.  I guess what I mean is that this matches the behavior of running 
from a cl command prompt, which is what we do elsewhere.  This is already in 
the codepath that is trying to build up a consistent WindowsSdk + CRT 
environment, we were just missing the atlmfc part.


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

https://reviews.llvm.org/D68736



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

[PATCH] D68736: [MSVC] Automatically add atlmfc include and lib directories as system paths.

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
zturner added a reviewer: rnk.

This matches the behavior of cl.


https://reviews.llvm.org/D68736

Files:
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MSVC.h


Index: clang/lib/Driver/ToolChains/MSVC.h
===
--- clang/lib/Driver/ToolChains/MSVC.h
+++ clang/lib/Driver/ToolChains/MSVC.h
@@ -98,12 +98,14 @@
 Lib,
   };
   std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent,
   llvm::Triple::ArchType TargetArch) const;
 
   // Convenience overload.
   // Uses the current target arch.
-  std::string getSubDirectoryPath(SubDirectoryType Type) const {
-return getSubDirectoryPath(Type, getArch());
+  std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent = "") const {
+return getSubDirectoryPath(Type, SubdirParent, getArch());
   }
 
   enum class ToolsetLayout {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -331,6 +331,11 @@
 TC.getSubDirectoryPath(
 toolchains::MSVCToolChain::SubDirectoryType::Lib)));
 
+CmdArgs.push_back(Args.MakeArgString(
+Twine("-libpath:") +
+
TC.getSubDirectoryPath(toolchains::MSVCToolChain::SubDirectoryType::Lib,
+   "atlmfc")));
+
 if (TC.useUniversalCRT()) {
   std::string UniversalCRTLibPath;
   if (TC.getUniversalCRTLibraryPath(UniversalCRTLibPath))
@@ -548,7 +553,7 @@
   EnvVar.substr(0, PrefixLen) +
   TC.getSubDirectoryPath(SubDirectoryType::Bin) +
   llvm::Twine(llvm::sys::EnvPathSeparator) +
-  TC.getSubDirectoryPath(SubDirectoryType::Bin, HostArch) +
+  TC.getSubDirectoryPath(SubDirectoryType::Bin, "", HostArch) +
   (EnvVar.size() > PrefixLen
? llvm::Twine(llvm::sys::EnvPathSeparator) +
  EnvVar.substr(PrefixLen)
@@ -824,6 +829,7 @@
 // of hardcoding paths.
 std::string
 MSVCToolChain::getSubDirectoryPath(SubDirectoryType Type,
+   llvm::StringRef SubdirParent,
llvm::Triple::ArchType TargetArch) const {
   const char *SubdirName;
   const char *IncludeName;
@@ -843,6 +849,9 @@
   }
 
   llvm::SmallString<256> Path(VCToolChainPath);
+  if (!SubdirParent.empty())
+llvm::sys::path::append(Path, SubdirParent);
+
   switch (Type) {
   case SubDirectoryType::Bin:
 if (VSLayout == ToolsetLayout::VS2017OrNewer) {
@@ -1228,6 +1237,8 @@
   if (!VCToolChainPath.empty()) {
 addSystemInclude(DriverArgs, CC1Args,
  getSubDirectoryPath(SubDirectoryType::Include));
+addSystemInclude(DriverArgs, CC1Args,
+ getSubDirectoryPath(SubDirectoryType::Include, "atlmfc"));
 
 if (useUniversalCRT()) {
   std::string UniversalCRTSdkPath;


Index: clang/lib/Driver/ToolChains/MSVC.h
===
--- clang/lib/Driver/ToolChains/MSVC.h
+++ clang/lib/Driver/ToolChains/MSVC.h
@@ -98,12 +98,14 @@
 Lib,
   };
   std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent,
   llvm::Triple::ArchType TargetArch) const;
 
   // Convenience overload.
   // Uses the current target arch.
-  std::string getSubDirectoryPath(SubDirectoryType Type) const {
-return getSubDirectoryPath(Type, getArch());
+  std::string getSubDirectoryPath(SubDirectoryType Type,
+  llvm::StringRef SubdirParent = "") const {
+return getSubDirectoryPath(Type, SubdirParent, getArch());
   }
 
   enum class ToolsetLayout {
Index: clang/lib/Driver/ToolChains/MSVC.cpp
===
--- clang/lib/Driver/ToolChains/MSVC.cpp
+++ clang/lib/Driver/ToolChains/MSVC.cpp
@@ -331,6 +331,11 @@
 TC.getSubDirectoryPath(
 toolchains::MSVCToolChain::SubDirectoryType::Lib)));
 
+CmdArgs.push_back(Args.MakeArgString(
+Twine("-libpath:") +
+TC.getSubDirectoryPath(toolchains::MSVCToolChain::SubDirectoryType::Lib,
+   "atlmfc")));
+
 if (TC.useUniversalCRT()) {
   std::string UniversalCRTLibPath;
   if (TC.getUniversalCRTLibraryPath(UniversalCRTLibPath))
@@ -548,7 +553,7 @@
   EnvVar.substr(0, PrefixLen) +
   TC.getSubDirectoryPath(SubDirectoryType::Bin) +
   llvm::Twine(llvm::sys::EnvPathSeparator) +
-  TC.getSubDirectoryPath(SubDirectoryType::Bin, HostArch) +
+  TC.getSubDirectoryPath(SubDirectoryType::Bin, "", HostArch) +
   

[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

[Zachary Turner via Phabricator via cfe-commits]

zturner added a reviewer: krasimir.
zturner added a comment.

What's the status of this patch, out of curiosity?  It doesn't seem there were 
any objections to the original idea, just that nobody with ownership over 
clang-format is still actively participating in the review.

+krasimir to maybe shed some light on whether this can move forward.


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

https://reviews.llvm.org/D44609



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

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

[Zachary Turner via Phabricator via cfe-commits]

zturner added reviewers: rnk, hans.
zturner added a comment.

+reid and hans, as they might be interested in this.


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

https://reviews.llvm.org/D58675



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

[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Probably @rnk needs to look at this, i'll ping him today.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58544



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

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1406401 , @probinson wrote:

> In D57896#1406353 , @MyDeveloperDay 
> wrote:
>
> > I can't argue with anything you say.. but I guess to reinforce your point 
> > introducing what is effectively a 3rd style would likely cause even more 
> > jarring...
>
>
> Zach isn't introducing a new style, the style already exists and is 
> consistently used by what I think is our 3rd largest subproject.  It happens 
> not to be used at all by the two largest subprojects, but those subprojects 
> already aren't consistent with themselves.
>  I would not mind a more concerted effort to migrate to whatever style we 
> pick, which was notably lacking last time around. Then the jarring 
> inconsistencies would go away, and we could all get back to complaining about 
> content and not style.


If I read the post correctly, it was actually agreeing with me (because it said 
"to reinforce your point...".  Meaning that something such as `lowerCaseCamel` 
would be the third style being referred to, while my proposal keeps the number 
of styles to 2.  But, maybe I read it wrong.  If I read it right, then 
obviously I agree :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1405334 , @MyDeveloperDay 
wrote:

> In D57896#1402280 , @zturner wrote:
>
> > Since someone already accepted this, I suppose I should mark require 
> > changes to formalize my dissent
>
>
> As it was Chris @lattner who accepted it, is your request for changes just 
> based on the fact that it doesn't fit LLDB style?


(Side note, but I think everyones' opinions hold the same weight with regards 
to issues like this, and that is in part why changes like this are so difficult 
to move forward with. Because it takes a lot of consensus, not just one person, 
to drive a change.)

To answer your question: In a way, yes.  To be clear, I don't actually care 
what the style we end up with is and I think arguing over which specific style 
we end up adopting is a silly argument.  No style is going to be aesthetically 
pleasing to everyone, and I conjecture that any style we choose will have just 
as many people who dislike it as there are that like it.  A coding style should 
serve exactly two purposes (in this order of importance): 1) Consistency across 
codebase, and 2) Visually distinguish semantically names that refer to 
semantically different things.

As long as it satisfies those two things, the specific choice of style is 
almost incosequential.

My objection is based on the fact adopting LLDB's style makes #1 
**significantly better** at literally no incremental cost, while maintaining 
#2.  So, the benefit of changing to literally any other style would be dwarfed 
by the benefit of changing to this particular style, because we would get 
instant consistency across a large swath of code.

If someone wants to propose a mass change of LLDB's names, I would actually be 
fine with that, but I suspect that will be just as difficult to drive, and so 
the path of least resistance here is to just use it and move on with our lives.

> I was trying to find where the LLDB coding style was documented but could 
> only find this 
> https://llvm.org/svn/llvm-project/lldb/branches/release_36/www/lldb-coding-conventions.html,
>  seemingly this file has been move/removed around release 3.9.
> 
> But reading that link its seems unlikely to find a concencous between either 
> naming conventions or formatting style between LLDB and the rest of LLVM, 
> unless of course the solution would be to adopt LLDB style completely (for 
> which I'd suspect there would be counter objections)

If there are counter objections, I'd like to hear them.  "I'm not a fan of that 
style" is not really a strong counter-objection in my opinion, because if we 
require a unanimous consensus on the most aesthetically pleasing style, I'm 
pretty sure nothing will ever happen.  After all, I'm not a huge fan of LLDB's 
style myself.  But as with any coding standard, you just deal with it.

> If that isn't a reality is blocking the rest of the LLVM community from 
> relieving some of their eye strain an acceptable solution?

Inconsistency is worse than eye strain, because it *causes* eye strain, as well 
as discourages people from contributing to the code at all.  Anyone who has 
worked on both LLDB and LLVM can attest to how jarring the shift is moving back 
and forth between them, and that is a much more serious problem than a subset 
of developers who don't like something and another subset who do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

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

Since someone already accepted this, I suppose I should mark require changes to 
formalize my dissent


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D57896#1402194 , @lattner wrote:

> > Changed recommendation for acronyms from lower case to upper case, as 
> > suggested by several responses to the RFC.
>
> I haven't been following the discussion closely - why is this the preferred 
> direction?  I don't think that things like "Basicblock *bb" or "MachineInstr 
> *mi" will be confusing, and going towards a consistently leading lower case 
> letter seems simple and preferable.
>
> -Chris


I don’t think we should use this review as evidence of consensus.  For example, 
I’m going to be against any change that doesn’t bring us closer to LLDB’s style 
of `lower_case` simply on the grounds that a move which brings us farther away 
from global consistency is strictly worse than one which brings us closer, 
despite ones personal aesthetic preferences.

And so far, I don’t really see that addressed here (or in the thread)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57896: Variable names rule

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Is this actually any better?  Whereas before we can’t differentiate type names 
and variable names, under this proposal we can’t differentiate type names and 
function names.  So it seems a bit of “6 of 1, half dozen of another”


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57343: lit: Let lit.util.which() return a normcase()ed path

[Zachary Turner via Phabricator via cfe-commits]

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

I think it's possible to detect the case insensitivity of the file system 
itself, rather than relying on the operating system which as you point out is 
neither necessary nor sufficient to identify case insensitivity.  But, also 
like you pointed out, that's another can of worms entirely, so I agree this is 
the simplest thing to make things better than before.


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

https://reviews.llvm.org/D57343



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

[PATCH] D57343: lit: Let lit.util.which() return a normcase()ed path

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Shouldn't the hash be case-insensitive on windows?


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

https://reviews.llvm.org/D57343



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

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

BTW, as far as updating the demangler, as long as it doesn't crash or generate 
an error on a valid `_E` mangling, that should be sufficient (with a test).  If 
you want bonus points you can make it print out `noexcept` when you see the 
`_E`, but I won't require it as it's a bit of extra work.  If you decide not to 
do that, I'll just file a bug for it so that we don't forget.




Comment at: lib/AST/MicrosoftMangle.cpp:2311-2314
+  if (FT->canThrow())
+Out << 'Z';
+  else
+Out << "_E";

rnk wrote:
> zahen wrote:
> > zturner wrote:
> > > I knew that the mangling changed whenever a pointer to a `noexcept` 
> > > function is passed as an argument, and we don't yet handle that, but I'm 
> > > surprised to hear that they changed an existing mangling, since it's a 
> > > hard ABI break.
> > > 
> > > Do you know the major and minor version numbers that this changed in?  
> > > I'd like to test it out for starters, but also since this is an ABI break 
> > > we would need to put it behind `-fms-compatibility-version` and only 
> > > mangle using the new scheme when the compatibility version is 
> > > sufficiently high.
> > It's only when a function is used as a type.  My original rathole was 
> > trying to enumerate all of the places where that could be, but instead I 
> > settled on "everywhere but the initial definition".  It's why false is 
> > passed in the 4th parameter on line 516.
> > 
> > I've confirmed this changed in 15.5 so I'll use that as the compat version.
> I see existing code that uses this pattern: 
> `getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015)`
> 
> The MSVCMajorVersion enum is symbolic, so I think you might have to multiply 
> it by a hundred and modify LangOptions::isCompatibleWithMSVC to multiply by 
> two fewer places.
> 
> I guess to fit with the existing enums we'd say MSVC2017_5, even though that 
> conflates VS and VC version numbers.
Ok, I see it now.  That should be fine.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55685



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

[PATCH] D55715: Add AddressSpace mangling to MS mode

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

It would be interesting to teach `llvm-undname` to demangle this more 
naturally.  Right for this mangling: `?f1@@YAXPEAU?$AS_@$00$$CBD@__clang@@@Z` I 
see this demangling: `void __cdecl f1(struct __clang::AS_<1, char const> *)`.  
That's an accurate representation of the underlying structure, but it would be 
cool if we had special cases for things in the `__clang` namespace, so that we 
could display this as `void __cdecl f1(char __attribute__((address_space(1))) 
const *) {}`
`


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

https://reviews.llvm.org/D55715



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

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Also we still need to put this behind `-fms-compatibility-version`.  Finally, 
it would be nice if you could also update the demangler 
(`llvm/lib/Demangle/MicrosoftDemangle.cpp`)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55685



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

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Ahh I read the new test cases, and all of the test cases are about the case 
where `noexcept` function is a parameter, so maybe this is actually the case I 
was referring to.  Can you add a test case for something like this:

  void f() noexcept { }

I get this:

  00A  SECT4  notype ()External | ?foo@@YAXXZ (void __cdecl 
foo(void))
  00B 0010 SECT4  notype ()External | ?bar@@YAXP6AXX_E@Z (void 
__cdecl bar(void (__cdecl*)(void) noexcept))

Showing that it is in fact only for function parameters.  So we should have a 
test to confirm that the behavior when the `noexcept` function is not a 
parameter remains `Z` (from the code it looks like this might be broken under 
this patch).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55685



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

[PATCH] D55685: Update Microsoft name mangling scheme for exception specifiers in the type system

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/AST/MicrosoftMangle.cpp:2311-2314
+  if (FT->canThrow())
+Out << 'Z';
+  else
+Out << "_E";

I knew that the mangling changed whenever a pointer to a `noexcept` function is 
passed as an argument, and we don't yet handle that, but I'm surprised to hear 
that they changed an existing mangling, since it's a hard ABI break.

Do you know the major and minor version numbers that this changed in?  I'd like 
to test it out for starters, but also since this is an ABI break we would need 
to put it behind `-fms-compatibility-version` and only mangle using the new 
scheme when the compatibility version is sufficiently high.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55685



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

[PATCH] D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Assuming this patch were to go in as-is (which it probably won't, based on the 
feedback, but let's just pretend), that would avoid having to explicitly update 
how many callsites?

What I'm wondering is, how hard would it be to just update the call-sites?

It looks like what you are really trying to do is avoid having to pass 
"IsVolatile" for many call-sites.  But to be honest, IsVolatile exactly 
describes this situation.  You have a file, and another program has the same 
file and it can change it out from underneath you.  That exactly describes the 
meaning of `IsVolatile`.  So, conceptually it makes sense to just find all the 
call-sites where this matters and pass `IsVolatile=true`.  Is there some reason 
we can't just do that here?  Are there too many?


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

https://reviews.llvm.org/D54995



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

[PATCH] D54995: [MemoryBuffer] By default assume that all files are volatile to prevent unintended file locks

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

We can work around it by reading the whole file, but it looks like a bug in 
QtCreator to me.  We have the file mapped, but maybe when they open the file to 
save it, *they* are opening the file without `FILE_SHARE_READ`.  It's a logical 
thing to do on the surface, because they are probably thinking "we are about to 
change the contents of the file, so we don't want anyone to be reading it at 
the same time while we modify it".  But this means their call to `CreateFile()` 
will fail, because we have the file opened for read, and if they want to open 
it for exclusive access, it's not possible.


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

https://reviews.llvm.org/D54995



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

[PATCH] D54995: [MemoryBuffer] By default assume that all files are volatile to prevent unintended file locks

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Original patch description says this:

> There are reported cases of non-system files being locked by libclang on 
> Windows (and likely by other clients as well)

What is the nature of the reports?  What operation is attempted on the files 
and fails due to locking?  And which application is it that's failing and in 
what way?


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

https://reviews.llvm.org/D54995



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

[PATCH] D54995: [MemoryBuffer] By default assume that all files are volatile to prevent unintended file locks

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In D54995#1311399 , @ilya-biryukov 
wrote:

> In D54995#1311303 , @yvvan wrote:
>
> > "could we figure out the exact cause of the issue?"
> >  And this review was not meant to proceed immediately but rather state the 
> > intention and get some feedback because I still don't know answers to the 
> > questions 1 and 2 (did somebody else investigate that?)
>
>
> I tried calling Windows APIs that LLVM uses with multiple combinations of 
> flags and couldn't make it lock the file. But maybe I didn't arrive at the 
> right combination of flags or was trying on a different version.
>
> Could you send a link for the issue itself, I'm not sure how to get from the 
> attachment to an actual issue, would be interested to look at this


My first thought would be that it depends on the flags to CreateFile moreso 
(and perhaps entirely) rather than the flags to MapViewOfFile or 
CreateFileMapping.  Specifically, the `FILE_SHARE_XXX` flags are the most 
relevant here.


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

https://reviews.llvm.org/D54995



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

[PATCH] D54567: Fix LLDB's lit files

[Zachary Turner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347216: Fix some issues with LLDB's lit configuration 
files. (authored by zturner, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54567?vs=174473&id=174610#toc

Repository:
  rC Clang

https://reviews.llvm.org/D54567

Files:
  test/lit.cfg.py


Index: test/lit.cfg.py
===
--- test/lit.cfg.py
+++ test/lit.cfg.py
@@ -43,6 +43,10 @@
 
 llvm_config.use_clang()
 
+config.substitutions.append(
+('%src_include_dir', config.clang_src_dir + '/include'))
+
+
 # Propagate path to symbolizer for ASan/MSan.
 llvm_config.with_system_environment(
 ['ASAN_SYMBOLIZER_PATH', 'MSAN_SYMBOLIZER_PATH'])


Index: test/lit.cfg.py
===
--- test/lit.cfg.py
+++ test/lit.cfg.py
@@ -43,6 +43,10 @@
 
 llvm_config.use_clang()
 
+config.substitutions.append(
+('%src_include_dir', config.clang_src_dir + '/include'))
+
+
 # Propagate path to symbolizer for ASan/MSan.
 llvm_config.with_system_environment(
 ['ASAN_SYMBOLIZER_PATH', 'MSAN_SYMBOLIZER_PATH'])
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77
+  internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {
+auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<
+typename 
ast_matchers::internal::VariadicAllOfMatcher::Type>();

steveire wrote:
> aaron.ballman wrote:
> > zturner wrote:
> > > steveire wrote:
> > > > aaron.ballman wrote:
> > > > > steveire wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Mildly not keen on the use of `auto` here. It's a factory 
> > > > > > > function, so it kind of names the resulting type, but it also 
> > > > > > > looks like the type will be 
> > > > > > > `ast_matchers::internal::VariadicAllOfMatcher::Type` 
> > > > > > > from the template argument, which is incorrect.
> > > > > > There is no reason to assume that taking a template argument means 
> > > > > > that type is result.
> > > > > > 
> > > > > > The method is `getFrom` which decreases the ambiguity still further.
> > > > > > 
> > > > > > Spelling out the type doesn't add anything useful. This should be 
> > > > > > ok.
> > > > > > There is no reason to assume that taking a template argument means 
> > > > > > that type is result.
> > > > > 
> > > > > Aside from all the other places that do exactly that (getAs<>, 
> > > > > cast<>, dyn_cast<>, castAs<>, and so on). Generally, when we have a 
> > > > > function named get that takes a template type argument, the result 
> > > > > when seen in proximity to auto is the template type.
> > > > > 
> > > > > > Spelling out the type doesn't add anything useful. This should be 
> > > > > > ok.
> > > > > 
> > > > > I slept on this one and fall on the other side of it; using auto 
> > > > > hides information that tripped up at least one person when reading 
> > > > > the code, so don't use auto. It's not clear enough what the resulting 
> > > > > type will be.
> > > > I put this in the category of requiring 
> > > > 
> > > > SomeType ST = SomeType::create();
> > > > 
> > > > instead of 
> > > > 
> > > > auto ST = SomeType::create();
> > > > 
> > > > `ast_type_traits::ASTNodeKind` is already on that line and you want to 
> > > > see it again.
> > > > 
> > > FWIW I'm with Aaron here.  Im' not familiar at all with this codebase, 
> > > and looking at the code, my first instinct is "the result type is 
> > > probably `ast_matchers::internal::VariadicAllOfMatcher::Type`".  
> > > According to Aaron's earlier comment, that is incorrect, so there's at 
> > > least 1 data point that it hinders readability.
> > > 
> > > So, honest question.  What *is* the return type here?
> > > So, honest question. What *is* the return type here?
> > 
> > Much to my surprise, it's `ASTNodeKind`. It was entirely unobvious to me 
> > that this was a factory function.
> @zturner Quoting myself:
> 
> > `ast_type_traits::ASTNodeKind` is already on that line and you want to see 
> > it again.
> 
> The expression is `getFromNodeKind`. There is a pattern of 
> `SomeType::fromFooKind()` returning a `SomeType`. This is not so 
> unusual.
> 
> 
Note that at the top of this file there's already a `using namespace 
clang::ast_type_traits;`  So if you're worried about verbosity, then you can 
remove the `ast_type_traits::`, remove the `auto`, and the net effect is that 
the overall line will end up being shorter.


Repository:
  rC Clang

https://reviews.llvm.org/D54405



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

[PATCH] D54405: Record whether a AST Matcher constructs a Node

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77
+  internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) {
+auto K = ast_type_traits::ASTNodeKind::getFromNodeKind<
+typename 
ast_matchers::internal::VariadicAllOfMatcher::Type>();

steveire wrote:
> aaron.ballman wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > Mildly not keen on the use of `auto` here. It's a factory function, so 
> > > > it kind of names the resulting type, but it also looks like the type 
> > > > will be `ast_matchers::internal::VariadicAllOfMatcher::Type` 
> > > > from the template argument, which is incorrect.
> > > There is no reason to assume that taking a template argument means that 
> > > type is result.
> > > 
> > > The method is `getFrom` which decreases the ambiguity still further.
> > > 
> > > Spelling out the type doesn't add anything useful. This should be ok.
> > > There is no reason to assume that taking a template argument means that 
> > > type is result.
> > 
> > Aside from all the other places that do exactly that (getAs<>, cast<>, 
> > dyn_cast<>, castAs<>, and so on). Generally, when we have a function named 
> > get that takes a template type argument, the result when seen in proximity 
> > to auto is the template type.
> > 
> > > Spelling out the type doesn't add anything useful. This should be ok.
> > 
> > I slept on this one and fall on the other side of it; using auto hides 
> > information that tripped up at least one person when reading the code, so 
> > don't use auto. It's not clear enough what the resulting type will be.
> I put this in the category of requiring 
> 
> SomeType ST = SomeType::create();
> 
> instead of 
> 
> auto ST = SomeType::create();
> 
> `ast_type_traits::ASTNodeKind` is already on that line and you want to see it 
> again.
> 
FWIW I'm with Aaron here.  Im' not familiar at all with this codebase, and 
looking at the code, my first instinct is "the result type is probably 
`ast_matchers::internal::VariadicAllOfMatcher::Type`".  According to 
Aaron's earlier comment, that is incorrect, so there's at least 1 data point 
that it hinders readability.

So, honest question.  What *is* the return type here?


Repository:
  rC Clang

https://reviews.llvm.org/D54405



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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D54187#1290432, @rnk wrote:

> I hadn't realized that Dexter knew how to drive VS tools. I'll have to go 
> read more and get back to you all.
>
> I think it would be more promising than attempting to come up with a new 
> llgdb.py-like abstraction for cdb. Specifically, abstracting over setting 
> breakpoints and reformatting output is what makes that difficult. Everything 
> can of course be done with enough effort, but especially in testing, it often 
> makes sense to trade off duplication between test cases for ease of use.


Note that WinDbg (specifically) is an important use case, and uses a different 
debug engine than VS.  So Dexter would (at the very least) need to be extended 
to support WinDbg (which has the same debugging engine as cdb).  But I agree 
it's worth trying out and seeing what kind of test cases we can and can't fit 
into it.


https://reviews.llvm.org/D54187



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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D54187#1290297, @aprantl wrote:

> In https://reviews.llvm.org/D54187#1290293, @zturner wrote:
>
> > Especially since as far as I can tell, nobody has even run debuginfo-tests 
> > since late August, because it was actually broken by r341135 on August 30 
> > (fixed in r346060 yesterday)
>
>
> Can you please refrain from making such general statements? They distract 
> from the discussion.
>
> http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/ runs the 
> debuginfo-tests continuously. There is a configuration issue that prevents it 
> from running the ASAN subset of the tests at the moment.


The issue introduced by r341135 doesn't seem related to ASAN unless I'm 
misunderstanding the issue.  Were debuginfo-tests passing on that bot even 
before r346060 landed?


https://reviews.llvm.org/D54187



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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D54187#1290282, @probinson wrote:

> +gbedwell
>
> Just to throw the idea out there, why not abandon debuginfo-tests entirely 
> and try using Dexter for this. Dexter's design center is debug-info quality, 
> not correctness, but it already knows how to drive several different 
> debuggers on multiple platforms.
>  Dexter would have to become an LLVM project tool, and I'm not sure how Sony 
> management feels about that, but I think this would be an awesome use-case.


Depends where we draw the distinction between quality and correctness.  We 
specifically want a way to test that when we fix a correctness bug, it's 
actually fixed against Microsoft debuggers.


https://reviews.llvm.org/D54187



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

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D54187#1290247, @aprantl wrote:

> > I think the only way to realistically make this work for all platforms 
> > would be to separate the source file from the input/output. The source file 
> > would be the test case, and if you wanted to support a different debugger 
> > you would need to supply a different input/output file..
>
> I don't necessarily agree with that statement. The debuginfo-tests use only a 
> very small subset of debugger functionality that includes
>
> - setting breakpoints
> - printing the value of integer variables
> - continuing to the next breakpoint
>
>   I'm genuinely curious what is so different about the Windows debugger that 
> it couldn't be wrapped in a translation layer like `llgdb.py` that abstracts 
> these three operations. This would cover a large set of the existing tests. 
> I'm fine with having a few extra tests that test something that only works on 
> a specific platform here and there, but I really don't want us to grow 
> separate platform-specific testsuites. Inevitably, someone working on 
> platform A will fix a general bug in LLVM and then only add a test for 
> platform A and that's bad for everyone. What I'm trying to avoid is a 
> situation like the debug info tests in LLVM that are almost entirely 
> x86-specific, even if they are testing stuff that happens at the IR level.


I don't think we want tests that are limited to that amount of simplicity.  We 
don't just want to print integers, we'd like to also print callstacks.  And 
objects.  And other things that aren't integers.  A cdb call stack looks like 
this:

00c4`aa35f730 7ffc`8944bc72 : 0294`3b6e0ae8 0294`3b6e0a98 
` ` : MSVCP140D!_Cnd_wait+0x20 
[f:\dd\vctools\crt\crtw32\stdcpp\thr\cond.c @ 106] 
08 00c4`aa35f760 7ffc`89450a54 : 0294`3b6e0ae8 0294`3b6e0a98 
` ` : liblldb!std::_Cnd_waitX+0x32 [c:\program 
files (x86)\microsoft visual 
studio\2017\professional\vc\tools\msvc\14.15.26726\include\thr\xthread @ 97] 
09 00c4`aa35f790 7ffc`8944122d : 0294`3b6e0ae8 00c4`aa35f828 
` ` : 
liblldb!std::condition_variable::wait+0x54 [c:\program files (x86)\microsoft 
visual studio\2017\professional\vc\tools\msvc\14.15.26726\include\mutex @ 714] 
0a 00c4`aa35f7d0 7ffc`89440897 : 0294`3b6e09e0 00c4`aa35fb78 
` ` : 
liblldb!lldb_private::Listener::GetEventInternal+0x20d 
[d:\src\llvm-mono\lldb\source\core\listener.cpp @ 367] 
0b 00c4`aa35f8e0 7ffc`8939b70e : 0294`3b6e09e0 00c4`aa35fa78 
00c4`aa35fb78 ` : 
liblldb!lldb_private::Listener::GetEvent+0x57 
[d:\src\llvm-mono\lldb\source\core\listener.cpp @ 404] 
0c 00c4`aa35f930 7ffc`8939b118 : 0294`3b6de700 ` 
` ` : 
liblldb!lldb_private::Debugger::DefaultEventHandler+0x27e 
[d:\src\llvm-mono\lldb\source\core\debugger.cpp @ 1546] 
0d 00c4`aa35fc30 7ffc`8960cf62 : 0294`3b6de700 0294`0001 
` ` : 
liblldb!lldb_private::Debugger::EventHandlerThread+0x28 
[d:\src\llvm-mono\lldb\source\core\debugger.cpp @ 1600]

Then you need a way to abstract over the command lines needed to generate the 
executables (`clang-cl` and `lld-link` use different flag syntax than `clang++` 
etc).  Then there's the issue of when a test is using Microsoft specific 
extensions.  At the end of all of this, it's going to take a lot of effort to 
implement this layer of abstraction that is ultimately going to be subverted 
for a large number of tests anyway when something doesn't fit cleanly into the 
abstraction.  I think there is also the issue of how much actual overlap there 
is between the set of interesting test cases for DWARF / Itanium ABI and PDB / 
Microsoft ABI.  Many issues that we would want to add tests for would be 
surrounding fixes specific to the way we emit the PDB or that are constructed 
due to knowledge of how we emit CodeView in certain situations.  And none of 
those test cases will be interesting to abstract over.

Finally, by strictly limiting the amount of output we're checking against, we 
can be too permissive.  For example, we have a command up there that checks 
against the line `g`.  But this will match any line that has the letter `g` in 
it, which is extremely permissive.  It would be more useful if the line said 
`DEBUGGER: 0:000> g`

The line that says `CHECK: a = 0n2` will also match if the variable happens to 
be 23 or any number of other values.  So we'd really like to be able to be more 
precise here.

So I'm not convinced there is a lot of added value, or at the very least, I 
don't believe it to be worth the cost.  Especially since as far as I can tell, 
nobody has even run debuginfo-tests since late August, because it was actually 
broken by r341135 on August 30 (fixed in r

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

I think the only way to realistically make this work for all platforms would be 
to separate the source file from the input/output.  The source file would be 
the test case, and if you wanted to support a different debugger you would need 
to supply a different input/output file..

That said, as Reid mentioned, we've been talking about getting this going for 
years, so at this point, and there's a steep initial overhead in making 
something that abstracts over multiple debuggers.  So at least until we have 
something that works on Windows with some interesting tests, I think we 
shouldn't try to create the abstraction just yet.


https://reviews.llvm.org/D54187



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

[PATCH] D53718: Change keep-static-consts to work on static storage duration, not storage class.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Will this work for the following variable?

  constexpr int N = 3;


Repository:
  rC Clang

https://reviews.llvm.org/D53718



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Zachary Turner via Phabricator via cfe-commits]

zturner added a subscriber: mstorsjo.
zturner added a comment.

It seems like some combination of checking the target triple, host triple,
and driver mode and putting the conversions behind those checks could work?

For paths like resource dir that are going into debug info it should be
driver mode. For paths we pass to another tool it should probably be based
on target triple . This probably isn’t perfect, but WDYT?


https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/Driver/Driver.cpp:1013-1014
   }
+  for (auto *Str : {&Dir, &InstalledDir, &SysRoot, &DyldPrefix, &ResourceDir})
+*Str = llvm::sys::path::convert_to_slash(*Str);
 

Is this going to affect the cl driver as well?



Comment at: lib/Driver/ToolChain.cpp:381
 if (getVFS().exists(P))
-  return P.str();
+  return llvm::sys::path::convert_to_slash(P);
   }

Same questions here and for the rest of the changes in this file


https://reviews.llvm.org/D53066



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

[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:24
+truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
+  double value = FloatLiteral.getValueAsApproximateDouble();
+  if (std::fmod(value, 1) == 0) {

All variables (local, global, function parameter) use exactly same naming 
convention `CamelCase`.


https://reviews.llvm.org/D53339



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

I can try to get some timings from my machine.  How do we handle crash
recovery in the case where we don't spawn a child process?  I thought the
whole reason for spawning the cc1 driver as a separate process was so that
we could collect and report crash information in a nice way.  Not having
that seems like a heavy price to pay.


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

It's not enough to just set `_HAS_EXCEPTIONS=1`, it has to match whatever the 
value of `/EH` is passed to the compiler.  So if we need to be able to throw 
catch exceptions, then you should pass `/EHsc` and not touch `_HAS_EXCEPTIONS` 
(technically you could set it to 1 but that's the default).  And if we don't 
need to be able to throw or catch exceptions, then we pass `/EHs-c-` (which is 
what we do today) and also set `_HAS_EXCEPTIONS=0`.  But the two should agree 
with each other.


https://reviews.llvm.org/D52998



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

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Zachary Turner via Phabricator via cfe-commits]

zturner added subscribers: eandrews, zturner.
zturner added a comment.

`_HAS_EXCEPTIONS=0` is an undocumented STL specific thing that the library
implementation uses to mean "don't write code that does `throw X;`, do
something else instead".


https://reviews.llvm.org/D52998



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

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D52773#1255491, @thakis wrote:

> In https://reviews.llvm.org/D52773#1255093, @zturner wrote:
>
> > I agree magic environment variables are bad, but without it we don’t
> >  address the only current actual use we have for this, which is making the
> >  vs integration print filenames. Detecting compiler version from inside the
> >  integration is hard
>
>
> We need some solution to this anyhow; e.g. say "this now requires clang 8.0", 
> or have a clang version dropdown in the UI (which defaults to the latest 
> release), or something. We can't add an env var for every future flag that 
> the vs integration might want to use.


But it is **very hard** to automatically detect the version from the 
integration.  I tried this and gave up because it was flaky.  So sure, we can 
present a drop-down in the UI, but it could be mismatched, and that would end 
up creating more problems than it solves.

Right now we have exactly 1 use case for showing filenames from clang-cl, and 
there's no good way to satisfy that use case with a command line option.

I agree that we can't add an environment variable for every future flag that 
the VS integration might want to use, but until that happens, YAGNI.  And if 
and when we do need it, if we manage to come up with a solution to the problem, 
we can delete support for the environment variable and make it use the new 
method.


https://reviews.llvm.org/D52773



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

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via Phabricator via cfe-commits]

zturner added a subscriber: rnk.
zturner added a comment.

I agree magic environment variables are bad, but without it we don’t
address the only current actual use we have for this, which is making the
vs integration print filenames. Detecting compiler version from inside the
integration is hard, but with an environment variable it’s very easy to
solve.

I’m not against a flag as the supported way, but I think we should also
have some backdoor into this functionality, because if we’re not going to
satisfy the only known use case, then maybe it’s better to not even do it
at all.


https://reviews.llvm.org/D52773



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

[PATCH] D52843: Update Clang Windows getting started docs

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang/www/get_started.html:246
+"C:\Program Files (x86)\Microsoft Visual
+  Studio\2017\Professional\VC\Auxiliary\Build\vcvarsall.bat" x64
+  

zturner wrote:
> STL_MSFT wrote:
> > This assumes the Professional SKU; perhaps the instructions should be for 
> > Community?
> Would something like `%VS2017INSTALLDIR%` be even better?
Granted it assumes 2017, but not as bad as assuming both Professional SKU + 
default installation path.


https://reviews.llvm.org/D52843



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

[PATCH] D52843: Update Clang Windows getting started docs

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang/www/get_started.html:246
+"C:\Program Files (x86)\Microsoft Visual
+  Studio\2017\Professional\VC\Auxiliary\Build\vcvarsall.bat" x64
+  

STL_MSFT wrote:
> This assumes the Professional SKU; perhaps the instructions should be for 
> Community?
Would something like `%VS2017INSTALLDIR%` be even better?


https://reviews.llvm.org/D52843



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

[PATCH] D52798: [cl-compat] Change /JMC from unsupported to ignored.

[Zachary Turner via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL343629: [cl-compat] Change /JMC from unsupported to ignored. 
(authored by zturner, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52798?vs=168006&id=168016#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52798

Files:
  cfe/trunk/include/clang/Driver/CLCompatOptions.td


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -354,6 +354,7 @@
 def _SLASH_FC : CLIgnoredFlag<"FC">;
 def _SLASH_Fd : CLIgnoredJoined<"Fd">;
 def _SLASH_FS : CLIgnoredFlag<"FS">;
+def _SLASH_JMC : CLIgnoredFlag<"JMC">;
 def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
 def _SLASH_nologo : CLIgnoredFlag<"nologo">;
 def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">;
@@ -405,7 +406,6 @@
 def _SLASH_H : CLFlag<"H">;
 def _SLASH_homeparams : CLFlag<"homeparams">;
 def _SLASH_hotpatch : CLFlag<"hotpatch">;
-def _SLASH_JMC : CLFlag<"JMC">;
 def _SLASH_kernel : CLFlag<"kernel">;
 def _SLASH_LN : CLFlag<"LN">;
 def _SLASH_MP : CLJoined<"MP">;


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -354,6 +354,7 @@
 def _SLASH_FC : CLIgnoredFlag<"FC">;
 def _SLASH_Fd : CLIgnoredJoined<"Fd">;
 def _SLASH_FS : CLIgnoredFlag<"FS">;
+def _SLASH_JMC : CLIgnoredFlag<"JMC">;
 def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
 def _SLASH_nologo : CLIgnoredFlag<"nologo">;
 def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">;
@@ -405,7 +406,6 @@
 def _SLASH_H : CLFlag<"H">;
 def _SLASH_homeparams : CLFlag<"homeparams">;
 def _SLASH_hotpatch : CLFlag<"hotpatch">;
-def _SLASH_JMC : CLFlag<"JMC">;
 def _SLASH_kernel : CLFlag<"kernel">;
 def _SLASH_LN : CLFlag<"LN">;
 def _SLASH_MP : CLJoined<"MP">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52798: [cl-compat] Change /JMC from unsupported to ignored.

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
zturner added reviewers: rnk, thakis.
Herald added subscribers: JDevlieghere, aprantl.

This command line option doesn't really affect generated code, only generated 
debug info, and it's set by default in newer versions of VS, so it's annoying 
to have to turn it off in order to be able to use the VS Integration extension.


https://reviews.llvm.org/D52798

Files:
  clang/include/clang/Driver/CLCompatOptions.td


Index: clang/include/clang/Driver/CLCompatOptions.td
===
--- clang/include/clang/Driver/CLCompatOptions.td
+++ clang/include/clang/Driver/CLCompatOptions.td
@@ -354,6 +354,7 @@
 def _SLASH_FC : CLIgnoredFlag<"FC">;
 def _SLASH_Fd : CLIgnoredJoined<"Fd">;
 def _SLASH_FS : CLIgnoredFlag<"FS">;
+def _SLASH_JMC : CLIgnoredFlag<"JMC">;
 def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
 def _SLASH_nologo : CLIgnoredFlag<"nologo">;
 def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">;
@@ -405,7 +406,6 @@
 def _SLASH_H : CLFlag<"H">;
 def _SLASH_homeparams : CLFlag<"homeparams">;
 def _SLASH_hotpatch : CLFlag<"hotpatch">;
-def _SLASH_JMC : CLFlag<"JMC">;
 def _SLASH_kernel : CLFlag<"kernel">;
 def _SLASH_LN : CLFlag<"LN">;
 def _SLASH_MP : CLJoined<"MP">;


Index: clang/include/clang/Driver/CLCompatOptions.td
===
--- clang/include/clang/Driver/CLCompatOptions.td
+++ clang/include/clang/Driver/CLCompatOptions.td
@@ -354,6 +354,7 @@
 def _SLASH_FC : CLIgnoredFlag<"FC">;
 def _SLASH_Fd : CLIgnoredJoined<"Fd">;
 def _SLASH_FS : CLIgnoredFlag<"FS">;
+def _SLASH_JMC : CLIgnoredFlag<"JMC">;
 def _SLASH_kernel_ : CLIgnoredFlag<"kernel-">;
 def _SLASH_nologo : CLIgnoredFlag<"nologo">;
 def _SLASH_openmp_ : CLIgnoredFlag<"openmp-">;
@@ -405,7 +406,6 @@
 def _SLASH_H : CLFlag<"H">;
 def _SLASH_homeparams : CLFlag<"homeparams">;
 def _SLASH_hotpatch : CLFlag<"hotpatch">;
-def _SLASH_JMC : CLFlag<"JMC">;
 def _SLASH_kernel : CLFlag<"kernel">;
 def _SLASH_LN : CLFlag<"LN">;
 def _SLASH_MP : CLJoined<"MP">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/Frontend/CompilerInvocation.cpp:792
   Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name);
+  if (Args.hasArg(OPT_echo_main_file_name))
+llvm::outs() << Opts.MainFileName << "\n";

steveire wrote:
> zturner wrote:
> > zturner wrote:
> > > steveire wrote:
> > > > hans wrote:
> > > > > zturner wrote:
> > > > > > steveire wrote:
> > > > > > > I'll have to keep a patch around anyway for myself locally to 
> > > > > > > remove this condition. But maybe someone else will benefit from 
> > > > > > > this.
> > > > > > > 
> > > > > > > What do you think about changing CMake to so that this is passed 
> > > > > > > by default when using Clang-CL?
> > > > > > > 
> > > > > > Do you mean llvms cmake?  Or cmake itself?  Which generator are you 
> > > > > > using?
> > > > > Can't you just configure your build to pass the flag to clang-cl?
> > > > > 
> > > > > I suppose CMake could be made to do that by default when using 
> > > > > clang-cl versions that support the flag and using the MSBuild 
> > > > > generator, but that's for the CMake community to decide I think. Or 
> > > > > perhaps our VS integration could do that, but it gets tricky because 
> > > > > it needs to know whether the flag is supported or not.
> > > > I mean upstream cmake. My suggestion is independent of the generator, 
> > > > but it would depend on what Brad decides anyway. I don't intend to 
> > > > implement it, just report it.
> > > > 
> > > > I only have one clang but I have multiple buildsystems, and I don't 
> > > > want to have to add a new flag too all of them. In each toplevel 
> > > > CMakeLists everyone will need this to make use of the flag:
> > > > 
> > > > ```
> > > > 
> > > > # Have to remember to use "${CMAKE_CXX_SIMULATE_ID}" instead of 
> > > > # Undecorated "CMAKE_CXX_SIMULATE_ID" because there is a 
> > > > # variable named "MSVC" and
> > > > # the way CMake variables and the "if()" command work requires this. 
> > > > if (CMAKE_CXX_COMPILER_ID STREQUAL Clang 
> > > > AND ${CMAKE_CXX_SIMULATE_ID} STREQUAL MSVC)
> > > > # CMake does not have explicit Clang-CL support yet.
> > > > # It should have a COMPILER_ID for it.
> > > > set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /showFilename")
> > > > # etc
> > > > endif()
> > > > ```
> > > > 
> > > > Upstream CMake can have the logic to add the flag instead.
> > > But then what if you don’t want it?  There would be no way to turn it 
> > > off.  I don’t think it’s a good fit for cmake 
> > Yes, and actually for this reason i was wondering if we can do it without a 
> > flag at all.  What if we just set an magic environment variable?  It 
> > handles Stephen’s use case (he can just set it globally), and MSBuild (we 
> > can set it in the extension).
> > But then what if you don’t want it? There would be no way to turn it off. 
> 
> Oh, I thought that the last flag would dominate. ie, if cmake generated 
> `/showFilename` and the user adds `/showFilename-`, then you would end up 
> with  
> 
> `clang-cl.exe /showFilename /showFilename-` 
> 
> and it would not be shown. I guess that's not how it works.
> 
> Maybe users want to not show it, but not seeing the filename is a surprising 
> first-time experience when porting from MSVC to clang-cl using Visual Studio.
> 
> However, I'll just drop out of the discussion and not make any bug to CMake. 
> If anyone else feels strongly, they can do so.
> 
> Thanks!
Right, but if we update the VS Integration Extension so that MSBuild specifies 
the flag (or sets the environment variable, etc), then any time you use MSBuild 
(even if not through the IDE) you would get the output, so to the user it would 
look no different.


https://reviews.llvm.org/D52773



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

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/Frontend/CompilerInvocation.cpp:792
   Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name);
+  if (Args.hasArg(OPT_echo_main_file_name))
+llvm::outs() << Opts.MainFileName << "\n";

steveire wrote:
> hans wrote:
> > zturner wrote:
> > > steveire wrote:
> > > > I'll have to keep a patch around anyway for myself locally to remove 
> > > > this condition. But maybe someone else will benefit from this.
> > > > 
> > > > What do you think about changing CMake to so that this is passed by 
> > > > default when using Clang-CL?
> > > > 
> > > Do you mean llvms cmake?  Or cmake itself?  Which generator are you using?
> > Can't you just configure your build to pass the flag to clang-cl?
> > 
> > I suppose CMake could be made to do that by default when using clang-cl 
> > versions that support the flag and using the MSBuild generator, but that's 
> > for the CMake community to decide I think. Or perhaps our VS integration 
> > could do that, but it gets tricky because it needs to know whether the flag 
> > is supported or not.
> I mean upstream cmake. My suggestion is independent of the generator, but it 
> would depend on what Brad decides anyway. I don't intend to implement it, 
> just report it.
> 
> I only have one clang but I have multiple buildsystems, and I don't want to 
> have to add a new flag too all of them. In each toplevel CMakeLists everyone 
> will need this to make use of the flag:
> 
> ```
> 
> # Have to remember to use "${CMAKE_CXX_SIMULATE_ID}" instead of 
> # Undecorated "CMAKE_CXX_SIMULATE_ID" because there is a 
> # variable named "MSVC" and
> # the way CMake variables and the "if()" command work requires this. 
> if (CMAKE_CXX_COMPILER_ID STREQUAL Clang 
> AND ${CMAKE_CXX_SIMULATE_ID} STREQUAL MSVC)
> # CMake does not have explicit Clang-CL support yet.
> # It should have a COMPILER_ID for it.
> set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /showFilename")
> # etc
> endif()
> ```
> 
> Upstream CMake can have the logic to add the flag instead.
But then what if you don’t want it?  There would be no way to turn it off.  I 
don’t think it’s a good fit for cmake 



Comment at: lib/Frontend/CompilerInvocation.cpp:792
   Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name);
+  if (Args.hasArg(OPT_echo_main_file_name))
+llvm::outs() << Opts.MainFileName << "\n";

zturner wrote:
> steveire wrote:
> > hans wrote:
> > > zturner wrote:
> > > > steveire wrote:
> > > > > I'll have to keep a patch around anyway for myself locally to remove 
> > > > > this condition. But maybe someone else will benefit from this.
> > > > > 
> > > > > What do you think about changing CMake to so that this is passed by 
> > > > > default when using Clang-CL?
> > > > > 
> > > > Do you mean llvms cmake?  Or cmake itself?  Which generator are you 
> > > > using?
> > > Can't you just configure your build to pass the flag to clang-cl?
> > > 
> > > I suppose CMake could be made to do that by default when using clang-cl 
> > > versions that support the flag and using the MSBuild generator, but 
> > > that's for the CMake community to decide I think. Or perhaps our VS 
> > > integration could do that, but it gets tricky because it needs to know 
> > > whether the flag is supported or not.
> > I mean upstream cmake. My suggestion is independent of the generator, but 
> > it would depend on what Brad decides anyway. I don't intend to implement 
> > it, just report it.
> > 
> > I only have one clang but I have multiple buildsystems, and I don't want to 
> > have to add a new flag too all of them. In each toplevel CMakeLists 
> > everyone will need this to make use of the flag:
> > 
> > ```
> > 
> > # Have to remember to use "${CMAKE_CXX_SIMULATE_ID}" instead of 
> > # Undecorated "CMAKE_CXX_SIMULATE_ID" because there is a 
> > # variable named "MSVC" and
> > # the way CMake variables and the "if()" command work requires this. 
> > if (CMAKE_CXX_COMPILER_ID STREQUAL Clang 
> > AND ${CMAKE_CXX_SIMULATE_ID} STREQUAL MSVC)
> > # CMake does not have explicit Clang-CL support yet.
> > # It should have a COMPILER_ID for it.
> > set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /showFilename")
> > # etc
> > endif()
> > ```
> > 
> > Upstream CMake can have the logic to add the flag instead.
> But then what if you don’t want it?  There would be no way to turn it off.  I 
> don’t think it’s a good fit for cmake 
Yes, and actually for this reason i was wondering if we can do it without a 
flag at all.  What if we just set an magic environment variable?  It handles 
Stephen’s use case (he can just set it globally), and MSBuild (we can set it in 
the extension).


https://reviews.llvm.org/D52773



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

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Canyou add a test that demonstrates that multiple input files on the command 
line will print all of them?  Also does this really need to be a cc1 arg?  Why 
not just print it in the driver?


https://reviews.llvm.org/D52773



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

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/Frontend/CompilerInvocation.cpp:792
   Opts.MainFileName = Args.getLastArgValue(OPT_main_file_name);
+  if (Args.hasArg(OPT_echo_main_file_name))
+llvm::outs() << Opts.MainFileName << "\n";

steveire wrote:
> I'll have to keep a patch around anyway for myself locally to remove this 
> condition. But maybe someone else will benefit from this.
> 
> What do you think about changing CMake to so that this is passed by default 
> when using Clang-CL?
> 
Do you mean llvms cmake?  Or cmake itself?  Which generator are you using?


https://reviews.llvm.org/D52773



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

The process stuff looks cool and useful independently of `/MP`.  Would it be 
possible to break that into a separate patch, and add a unit test for the 
behavior of the `WaitAll` flag?


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D52193#1238978, @aganea wrote:

> In https://reviews.llvm.org/D52193#1238944, @zturner wrote:
>
> > In https://reviews.llvm.org/D52193#1238923, @aganea wrote:
> >
> > > The goal of this change is frictionless compilation into VS2017 when 
> > > using `clang-cl` as a compiler. We've realized that compiling Clang+LLD 
> > > with Clang generates a faster executable that with MSVC (even latest one).
> > >  I currently can't see a good way of generating the Visual Studio 
> > > solution with CMake, while using Ninja+clang-cl for compilation. They are 
> > > two orthogonal configurations. Any suggestions?
> >
> >
> > I don't think this is necessary.  I think your updated Matrix is pretty 
> > good.
> >
> > I'm surprised to see that Ninja + Clang is slower than Ninja + MSVC.  Are 
> > you using lld here?
>
>
> Yes, all the ‘Clang’ rows use both clang-cl and lld-link.
>  Like stated above, I think this is caused by a lot more processes 
> (clang-cl.exe) being invoked. In contrast, cl.exe does not invoke a child 
> process. There are about 3200 files to compiles, which makes 6400 calls to 
> clang-cl. At about ~60ms lead time per process, that adds up to an extra 
> ~3min wall clock time. It is the simplest explanation I could find.


Would you mind syncing to r342002 and trying again?  I don't doubt your 
results, but I'm interested to see how much of an improvement this patch 
provides.

  commit 840717872a0e0f03b19040ef143bf45aa1a7f0a0
  Author: Reid Kleckner 
  Date:   Tue Sep 11 22:25:13 2018 +
  
  [cmake] Speed up check-llvm 5x by delay loading shell32 and ole32
  
  Summary:
  Previously, check-llvm on my Windows 10 workstation took about 300s to
  run, and it would lock up my mouse. Now, it takes just under 60s.
  Previously running the tests only used about 15% of the available CPU
  time, now it uses up to 60%.
  
  Shell32.dll and ole32.dll have direct dependencies on user32.dll and
  gdi32.dll. These DLLs are mostly used to for Windows GUI functionality,
  which most LLVM tools don't need. It seems that these DLLs acquire and
  release some system resources on startup and exit, and that requires
  acquiring the same highly contended lock discussed in this post:
  
https://randomascii.wordpress.com/2017/07/09/24-core-cpu-and-i-cant-move-my-mouse/
  
  Most LLVM tools still have a transitive dependency on
  SHGetKnownFolderPathW, which is used to look up the user home directory
  and local AppData directory. We also use SHFileOperationW to recursively
  delete a directory, but only LLDB and clang-doc use this today. At some
  point, we might consider removing these last shell32.dll deps, but for
  now, I would like to take this free performance.
  
  Many thanks to Bruce Dawson for suggesting this fix. He looked at an ETW
  trace of an LLVM test run, noticed these DLLs, and suggested avoiding
  them.
  
  Reviewers: zturner, pcc, thakis
  
  Subscribers: mgorny, llvm-commits, hiraditya
  
  Differential Revision: https://reviews.llvm.org/D51952
  
  Notes:
  git-svn-rev: 342002


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Zachary Turner via Phabricator via cfe-commits]

zturner added a reviewer: thakis.
zturner added a comment.

That said, the numbers are pretty convincing

These two rows alone:
MSBuild, Clang + LLD(29min 12sec)   32 parallel msbuild
MSBuild, Clang /MP + LLD(9min 22sec)32 parallel msbuild

Are enough to make this patch worthy of serious consideration.  (I haven't 
looked at the content / complexity yet, was waiting on the numbers first).


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D52193#1238923, @aganea wrote:

> The goal of this change is frictionless compilation into VS2017 when using 
> `clang-cl` as a compiler. We've realized that compiling Clang+LLD with Clang 
> generates a faster executable that with MSVC (even latest one).
>  I currently can't see a good way of generating the Visual Studio solution 
> with CMake, while using Ninja+clang-cl for compilation. They are two 
> orthogonal configurations. Any suggestions?


I don't think this is necessary.  I think your updated Matrix is pretty good.

I'm surprised to see that Ninja + Clang is slower than Ninja + MSVC.  Are you 
using lld here?


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Zachary Turner via Phabricator via cfe-commits]

zturner added a subscriber: aganea.
zturner added a comment.

In an ideal world yes, but the reality is that many people still use
MSBuild, and in that world /MP presumably helps quite a bit. And given that
many people already depend on this functionality of cl, it’s a potential
showstopper for migrating if we don’t support it. That said, if the benefit
isn’t significant that’s a stronger argument for not supporting it imo


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

[Zachary Turner via Phabricator via cfe-commits]

zturner added a reviewer: rsmith.
zturner added a comment.

What about the timings of clang-cl without /MP?


Repository:
  rC Clang

https://reviews.llvm.org/D52193



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

[PATCH] D51847: Print correctly dependency paths on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

clang-cl in general tries to do the thing that makes native Windows developers 
the most comfortable, while gcc in general tries to do the thing that makes 
Unix developers most comfortable.  So I think we should probably use \ here.  
If anyone complains we can revisit.


https://reviews.llvm.org/D51847



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

[PATCH] D51847: Print correctly dependency paths on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Alright I see it.  The reason I'm asking is because after your change you're 
now printing `main.o: main.c ../include/lib/test.h`.  But I wonder if you 
should instead be printing `main.o: main.c ..\include\lib\test.h`.  It seems 
like paths that we show to the user should be in the native path style.




Comment at: lib/Frontend/DependencyFile.cpp:389
   DependencyOutputFormat OutputFormat) {
+  std::string &NativePath = llvm::sys::path::convert_to_slash(Filename);
   if (OutputFormat == DependencyOutputFormat::NMake) {

This looks like a dangling reference -- undefined behavior.


https://reviews.llvm.org/D51847



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

[PATCH] D51847: Print correctly dependency paths on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

I mean in practice. What command do i run to hit this and what will the
output look like? Can you paste some terminal output showing the command
and output?


https://reviews.llvm.org/D51847



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

[PATCH] D51847: Print correctly dependency paths on Windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a subscriber: xbolva00.
zturner added a comment.

What prints this? How do you exercise this codepath?


https://reviews.llvm.org/D51847



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

[PATCH] D51500: [MS ABI] Fix mangling incompatibility with dynamic initializer stubs.

[Zachary Turner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC341117: [MS ABI] Fix mangling issue with dynamic initializer 
stubs. (authored by zturner, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51500?vs=163386&id=163404#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51500

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenCXX/microsoft-abi-static-initializers.cpp
  test/CodeGenCXX/pragma-init_seg.cpp


Index: test/CodeGenCXX/pragma-init_seg.cpp
===
--- test/CodeGenCXX/pragma-init_seg.cpp
+++ test/CodeGenCXX/pragma-init_seg.cpp
@@ -44,15 +44,15 @@
 template  const int A::x = f();
 template struct A;
 // CHECK: @"?x@?$A@H@explicit_template_instantiation@@2HB" = weak_odr 
dso_local global i32 0, comdat, align 4
-// CHECK: @__cxx_init_fn_ptr.4 = private constant void ()* 
@"??__Ex@?$A@H@explicit_template_instantiation@@2HB@YAXXZ", section ".asdf", 
comdat($"?x@?$A@H@explicit_template_instantiation@@2HB")
+// CHECK: @__cxx_init_fn_ptr.4 = private constant void ()* 
@"??__E?x@?$A@H@explicit_template_instantiation@@2HB@@YAXXZ", section ".asdf", 
comdat($"?x@?$A@H@explicit_template_instantiation@@2HB")
 }
 
 namespace implicit_template_instantiation {
 template  struct A { static const int x; };
 template  const int A::x = f();
 int g() { return A::x; }
 // CHECK: @"?x@?$A@H@implicit_template_instantiation@@2HB" = linkonce_odr 
dso_local global i32 0, comdat, align 4
-// CHECK: @__cxx_init_fn_ptr.5 = private constant void ()* 
@"??__Ex@?$A@H@implicit_template_instantiation@@2HB@YAXXZ", section ".asdf", 
comdat($"?x@?$A@H@implicit_template_instantiation@@2HB")
+// CHECK: @__cxx_init_fn_ptr.5 = private constant void ()* 
@"??__E?x@?$A@H@implicit_template_instantiation@@2HB@@YAXXZ", section ".asdf", 
comdat($"?x@?$A@H@implicit_template_instantiation@@2HB")
 }
 
 // ... and here's where we emitted user level ctors.
Index: test/CodeGenCXX/microsoft-abi-static-initializers.cpp
===
--- test/CodeGenCXX/microsoft-abi-static-initializers.cpp
+++ test/CodeGenCXX/microsoft-abi-static-initializers.cpp
@@ -3,8 +3,8 @@
 // CHECK: @llvm.global_ctors = appending global [5 x { i32, void ()*, i8* }] [
 // CHECK: { i32, void ()*, i8* } { i32 65535, void ()* 
@"??__Eselectany1@@YAXXZ", i8* getelementptr inbounds (%struct.S, %struct.S* 
@"?selectany1@@3US@@A", i32 0, i32 0) },
 // CHECK: { i32, void ()*, i8* } { i32 65535, void ()* 
@"??__Eselectany2@@YAXXZ", i8* getelementptr inbounds (%struct.S, %struct.S* 
@"?selectany2@@3US@@A", i32 0, i32 0) },
-// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* 
@"??__Es@?$ExportedTemplate@H@@2US@@A@YAXXZ", i8* getelementptr inbounds 
(%struct.S, %struct.S* @"?s@?$ExportedTemplate@H@@2US@@A", i32 0, i32 0) },
-// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* 
@"??__Efoo@?$B@H@@2VA@@A@YAXXZ", i8* bitcast (%class.A* @"?foo@?$B@H@@2VA@@A" 
to i8*) },
+// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* 
@"??__E?s@?$ExportedTemplate@H@@2US@@A@@YAXXZ", i8* getelementptr inbounds 
(%struct.S, %struct.S* @"?s@?$ExportedTemplate@H@@2US@@A", i32 0, i32 0) },
+// CHECK: { i32, void ()*, i8* } { i32 65535, void ()* 
@"??__E?foo@?$B@H@@2VA@@A@@YAXXZ", i8* bitcast (%class.A* @"?foo@?$B@H@@2VA@@A" 
to i8*) },
 // CHECK: { i32, void ()*, i8* } { i32 65535, void ()* 
@_GLOBAL__sub_I_microsoft_abi_static_initializers.cpp, i8* null }
 // CHECK: ]
 
@@ -231,18 +231,18 @@
   DynamicDLLImportInitVSMangling::switch_test3();
 }
 
-// CHECK: define linkonce_odr dso_local void @"??__Efoo@?$B@H@@2VA@@A@YAXXZ"() 
{{.*}} comdat
+// CHECK: define linkonce_odr dso_local void 
@"??__E?foo@?$B@H@@2VA@@A@@YAXXZ"() {{.*}} comdat
 // CHECK-NOT: and
 // CHECK-NOT: ?_Bfoo@
 // CHECK: call x86_thiscallcc %class.A* @"??0A@@QAE@XZ"
-// CHECK: call i32 @atexit(void ()* @"??__Ffoo@?$B@H@@2VA@@A@YAXXZ")
+// CHECK: call i32 @atexit(void ()* @"??__F?foo@?$B@H@@2VA@@A@@YAXXZ")
 // CHECK: ret void
 
 // CHECK: define linkonce_odr dso_local x86_thiscallcc %class.A* 
@"??0A@@QAE@XZ"({{.*}}) {{.*}} comdat
 
 // CHECK: define linkonce_odr dso_local x86_thiscallcc void 
@"??1A@@QAE@XZ"({{.*}}) {{.*}} comdat
 
-// CHECK: define internal void @"??__Ffoo@?$B@H@@2VA@@A@YAXXZ"
+// CHECK: define internal void @"??__F?foo@?$B@H@@2VA@@A@@YAXXZ"
 // CHECK: call x86_thiscallcc void @"??1A@@QAE@XZ"{{.*}}foo
 // CHECK: ret void
 
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -3217,10 +3217,13 @@
   msvc_hashing_ostream MHO(Out);
   MicrosoftCXXNameMangler Mangler(*this, MHO);
   Mangler.getStream() << "??__" << CharCode;
-  Mangler.mangleName(D);
   if (D->isStaticDataMember()) {
+Mangler.getStream() << '?';
+Mangler.mangleName(D);
 Mangler.mangleVariableEncoding(D);
-Mangler.

[PATCH] D50877: [MS] Mangle a hash of the main file path into anonymous namespaces

[Zachary Turner via Phabricator via cfe-commits]

zturner added subscribers: rnk, zturner.
zturner added a comment.

IIRC it’s `?A0xABCDABCD@` where the hex value is some kind of hash


https://reviews.llvm.org/D50877



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

[PATCH] D50839: [llvm] Optimize YAML::isNumeric

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: llvm/include/llvm/Support/YAMLTraits.h:454
+inline bool isNumeric(StringRef S) {
+  if (S.empty())
+return false;

What would happen if we re-wrote this entire function as:

```
inline bool isNumeric(StringRef S) {
  uint64_t N;
  int64_t I;
  APFloat F;
  return S.getAsInteger(N) || S.getAsInteger(I) || (F.convertFromString(S) == 
opOK);
}
```

Would this a) Be correct, and b) have similar performance characteristics to 
what you've got here?


https://reviews.llvm.org/D50839



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

[PATCH] D50564: Add support for SEH unwinding on Windows.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D50564#1199285, @rnk wrote:

> In https://reviews.llvm.org/D50564#1198996, @cdavis5x wrote:
>
> > Could somebody verify that the `DISPATCHER_CONTEXT` struct is defined in 
> > `` for the Win8 and Win10 SDKs? I can't install them right now.
>
>
> I checked both, and they are available, so I think we should guard the 
> definition like this:
>
>   // Provide a definition for _DISPATCHER_CONTEXT for Win7 and earlier SDK 
> versions.
>   // Mingw64 has always provided this struct.
>   #if defined(_LIBUNWIND_SUPPORT_SEH_UNWIND) && !defined(__MINGW64__) && 
> defined(WINVER) && WINVER < _WIN32_WINNT_WIN8
>   # if defined(__x86_64__)
>   typedef struct _DISPATCHER_CONTEXT { ... } DISPATCHER_CONTEXT;
>   # elif defined(__arm__)
>   typedef struct _DISPATCHER_CONTEXT { ... } DISPATCHER_CONTEXT;
>   # endif
>   #endif
>
>
> Does that seem right? I'm not super familiar with SDK version detection, so I 
> might have the wrong macros.


I believe you need to check `VER_PRODUCTBUILD` from `` if you are 
looking for the version of the Windows SDK that is being used to build the 
current application.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D50564



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

[PATCH] D42762: Rewrite the VS Integration Scripts

[Zachary Turner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337572: Rewrite the VS integration scripts. (authored by 
zturner, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D42762?vs=156387&id=156520#toc

Repository:
  rC Clang

https://reviews.llvm.org/D42762

Files:
  tools/driver/CMakeLists.txt


Index: tools/driver/CMakeLists.txt
===
--- tools/driver/CMakeLists.txt
+++ tools/driver/CMakeLists.txt
@@ -63,10 +63,6 @@
 
 if(NOT CLANG_LINKS_TO_CREATE)
   set(CLANG_LINKS_TO_CREATE clang++ clang-cl clang-cpp)
-
-  if (MSVC)
-list(APPEND CLANG_LINKS_TO_CREATE ../msbuild-bin/cl)
-  endif()
 endif()
 
 foreach(link ${CLANG_LINKS_TO_CREATE})


Index: tools/driver/CMakeLists.txt
===
--- tools/driver/CMakeLists.txt
+++ tools/driver/CMakeLists.txt
@@ -63,10 +63,6 @@
 
 if(NOT CLANG_LINKS_TO_CREATE)
   set(CLANG_LINKS_TO_CREATE clang++ clang-cl clang-cpp)
-
-  if (MSVC)
-list(APPEND CLANG_LINKS_TO_CREATE ../msbuild-bin/cl)
-  endif()
 endif()
 
 foreach(link ${CLANG_LINKS_TO_CREATE})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45124: [CodeGen] Record if a C++ record is a trivial type when emitting Codeview

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Why do we only want this for CodeView?


Repository:
  rC Clang

https://reviews.llvm.org/D45124



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

[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Lgtm. I also have a hard time saying which is best, we’re basically trying
to help people who have already strayed from the path, so there’s probably
no objectively correct answer


https://reviews.llvm.org/D49398



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

[PATCH] D49398: clang-cl: Postpone Wmsvc-not-found emission until link.exe gets used.

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/Driver/ToolChains/MSVC.cpp:467-468
   if (Linker.equals_lower("link")) {
+if (!TC.FoundMSVCInstall())
+  C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+

It looks like it's possible for this warning to be emitted even when 
`FindVisualStudioExecutable` succeeds (after looking in the install location it 
checks `PATH`).  Would it make more sense to put this check after the call to 
`FindVisualStudioExecutable`, but only if it fails?


https://reviews.llvm.org/D49398



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

[PATCH] D48601: Added -fcrash-diagnostics-dir flag

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:4043-4044
+if (CCGenDiagnostics && A) {
+  SmallString<128> CrashDirectory;
+  CrashDirectory = A->getValue();
+  llvm::sys::path::append(CrashDirectory, Split.first);

Maybe you can combine these into one line with:

```
SmallString<128> CrashDirectory{ A->getValue() };
```



Comment at: clang/test/Driver/crash-diagnostics-dir.c:5
+// CHECK: Preprocessed source(s) and associated run script(s) are located at:
+// CHECK: diagnostic msg: {{.*}}/Output/crash-diagnostics-dir.c.tmp/{{.*}}.c

This will fail on Windows I think where path separators are backslashes.  I 
think you need to do something like:

```
{{.*}}Output{{/|\\}}crash-diagnostics-dir.c.tmp{{(/|\\).*}}.c
```



Comment at: llvm/include/llvm/Support/FileSystem.h:758-759
 
+std::error_code createUniqueFile(const Twine &Prefix, StringRef Suffix,
+ SmallVectorImpl &ResultPath);
 /// Represents a temporary file.

Is there any reason we can't just use the existing overload?  In other words, 
instead of creating this overload and having the user write 
`createUniqueFile("foo", "bar")`, how about just 
`createUniqueFile("foo-%%.bar")` which seems equivalent.


Repository:
  rL LLVM

https://reviews.llvm.org/D48601



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

[PATCH] D47887: Move VersionTuple from clang/Basic to llvm/Support, llvm part

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

It would be better if you're using a monorepo, that way both parts can just be 
one single patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D47887



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

[PATCH] D44778: [clang-format] Wildcard expansion on Windows.

[Zachary Turner via Phabricator via cfe-commits]

zturner added subscribers: alexfh, zturner.
zturner added a comment.

Never seen this PURE_WINDOWS CMake variable. How is it different than MSVC?


Repository:
  rC Clang

https://reviews.llvm.org/D44778



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

[PATCH] D44426: Fix llvm + clang build with Intel compiler

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D44426#1040938, @nhaehnle wrote:

> I don't think we should add workarounds for broken compilers.


I don't follow.  What should we do then?  If LLVM doesn't compile on a compiler 
which we claim is supported, then how should we proceed?


https://reviews.llvm.org/D44426



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

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

I had to revert this for now.  It breaks the asan bots which expect column 
info.  That fix is easy, but it also breaks a random linux bots which I don't 
have the ability to debug at the moment because my linux machine is not 
working.  Hopefully I can get that resolved soon.


Repository:
  rC Clang

https://reviews.llvm.org/D43700



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

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Zachary Turner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC326113: Emit proper CodeView when -gcodeview is passed 
without the cl driver. (authored by zturner, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43700?vs=135723&id=135932#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43700

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/codeview-column-info.c


Index: test/Driver/codeview-column-info.c
===
--- test/Driver/codeview-column-info.c
+++ test/Driver/codeview-column-info.c
@@ -0,0 +1,13 @@
+// Check that -dwarf-column-info does not get added to the cc1 line:
+// 1) When -gcodeview is present via the clang or clang++ driver
+// 2) When /Z7 is present via the cl driver.
+
+// RUN: %clang -### -c -g -gcodeview %s 2> %t1
+// RUN: FileCheck < %t1 %s
+// RUN: %clangxx -### -c -g -gcodeview %s 2> %t2
+// RUN: FileCheck < %t2 %s
+// RUN: %clang_cl -### /c /Z7 %s 2> %t2
+// RUN: FileCheck < %t2 %s
+
+// CHECK: "-cc1"
+// CHECK-NOT: "-dwarf-column-info"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2968,7 +2968,7 @@
 
   // Forward -gcodeview. EmitCodeView might have been set by CL-compatibility
   // argument parsing.
-  if (Args.hasArg(options::OPT_gcodeview) || EmitCodeView) {
+  if (EmitCodeView) {
 // DWARFVersion remains at 0 if no explicit choice was made.
 CmdArgs.push_back("-gcodeview");
   } else if (DWARFVersion == 0 &&
@@ -3567,6 +3567,8 @@
   types::ID InputType = Input.getType();
   if (D.IsCLMode())
 AddClangCLArgs(Args, InputType, CmdArgs, &DebugInfoKind, &EmitCodeView);
+  else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
 
   const Arg *SplitDWARFArg = nullptr;
   RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,


Index: test/Driver/codeview-column-info.c
===
--- test/Driver/codeview-column-info.c
+++ test/Driver/codeview-column-info.c
@@ -0,0 +1,13 @@
+// Check that -dwarf-column-info does not get added to the cc1 line:
+// 1) When -gcodeview is present via the clang or clang++ driver
+// 2) When /Z7 is present via the cl driver.
+
+// RUN: %clang -### -c -g -gcodeview %s 2> %t1
+// RUN: FileCheck < %t1 %s
+// RUN: %clangxx -### -c -g -gcodeview %s 2> %t2
+// RUN: FileCheck < %t2 %s
+// RUN: %clang_cl -### /c /Z7 %s 2> %t2
+// RUN: FileCheck < %t2 %s
+
+// CHECK: "-cc1"
+// CHECK-NOT: "-dwarf-column-info"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2968,7 +2968,7 @@
 
   // Forward -gcodeview. EmitCodeView might have been set by CL-compatibility
   // argument parsing.
-  if (Args.hasArg(options::OPT_gcodeview) || EmitCodeView) {
+  if (EmitCodeView) {
 // DWARFVersion remains at 0 if no explicit choice was made.
 CmdArgs.push_back("-gcodeview");
   } else if (DWARFVersion == 0 &&
@@ -3567,6 +3567,8 @@
   types::ID InputType = Input.getType();
   if (D.IsCLMode())
 AddClangCLArgs(Args, InputType, CmdArgs, &DebugInfoKind, &EmitCodeView);
+  else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
 
   const Arg *SplitDWARFArg = nullptr;
   RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D43700#1019533, @smeenai wrote:

> The `-g` meaning `-gcodeview` for MSVC environments change should be a 
> separate diff though, right?


Yes I'm not doing that in this patch.  Just wanted to clarify what he meant.  
I'm writing a test for this right now and then I'll commit.


https://reviews.llvm.org/D43700



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

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

@rnk by "in an MSVC environment" do you mean "when -fms-compatibility is 
present"?


https://reviews.llvm.org/D43700



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

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D43700#1018042, @colden wrote:

> Seems good to me! I'll give it a test on my end.
>
> One alternate implementation idea though, what if you defaulted EmitCodeView 
> to the hasArg check instead of false, then removed the `else *EmitCodeView = 
> false;` block on line 4999?


That would actually change the behavior of the cl driver, which I kind of don't 
want to do since it's not necessary.  It would whitelist an additional clang 
option to be recognized by the cl driver.  Specifically, you could then get 
debug info via the cl driver without specifying /Z7 or /Zi.  It makes the 
possibilities more confusing, and someone will invariably try to do it and 
screw something up.  We already whitelist some dash options so that the cl 
driver will recognize them, but it's on a case by case basis and only when 
there's a strong need for it.


https://reviews.llvm.org/D43700



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

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
zturner added a reviewer: rnk.

Although the supported way of invoking clang on Windows is via the cl driver, 
some people may wish to use the g++ driver. and manually specify `-gcodeview`.  
This codepath is currently broken, as it will cause column information to be 
emitted, which causes debuggers to not work.

While we still don't claim to support clang on Windows without the cl driver, 
this is a straightforward patch to make things slightly better for people who 
want to go this route anyway.


https://reviews.llvm.org/D43700

Files:
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2965,7 +2965,7 @@
 
   // Forward -gcodeview. EmitCodeView might have been set by CL-compatibility
   // argument parsing.
-  if (Args.hasArg(options::OPT_gcodeview) || EmitCodeView) {
+  if (EmitCodeView) {
 // DWARFVersion remains at 0 if no explicit choice was made.
 CmdArgs.push_back("-gcodeview");
   } else if (DWARFVersion == 0 &&
@@ -3552,6 +3552,8 @@
   types::ID InputType = Input.getType();
   if (D.IsCLMode())
 AddClangCLArgs(Args, InputType, CmdArgs, &DebugInfoKind, &EmitCodeView);
+  else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
 
   const Arg *SplitDWARFArg = nullptr;
   RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2965,7 +2965,7 @@
 
   // Forward -gcodeview. EmitCodeView might have been set by CL-compatibility
   // argument parsing.
-  if (Args.hasArg(options::OPT_gcodeview) || EmitCodeView) {
+  if (EmitCodeView) {
 // DWARFVersion remains at 0 if no explicit choice was made.
 CmdArgs.push_back("-gcodeview");
   } else if (DWARFVersion == 0 &&
@@ -3552,6 +3552,8 @@
   types::ID InputType = Input.getType();
   if (D.IsCLMode())
 AddClangCLArgs(Args, InputType, CmdArgs, &DebugInfoKind, &EmitCodeView);
+  else
+EmitCodeView = Args.hasArg(options::OPT_gcodeview);
 
   const Arg *SplitDWARFArg = nullptr;
   RenderDebugOptions(getToolChain(), D, RawTriple, Args, EmitCodeView,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41444: [ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Can you just have `getLangArgs` return a vector of vectors, and then in 
`testImport` run it in a loop over all sets of args?


Repository:
  rC Clang

https://reviews.llvm.org/D41444



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

[PATCH] D41368: [libc++] Ignore bogus tautologic comparison warnings

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

It would be better if we could just fix the code.  If another compiler comes 
along and implements this same warning, we're going to have to fix it again.  
The only difference between any of this function is which cstdlib function is 
called.  Why not just wrap all of this and put a cast inside the wrapped 
function, like:

  template<> inline int as_integer(const string &func, const wstring &s, size_t 
*idx, int base) {
return as_integer_helper(func, s, idx, base, wcstol);
  }

and then inside of this function put the range check and use a cast to silence 
the warning.


Repository:
  rCXX libc++

https://reviews.llvm.org/D41368



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

[PATCH] D39994: Loosen MSVC 2017 path requirements

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

I'm not suuuper opposed, but at the same time if this code is bothering people 
(and it is, consistently), I don't changing the requirements from "confusing 
rule A" to "confusing rule B" is going to solve the long term burden that 
people keep running into.

Not asking you to do this work, but my ideal solution is probably to teach 
clang-cl to recognize 3 new environment variables.

`CLANGCL_MSVC_BIN` - Where to look for `cl.exe`, and `link.exe`. Under no 
circumstances do we consult `PATH` or anything else.  This is only used when we 
need to fallback to `cl` (rarely, anymore) or when the compiler needs to invoke 
the linker.   But!  At the same time we make `-fuse-ld=lld` the default.  We 
only do something else if the user specifies `-fuse-ld=link`, and in that case 
it uses `CLANGCL_MSVC_BIN` (or if you specified `-fuse-ld=` then the 
env var isn't needed).

`CLANGCL_WINSDK` - Points to the root of the Windows SDK.  The folder here 
should have a "standard" layout so that it least pretends to be an 
installation, so that we can find the right lib directory when cross-compiling.

`CLANGCL_MSVCRT` - Same as before, but for the CRT.

I would honestly like to delete just about 100% of this stuff about finding 
MSVC.  We should just use exactly what we're told to use and nothing else.  
Simple, easy to understand, and easy to explain.

Anyway, I'm just venting.  If rnk@ wants to lgtm this, I'm fine.


https://reviews.llvm.org/D39994



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

[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

Do I understand correctly that this will insert breakpoints on *all* clang 
diagnostics?  That's not necessarily bad, but I was under the impression 
originally that it would let you pick the diagnostics you wanted to insert 
breakpoints on.

Also, What is the workflow for using the "clangdiag diagtool" subcommand?  
Would you have to do two steps, `clangdiag enable` and then `clangdiag 
diagtool`?  If so, maybe it could just be `clangdiag enable --diagtool=`


https://reviews.llvm.org/D36347



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

[PATCH] D38704: [libunwind] Abstract rwlocks into a class, provide a SRW lock implementation for windows

[Zachary Turner via Phabricator via cfe-commits]

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

This looks much better than the previous solution.  Thanks!


https://reviews.llvm.org/D38704



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

[PATCH] D39162: [test] Fix clang-test for FreeBSD and NetBSD

[Zachary Turner via Phabricator via cfe-commits]

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

Please don't throw an exception here.  Instead, write this as:

  shlibpath_var = None
  if platform.system() in ['Linux', 'FreeBSD', 'NetBSD']:
  shilbpath = 'LD_LIBRARY_PATH'
  elif platform.system() == 'Darwin':
  shlibpath_var = 'DYLD_LIBRARY_PATH'
  elif platform.system() == 'Windows':
  shlibpath_var = 'PATH'
  
  if shlibpath_var:
  shlibpath = os.path.pathsep.join((config.shlibdir, config.llvm_libs_dir, 
config.environment.get(shlibpath_var,'')))
  config.environment[shlibpath_var] = shlibpath
  else:
  lit_config.warning('Unable to determine shared library path variable for 
platform {}'.format(platform.system()))


https://reviews.llvm.org/D39162



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

[PATCH] D38549: [clang-tidy] Link targets and Asm parsers

[Zachary Turner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL316246: [clang-tidy] Don't error on MS-style inline 
assembly. (authored by zturner).

Changed prior to commit:
  https://reviews.llvm.org/D38549?vs=119677&id=119722#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38549

Files:
  clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler-msvc.cpp
  clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler.cpp


Index: clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler-msvc.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler-msvc.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler-msvc.cpp
@@ -0,0 +1,9 @@
+// REQUIRES: system-windows
+// RUN: %check_clang_tidy %s hicpp-no-assembler %t
+
+void f() {
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in 
safety-critical code [hicpp-no-assembler]
+  }
+}
Index: clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler.cpp
@@ -9,4 +9,9 @@
 void f() {
   __asm("mov al, 2");
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use inline assembler in 
safety-critical code [hicpp-no-assembler]
+
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in 
safety-critical code [hicpp-no-assembler]
+  }
 }
Index: clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
@@ -1,4 +1,7 @@
 set(LLVM_LINK_COMPONENTS
+  AllTargetsAsmParsers
+  AllTargetsDescs
+  AllTargetsInfos
   Support
   )
 
Index: clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
@@ -18,6 +18,7 @@
 #include "../ClangTidy.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/TargetSelect.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
@@ -403,6 +404,10 @@
 
   ProfileData Profile;
 
+  llvm::InitializeAllTargetInfos();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmParsers();
+
   ClangTidyContext Context(std::move(OwningOptionsProvider));
   runClangTidy(Context, OptionsParser.getCompilations(), PathList,
EnableCheckProfile ? &Profile : nullptr);


Index: clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler-msvc.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler-msvc.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler-msvc.cpp
@@ -0,0 +1,9 @@
+// REQUIRES: system-windows
+// RUN: %check_clang_tidy %s hicpp-no-assembler %t
+
+void f() {
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in safety-critical code [hicpp-no-assembler]
+  }
+}
Index: clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/hicpp-no-assembler.cpp
@@ -9,4 +9,9 @@
 void f() {
   __asm("mov al, 2");
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use inline assembler in safety-critical code [hicpp-no-assembler]
+
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in safety-critical code [hicpp-no-assembler]
+  }
 }
Index: clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/CMakeLists.txt
@@ -1,4 +1,7 @@
 set(LLVM_LINK_COMPONENTS
+  AllTargetsAsmParsers
+  AllTargetsDescs
+  AllTargetsInfos
   Support
   )
 
Index: clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/trunk/clang-tidy/tool/ClangTidyMain.cpp
@@ -18,6 +18,7 @@
 #include "../ClangTidy.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/TargetSelect.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
@@ -403,6 +404,10 @@
 
   ProfileData Profile;
 
+  llvm

[PATCH] D38549: [clang-tidy] Link targets and Asm parsers

[Zachary Turner via Phabricator via cfe-commits]

zturner updated this revision to Diff 119677.
zturner added a comment.

Added a test.  Alex, if you can confirm this test gives sufficient coverage I 
can go ahead and submit today.  Thanks!


https://reviews.llvm.org/D38549

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp


Index: clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp
===
--- clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp
+++ clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp
@@ -9,4 +9,9 @@
 void f() {
   __asm("mov al, 2");
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use inline assembler in 
safety-critical code [hicpp-no-assembler]
+
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in 
safety-critical code [hicpp-no-assembler]
+  }
 }
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -18,6 +18,7 @@
 #include "../ClangTidy.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/TargetSelect.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
@@ -403,6 +404,10 @@
 
   ProfileData Profile;
 
+  llvm::InitializeAllTargetInfos();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmParsers();
+
   ClangTidyContext Context(std::move(OwningOptionsProvider));
   runClangTidy(Context, OptionsParser.getCompilations(), PathList,
EnableCheckProfile ? &Profile : nullptr);
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -1,4 +1,6 @@
 set(LLVM_LINK_COMPONENTS
+  AllTargetsAsmParsers
+  AllTargetsInfos
   Support
   )
 


Index: clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp
===
--- clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp
+++ clang-tools-extra/test/clang-tidy/hicpp-no-assembler.cpp
@@ -9,4 +9,9 @@
 void f() {
   __asm("mov al, 2");
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not use inline assembler in safety-critical code [hicpp-no-assembler]
+
+  _asm {
+mov al, 2;
+// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: do not use inline assembler in safety-critical code [hicpp-no-assembler]
+  }
 }
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -18,6 +18,7 @@
 #include "../ClangTidy.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/TargetSelect.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::driver;
@@ -403,6 +404,10 @@
 
   ProfileData Profile;
 
+  llvm::InitializeAllTargetInfos();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmParsers();
+
   ClangTidyContext Context(std::move(OwningOptionsProvider));
   runClangTidy(Context, OptionsParser.getCompilations(), PathList,
EnableCheckProfile ? &Profile : nullptr);
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -1,4 +1,6 @@
 set(LLVM_LINK_COMPONENTS
+  AllTargetsAsmParsers
+  AllTargetsInfos
   Support
   )
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D36347#902157, @clayborg wrote:

> Please do convert to python. Just know that you can use "lldb -P" to get the 
> python path that is needed in order to do "import lldb" in the python script. 
> So you can try doing a "import lldb", and if that fails, catch the exception, 
> run "lldb -P", add that path to the python path:
>
>   try:
>   # Just try for LLDB in case the lldb module is already in the python 
> search
>   # paths
>   import lldb
>   except ImportError:
>   # We failed to import lldb automatically. Run lldb with the -P option so
>   # it tells us the python path we should use.
>   lldb_py_dirs = list()
>   (status, output) = commands.getstatusoutput("lldb -P")
>   dir = os.path.realpath(output)
>   if status == 0 and os.path.isdir(dir):
>   lldb_py_dirs.append(dir)
>   success = False
>   for lldb_py_dir in lldb_py_dirs:
>   if os.path.exists(lldb_py_dir):
>   if not (sys.path.__contains__(lldb_py_dir)):
>   sys.path.append(lldb_py_dir)
>   try:
>   import lldb
>   except ImportError:
>   pass
>   else:
>   success = True
>   break
>   if not success:
>   print("error: couldn't locate the 'lldb' module, please set "
> "PYTHONPATH correctly")
>   sys.exit(1)
>  
>


Is any of this really necessary?  If you load this script via `command script 
add` (which is definitely better than having this be a post-processing script 
that someone has to manually run) then it is guaranteed to be in the path.  
Just import it, like the other examples in `lldb/examples/python/jump.py` for 
an example.  The idea is to have it do the indexing when the command is loaded 
and save it to a global, and then each time it runs it uses the global index.  
This way it's invisible to the user, you just run `bcd -Wcovered-switch` or 
something without worrying about it.


https://reviews.llvm.org/D36347



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