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

zturner added a comment. If the current listed reviewers on this patch are not the best people for reviewing this, would one of them please suggest a more appropriate reviewer so we can get some traction on this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69585/new/ https://reviews

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

zturner added a comment. Adding a ping since it's been a week with no additional feedback for the author. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69585/new/ https://reviews.llvm.org/D69585 ___ cfe-commits mailing list cfe-commits@list

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

zturner added a comment. In D70553#1757862 , @aaron.ballman wrote: > Can you add a test case for this functionality? The reason there's no test case is because it seems like that wouldn't really be any different than testing the functionality of `llvm:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG64f74bf72eb4: [clang-tidy] Rewrite modernize-avoid-bind check. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D70368?vs=230661&id=231793#toc Repository: rG LLVM Github Monor

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

zturner updated this revision to Diff 230661. zturner added a comment. Addressed suggestions from @Eugene.Zelenko CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70368/new/ https://reviews.llvm.org/D70368 Files: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp clang-tools-ex

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

zturner updated this revision to Diff 230495. zturner added a comment. - Updated documentation for this check - Incorporated additional suggestions from @aaron.ballman - Fixed an invalid transformation that was generated when binding a member function and the second argument of `bind` (the object

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

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 impossi

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

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.cp

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

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-

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

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 ignoreTemporar

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

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::bi

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

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

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

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 'th

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

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/AvoidBind

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

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.

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

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:

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

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

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

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

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

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 =

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

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 sh

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

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

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

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@lis

[PATCH] D57896: Variable names rule

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

[PATCH] D57896: Variable names rule

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 diss

[PATCH] D57896: Variable names rule

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://rev

[PATCH] D57896: Variable names rule

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 pre

[PATCH] D57896: Variable names rule

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 CHA

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

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 id

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

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.

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

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'

[PATCH] D55715: Add AddressSpace mangling to MS mode

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

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

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/ h

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

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 000

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

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

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

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 w

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

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, becaus

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

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

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

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 immedia

[PATCH] D54567: Fix LLDB's lit files

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=174

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

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::VariadicAllOfMa

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

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::VariadicAllOfMa

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

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 abstract

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

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

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

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 know

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

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 suppor

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

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/outpu

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

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/listinf

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

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

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

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?

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

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

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

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 ni

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

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 b

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

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 ___

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

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 t

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

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, b

[PATCH] D52843: Update Clang Windows getting started docs

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 Pro

[PATCH] D52843: Update Clang Windows getting started docs

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; perh

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

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 subscrib

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

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

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

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: > ztur

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

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

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

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 _

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

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

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

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 ___

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

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

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

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 se

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

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 la

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

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 mig

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

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/

[PATCH] D51847: Print correctly dependency paths on Windows

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.

[PATCH] D51847: Print correctly dependency paths on Windows

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

[PATCH] D51847: Print correctly dependency paths on Windows

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

[PATCH] D51847: Print correctly dependency paths on Windows

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/listinf

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

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

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

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

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

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) {

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

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

[PATCH] D42762: Rewrite the VS Integration Scripts

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

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

zturner added a comment. Why do we only want this for CodeView? Repository: rC Clang https://reviews.llvm.org/D45124 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

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 warnin

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

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 yo

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

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.ll

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

zturner added subscribers: alexfh, zturner. zturner added a comment. Never seen this PURE_WINDOWS CMake variable. How is it different than MSVC? Repository: rC Clang https://reviews.llvm.org/D44778 ___ cfe-commits mailing list cfe-commits@lists.l

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

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

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

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 s

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

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 Repo

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

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 thi

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

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

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

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 =

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

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

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

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@l

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

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 thi

[PATCH] D39994: Loosen MSVC 2017 path requirements

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

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

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

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

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

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

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'

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

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://revi

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

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/

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

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

  1   2   >