Thanks for switching to pragma!

Lgtm

Isn't it better to silence this warning with a pragma instead of disabling it for the whole file?

(Please don't land this until @MaskRay have had his say).

LGTM - there shouldn't be that much use for duplicates here.

Uppercase all variables

Ran some internal benchmarks on 1570 files (C and C++ mixed but much more C++ than C):

Thanks for the review! I uploaded a new version addressing all (I think) of your feedback and added a release note.

Addressed review feedback.

Clean-ups and error handling.

Add test. Just repurpose the same test we already have but add a step to
generate ast first and then pushing that through extdef-mapping. It should
always produce the same result.

LGTM.

This still needs tests - but I wanted to get your early input on the approach here and what you all think.

Updated commit message

Just handle /o for AST and plist files

Fixed the test issue. Regarding /Fo bit - if you want I can change it to only handle /o in this if statement. I don't mind either way.

Fixed missing -- in test file

Added tests and moved the check for TY_Plist and TY_AST to it's own if statement block

Removed unintentional whitespace removal and landed that as a NFC

In D128409#3604460, @hans wrote:

I'm unfamiliar with -emit-ast. Can you add some background on what this is for? What's CTU?

Lgtm. Wait with merge until Hans checks it as well.

I was just also about to comment that it seems like it's just ignoring the incremental flag and that it has to be more to it?

A bunch of questions and comments. I think there are some really nice cleanups here, but also some features / behaviors that change that we need to understand.

Thanks for the patch! Wait for Hans to give his approval as well before committing.

Thanks for the test - but I am not sure it should be a part of nodefaultlib? Is there a reason you didn't add it as it's own test?

Yep seems correct to me as well - but I think we need a test.

Looks fine to me. Thanks for fixing!

Thanks for fixing - but it's not completely accurate right now.

FYI: I helped @PatriosTheGreat to commit this after he asked for help in discord.

Much cleaner! Thanks!

I think you can drop the content lines from the test cases. But other that that it looks fine.

This looks good - only a comment about error handling, but I think we can also address that later if we see it's a large problem.

I think I rather would name the variable LLVM_LINKER_SKIP_TEST or something like that. I don't like the cmake variable name and since it's not something upstream we are adding I think I rather use a more descriptive name.

Removed comment

Switched to unit test over lit test

In D125961#3527447, @curdeius wrote:

Thanks for the patch!
Could you please add a unit test in unittest/Format/FormatTest.cpp instead of in lit-based test/?
Is there a bug report that your patch fixes?

Nice!

In D125961#3526347, @wanders wrote:

There are other pragmas which include colon. How do they fare?

Example https://docs.microsoft.com/en-us/cpp/preprocessor/warning?view=msvc-170

#pragma warning( disable : 4507 34; once : 4385; error : 164 )

Added additional tests to check other pragma statements

Thanks everyone for your input! Let's hope this makes it easier for new users to pick the right defaults!

I will land this documentation update now since it's pretty low risk, and I want to land before the other diff making CMAKE_BUILD_TYPE mandatory. I'll be happy to make more changes if people feel it's needed. Hit me up with other changes in that case.

In D124153#3487580, @awarzynski wrote:

Thanks for seeing this through!

Could you also update the CMake invocation in https://github.com/llvm/llvm-project/blob/be656df18721dc55a1de2eea64a3f73b6afa29a2/llvm/docs/GettingStarted.rst#getting-the-source-code-and-building-llvm? Or https://clang.llvm.org/get_started.html? There's probably going to be many more places like this.

Added CMAKE_BUILD_TYPE in several places in the documentation.

Various grammar nits

Thanks everyone for the input! If you have any more input on this - please try to get to it today since I plan to merge this and the CMake change to show this documentation tomorrow CEST.

Update URL to documentation that will point directly to CMAKE_BUILD_TYPE when https://reviews.llvm.org/D124367 has landed, which I plan to land at the same time.

Added more detail to the release notes. Let me know if anyone else have any concerns - otherwise I plan to land this tomorrow morning CEST.

Added more details to Release Notes

In D124153#3485469, @aaron.ballman wrote:

LGTM!

I applied the patch locally and tried it out with the MSVC built-in CMake support and confirmed that everything still works as I'd expect.

Friendly ping on this one - I would love to get feedback positive or negative on this latest revision. I think it's important that we don't have anyone strongly objecting to this approach.

Thanks for all your detailed feedback @jhenderson

Latest batch up of updates

Rewording.

Make omitting CMAKE_BUILD_TYPE an error.

Finally fix this line

Smaller grammar fixes

Here is my latest attempt:

Review updates, make it a more readable table and point more people to
this table

In D124153#3476598, @jhenderson wrote:

I rarely, if ever, build the whole of LLVM on my Linux VM, because I don't need to, as I tend to work on only a small subset of the LLVM tools. This means that I rarely run into linker issues due to exhausted memory. I don't believe I ever bothered changing my linker default. Whilst my usage may not be the same as others, I want to raise it to highlight that the configuration isn't always broken - it mostly works just fine if you don't try and build everything at once.

I think this is fine. But I wonder if we should split out the version into a separate file that can be easier to parse and so we don't have to edit the version in several different places. In my last job we usually had a single VERSION file in the root that we read from CMake and all other places that needed the current version.

Thanks for the suggestions all - I am working on this and came up with this table, WDYT. I will upload this as a diff when I have addressed all the rest of the comments, but wanted to get the input on the table first since it's a bit annoying to mark-up correctly.

In D124153#3471610, @erichkeane wrote:

I'm OK with #1 and #2, but note that the combo in #3 is not ALWAYS broken, just if you don't have enough memory or are running too many link jobs at the same time. I'm not sure we want it to be an 'error'.

One thing we all seem to agree on is that documentation could be better - so I took a stab at that here - I think it's connected to this discussion, but I don't think it's the only fix, please see the doc suggestions here: https://reviews.llvm.org/D124367

In D124153#3467803, @tschuett wrote:

Rather than changing the default and annoying or confusing people, improve the documentation! Have cmake print a link to the awesome documentation.

In D124153#3467394, @aaron.ballman wrote:

I think both of the situations are basically the same with basically the same outcomes -- if the user picks the "correct" build target (either through the default or explicitly), they're happy, if not, they're unhappy.

In D124153#3467348, @aaron.ballman wrote:

Personally, I think the default depends entirely on who is doing the download and from where. If people are doing a git clone of the main branch, I think they're more often developers and would want a debug build and switching to release will be a burden/surprise/annoyance. If they're snagging source from the release downloads page, they're probably not doing active development with that and would want a release build of some kind and the current behavior is a burden/surprise/annoyance. Without some measurements of how users are getting to the source in the first place, I don't think we can reasonably pick a default for all situations and call it better than the current one. It'll just be different.

Added release note about assertions.