User Details
- User Since
- Oct 18 2019, 11:34 AM (141 w, 4 d)
Today
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):
Sun, Jul 3
Thanks for the review! I uploaded a new version addressing all (I think) of your feedback and added a release note.
Addressed review feedback.
Thu, Jun 30
Clean-ups and error handling.
Wed, Jun 29
Tue, Jun 28
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.
Mon, Jun 27
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
Thu, Jun 23
Removed unintentional whitespace removal and landed that as a NFC
Tue, Jun 21
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?
Thu, Jun 16
Wed, Jun 15
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.
Tue, Jun 14
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.
Fri, Jun 10
Looks fine to me. Thanks for fixing!
Thanks for fixing - but it's not completely accurate right now.
Thu, Jun 9
FYI: I helped @PatriosTheGreat to commit this after he asked for help in discord.
Tue, Jun 7
Much cleaner! Thanks!
Mon, Jun 6
I think you can drop the content lines from the test cases. But other that that it looks fine.
Jun 2 2022
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.
Jun 1 2022
May 31 2022
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.
May 27 2022
May 20 2022
Removed comment
Switched to unit test over lit test
Nice!
May 19 2022
Added additional tests to check other pragma statements
May 4 2022
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.
Added CMAKE_BUILD_TYPE in several places in the documentation.
May 3 2022
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
May 2 2022
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.
May 1 2022
Thanks for all your detailed feedback @jhenderson
Latest batch up of updates
Apr 29 2022
Rewording.
Make omitting CMAKE_BUILD_TYPE an error.
Apr 28 2022
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
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.
Apr 27 2022
Apr 26 2022
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.
Apr 25 2022
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
Apr 22 2022
Apr 21 2022
Added release note about assertions.