@arsenm @efriedma we'll update the SEH patch with this change if it's ready?

Hi, this patch has been up for 3 months. Is there anything reviewers would like addressed before it's committed?

Thanks everyone for the great feedback!
We'll move forward with landing this first patch and start the review of the next one.

@rjmccall @rsmith Thanks for your help. Do you have any additional feedback before we commit?

In D80344#2671157, @lebedev.ri wrote:

It would be good for @rjmccall / @rsmith / etc to actually finish reviewing this and accept it.
I would personally want to see the next patches - what changes are needed for llvm analysis, transforms?

Hi John, how does this look now?

This patch looks ready to land. Are there any other concerns or feedback?

addressed reviewer comments

Since this has no impact on anyone I think we can just commit it. Anyone else want to comment on the change?

In D89601#2336431, @jpienaar wrote:

In D89601#2336394, @asmith wrote:

In D89601#2336384, @jpienaar wrote:

Nice, to follow the naming convention of other utils folders here, how about mlir/utils/vscode/mlir.natvis ? (or s/vscode/vstudio/? I don't now who which is considered more general/common in folks minds :)).

This is following the convention of the other natvis files.
./clang/utils/ClangVisualizers/clang.natvis
./llvm/utils/LLVMVisualizers/llvm.natvis

Ah I see, that makes sense.

In D89601#2336384, @jpienaar wrote:

Nice, to follow the naming convention of other utils folders here, how about mlir/utils/vscode/mlir.natvis ? (or s/vscode/vstudio/? I don't now who which is considered more general/common in folks minds :)).

@rjmccall wrote:
I think you're missing what I'm asking. If LLVM accepts this feature, it will become our collective responsibility as a project to keep it working. You have a large external correctness test suite for this feature. It does not sound like you intend to open-source that test suite; instead, you intend to extract a small number of unit tests from it and add those to the LLVM test suite. So I'm asking if you're at least going to have an external CI bot which will run the full test suite for this feature to ensure it hasn't been broken by the last day of commits. It does not seem reasonable to expect that the few unit tests you extract will themselves be sufficient to keep the feature tested.

Yes there was an RFC and discussion and several requests for comments.
http://lists.llvm.org/pipermail/llvm-dev/2020-March/140541.html

@srhines Not at the moment. I updated the PR with the changes I have locally.

The buildbot is using MSVC 19.16.27042.0 which is Visual Studio 2017 15.9.25.

The warning is due to the version of Gtest that LLVM is using.

I think this should be okay. VS2017 or higher are required to build LLVM and this support is in VS2017.

Some changes for grammar to the title -- "[lldb/Process/Windows] Attempting to kill an exited/detached process is not an error"

Thanks for all the helpful feedback. Before we commit this just want to double check there are no final comments?

Duplicated by https://reviews.llvm.org/D79474

LGTM. Anyone else have comments?

really happy for native bfloat support!

Thanks for all the comments! Does this look good? Or other considerations?

In D78982#2012992, @MyDeveloperDay wrote:

LGTM

Enable for C#

The is done unless there are other comments.

In D78982#2007172, @MyDeveloperDay wrote:

• This needs unit tests in clang/unittests/Format to demonstrate the problem and test this fixing it

Rename option
Add test case
Only apply to Microsoft CPP style

Fix line breaks like this:

This change tries to fix this bug.
https://bugs.llvm.org/show_bug.cgi?id=32810

In D78221#1985170, @MaskRay wrote:

Use the unique filenames that are used when /lldsavetemps is passed.

Is this for debug convenience? (llvm-pdbutil dump output) Or required by some tools?

Nit - update comment

Adding a test

Curious what the status of this is? Looks like its been ready for almost one year :)

Address comments on comment character #, auto and rst file.

In D67689#1679198, @jhenderson wrote:

Thinks for the patch! I haven't had time to review the meat of it yet, but will try to get back to it in the next couple of days. One quick question in the meantime: what's the motivation behind this? Is it to improve GNU compatibility?

Add new options to objcopy documentation

Address reviewers comments about auto

Any comments from the llvm-objcopy community?

This was split into separate patches and committed. No longer need this review.

Any more comments on the tests for Windows?