[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] 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] 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] D38549: [clang-tidy] Link targets and Asm parsers

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
Herald added subscribers: xazax.hun, mgorny.

Currently clang-tidy fails on any file that includes inline assembly.  This is 
not a great out-of-the-box experience since many system headers include inline 
assembly, particularly on Windows.

I checked through the various other clang tools and they all have similar logic 
to link in a minimal amount of backend stuff to get a target registry, so this 
patch does the same for clang-tidy


https://reviews.llvm.org/D38549

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp


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;
@@ -401,6 +402,10 @@
 return 0;
   }
 
+  llvm::InitializeAllTargetInfos();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmParsers();
+
   ProfileData Profile;
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -1,5 +1,7 @@
 set(LLVM_LINK_COMPONENTS
   Support
+  AllTargetsAsmParsers
+  AllTargetsInfos
   )
 
 add_clang_library(clangTidy


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;
@@ -401,6 +402,10 @@
 return 0;
   }
 
+  llvm::InitializeAllTargetInfos();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmParsers();
+
   ProfileData Profile;
 
   ClangTidyContext Context(std::move(OwningOptionsProvider));
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -1,5 +1,7 @@
 set(LLVM_LINK_COMPONENTS
   Support
+  AllTargetsAsmParsers
+  AllTargetsInfos
   )
 
 add_clang_library(clangTidy
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38704: [libunwind] Emulate pthread rwlocks via SRW locks for windows

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

I'm a little nervous about re-inventing a poor man's version of a reader writer 
lock.  Can we not just copy LLVM's?




Comment at: src/UnwindCursor.hpp:20
 #ifndef _LIBUNWIND_HAS_NO_THREADS
-  #include 
+  #ifdef _WIN32
+#include 

Maybe you want to check `_MSC_VER` here?  I think MinGW compilers will have 
`pthread` support.



Comment at: src/UnwindCursor.hpp:43-45
+#if !defined(_LIBUNWIND_HAS_NO_THREADS) && defined(_WIN32)
+#define pthread_rwlock_t SRWLOCK
+#define PTHREAD_RWLOCK_INITIALIZER SRWLOCK_INIT

As a matter of principle, I'm not a huge fan of naming things with posix 
specific names if the implementation is not actually posix.  For example, these 
functions always return success, but this is not true of the posix functions 
which can fail for various reasons.  In general, if people are unaware that 
there is some abstraction behind the scenes for Windows, and they just see a 
function named `posix_rwlock_rdlock`, then they are going to assume that they 
can expect the semantics documented on the posix man pages, which is not true.

The alternative is to create a proper abstraction, which might be more work 
than you're interested in taking on, so consider this just an advisory 
suggestion.



Comment at: src/UnwindCursor.hpp:50
+static bool lockedForWrite = false;
+static int pthread_rwlock_rdlock(pthread_rwlock_t *lock) {
+  AcquireSRWLockShared(lock);

This doesn't seem like what you want.  a global `static` function / variable in 
a header file is going to be duplicated in every translation unit.  i.e. two 
translation units will have different copies of `lockedForWrite`.  Same goes 
for the rest of the functions.



Comment at: src/UnwindCursor.hpp:61-64
+  if (lockedForWrite) {
+lockedForWrite = false;
+ReleaseSRWLockExclusive(lock);
+  } else {

Doesn't `lockedForWrite` need to be atomic?  If it is not atomic, there is no 
guarantee that thread 2 will see the results of thread 1's modifications with 
any kind of reasonable order.


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] 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 subscriber: lldb-commits.
zturner added a comment.

One possible reason for why this never got any traction is that `lldb-commits` 
wasn't added as a subscriber.  While it's true that the tagged people should 
have chimed in, having the whole commits list will get some more visibility.  I 
never saw this come to my inbox.

I think this would be most suitable in the `lldb/examples` folder.

I can't really review this thoroughly, because it relies on a bash script, and 
I use Windows where we bash isn't really a thing.  My bash is rusty, but it 
looks like you're embedding a python script in the bash script?  It might be 
good if this were just an lldb script command.  Take a look at `command script 
add` in LLDB, and in the `examples` folder for some examples of existing 
commands that work this way.  The nice thing about doing it this way is that 
you could just be inside LLDB and write `(lldb) break-diag -Wcovered-switch`, 
for example, which would be a much tighter integration with the debugger.


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] 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

[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] 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] 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] 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] 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] 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] 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 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.

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] 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] 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] 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 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#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]

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.

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] D33923: [Driver] Don't force .exe suffix for lld

[Zachary Turner via Phabricator via cfe-commits]

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

Seems fine to me.


https://reviews.llvm.org/D33923



___
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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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.

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#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#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] 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] 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] 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] 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] 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.

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] 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] 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] 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] 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] 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 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 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#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 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] 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] 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:
> 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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#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 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] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang/test/CodeGenCXX/debug-info-nested-exprs.cpp:44
+  int a = bar(x, y) +
+  baz(x, z) +
+  qux(y, z);

inglorion wrote:
> zturner wrote:
> > Can you make a function called `int foo()` and make this `int a = 
> > bar(foo(), y) + ...`
> Yes. Why? To test an additional level of nesting?
Yes.  for that matter, even better would be if this call to `foo()` spans 
multiple lines.  Right here you've got a single statement which spans multiple 
lines, but no individual sub-expression spans multiple lines.  And FWICT there 
is no nesting at all, since + is just a builtin operator.


https://reviews.llvm.org/D37529



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

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.h:65
   llvm::MDNode *CurInlinedAt = nullptr;
+  bool LocationEnabled = true;
   llvm::DIType *VTablePtrType = nullptr;

Can you move this line up to put it next to another bool?  Not a huge deal, but 
might as well pack the class members.



Comment at: clang/test/CodeGenCXX/debug-info-nested-exprs.cpp:44
+  int a = bar(x, y) +
+  baz(x, z) +
+  qux(y, z);

Can you make a function called `int foo()` and make this `int a = bar(foo(), y) 
+ ...`


https://reviews.llvm.org/D37529



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

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang/test/CodeGenCXX/debug-info-nested-exprs.cpp:44
+  int a = bar(x, y) +
+  baz(x, z) +
+  qux(y, z);

inglorion wrote:
> zturner wrote:
> > inglorion wrote:
> > > zturner wrote:
> > > > Can you make a function called `int foo()` and make this `int a = 
> > > > bar(foo(), y) + ...`
> > > Yes. Why? To test an additional level of nesting?
> > Yes.  for that matter, even better would be if this call to `foo()` spans 
> > multiple lines.  Right here you've got a single statement which spans 
> > multiple lines, but no individual sub-expression spans multiple lines.  And 
> > FWICT there is no nesting at all, since + is just a builtin operator.
> The nesting here is the calls to bar, baz, and qux inside the declaration of 
> a. The old behavior would emit separate locations for each of the calls, the 
> new behavior annotates each of the calls with the same location as the 
> declaration. This causes the debugger to stop only once for the entire 
> statement when using step over, and makes step into specific work.
Sure, but does that execute the same codepath as:

```
int a = bar(
  baz());

a = bar(baz());

for (const auto &X : foo(bar()))

for (int x = foo(); x < bar(); baz(x))

if (foo() && bar())

if (createObject().func())
```

etc?  And what if one or more of these functions get inlined?  For example, 
what if you have:

```
int a = foo(bar(baz()), bar(buzz()));
```



https://reviews.llvm.org/D37529



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

[PATCH] D37529: [codeview] omit debug locations for nested exprs unless column info enabled

[Zachary Turner via Phabricator via cfe-commits]

zturner added a comment.

@inglorion : Since we're already in this code anyway, should we at least do due 
diligence and check the basics, even if only manually?  As in, at least we 
should check in MSVC if any of those other types of examples that I came up 
with present a problem.  Otherwise we're going to be back here fixing this 
again in a few weeks.  And it doesn't seem like it would be that hard to whip 
up some sample code with 10 or 12 cases and just do a sanity check that they 
all work.  If they don't, we can then decide whether it should be fixed in this 
patch, or a bug can be filed to track it.


https://reviews.llvm.org/D37529



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

[PATCH] D37818: [lit] Update clang and lld to use the new shared LLVMConfig stuff

[Zachary Turner via Phabricator via cfe-commits]

zturner created this revision.
Herald added a reviewer: modocache.
Herald added a subscriber: fedor.sergeev.

This is probably the last work I'm going to do here for a while.  I still see 
some room for improvement, but I think after this patch:

a) refactor begins to have diminishing returns
b) There's a sufficient amount of sharing and re-use to validate the design
c) There's enough specific examples of how to use this that other projects 
(e.g. compiler-rt, etc) can use those as a model of how to simplify their own 
configs.

In other words, I'm not brave enough to touch compiler-rt's config files :)

There's some minor changes in the config api that increase flexibility in how 
you override environment variables and spawn an external `llvm-config`, but 
otherwise this is mostly just deleting code.


https://reviews.llvm.org/D37818

Files:
  clang/test/lit.cfg
  clang/test/lit.site.cfg.in
  lld/test/lit.cfg
  lld/test/lit.site.cfg.in
  llvm/test/lit.cfg
  llvm/utils/lit/lit/llvm/config.py

Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -1,4 +1,5 @@
 import os
+import platform
 import re
 import subprocess
 import sys
@@ -23,35 +24,40 @@
 
 # Tweak PATH for Win32 to decide to use bash.exe or not.
 if sys.platform == 'win32':
-# For tests that require Windows to run.
-features.add('system-windows')
-
 # Seek sane tools in directories and set to $PATH.
 path = self.lit_config.getToolsPath(config.lit_tools_dir,
config.environment['PATH'],
['cmp.exe', 'grep.exe', 'sed.exe'])
 self.with_environment('PATH', path, append_path=True)
 
-if use_lit_shell:
-# 0 is external, "" is default, and everything else is internal.
-self.use_lit_shell = (use_lit_shell != "0")
-else:
-# Otherwise we default to internal on Windows and external elsewhere, as
-# bash on Windows is usually very slow.
-self.use_lit_shell = (sys.platform in ['win32'])
+self.with_environment('LLVM_SRC_ROOT', config.llvm_src_root)
 
 self.use_lit_shell = litsh
-if not self.litsh:
+if not litsh:
 features.add('shell')
 
+
+# Running on Darwin OS
+if platform.system() in ['Darwin']:
+# FIXME: lld uses the first, other projects use the second.
+# We should standardize on the former.
+features.add('system-linker-mach-o')
+features.add('system-darwin')
+elif platform.system() in ['Windows']:
+# For tests that require Windows to run.
+features.add('system-windows')
+
 # Native compilation: host arch == default triple arch
-# FIXME: Consider cases that target can be executed
-# even if host_triple were different from target_triple.
-if config.host_triple == config.target_triple:
+# Both of these values should probably be in every site config (e.g. as
+# part of the standard header.  But currently they aren't)
+host_triple = getattr(config, 'host_triple', None)
+target_triple = getattr(config, 'target_triple', None)
+if host_triple and host_triple == target_triple:
 features.add("native")
 
 # Sanitizers.
-sanitizers = frozenset(x.lower() for x in getattr(config, 'llvm_use_sanitizer', []).split(';'))
+sanitizers = getattr(config, 'llvm_use_sanitizer', '')
+sanitizers = frozenset(x.lower() for x in sanitizers.split(';'))
 features.add(binary_feature('address' in sanitizers, 'asan', 'not_'))
 features.add(binary_feature('memory' in sanitizers, 'msan', 'not_'))
 features.add(binary_feature('undefined' in sanitizers, 'ubsan', 'not_'))
@@ -64,7 +70,6 @@
 if lit.util.pythonize_bool(long_tests):
 features.add("long_tests")
 
-target_triple = getattr(config, 'target_triple', None)
 if target_triple:
 if re.match(r'^x86_64.*-linux', target_triple):
 features.add("x86_64-linux")
@@ -80,25 +85,34 @@
 if gmalloc_path_str is not None:
 self.with_environment('DYLD_INSERT_LIBRARIES', gmalloc_path_str)
 
-breaking_checks = getattr(config, 'enable_abi_breaking_checks')
-if lit.util.pythonize_bool(lit_config, breaking_checks):
+breaking_checks = getattr(config, 'enable_abi_breaking_checks', None)
+if lit.util.pythonize_bool(breaking_checks):
 features.add('abi-breaking-checks')
 
 def with_environment(self, variable, value, append_path = False):
-if append_path and variable in self.config.environment:
+if append_path:
+# For paths, we should be able to take a list

[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] 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 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] 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.

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] 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] 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] 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] 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] 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] 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] 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] 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]

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] 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] 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] 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 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] 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] 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 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 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] 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