- User Since
- Jun 28 2016, 8:37 AM (107 w, 2 d)
@aaron.ballman s comments.
@mclow.lists s comments.
I've got a patch on deck in my environment, but I'll commit that when I'm done with it.
All of @aaron.ballman s comments have been dealt with except for Cpus->CPUs in the Attr.td file for explained reasons. Still negotiable :)
Tue, Jul 17
Mon, Jul 16
Bump! Would love to have someone take a look!
Fri, Jul 13
See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161
Thu, Jul 12
Wed, Jul 11
@aaron.ballman any more feedback here? Or am I good to go?
Tue, Jul 10
Mon, Jul 9
@lebedev.ri 's comments, plus a rebase.
1.5 is derived from the '3 year' limit here. We were intending to make sure that this would warn for users for at least 'a version or two'. 2 years would likely still work since we'd have a release between the 2 and 3 years. THAT SAID, I'd be very much against a warning without some sort of date-based rule in place. Otherwise all we end up doing is making it so that changing our required compiler version become a 2 year+ long process, since we'd need to warn for that long.
I'm curious why that file is declaring the builtin like that? The declaration doesn't seem like it is ever useful, builtins are available without any includes/etc as far back as I could go on all 3 compilers that I validated against.
Fri, Jul 6
Thu, Jul 5
I think I got all of @aaron.ballman s comments.
Tue, Jul 3
Thanks for the review! I fixed the two nits. I'm definitely going to wait for @aaron.ballman to give the final approval, he's promised me a slice of his time when he gets a chance :)
Forgot to answer this before:
How do you intend to commit it? Just merging with D48100 into a single commit?
Mon, Jul 2
All tests pass!
Fix things found by @Meinersbur
This patch rebases, since I didn't have @Meinersbur s revert. Additionally, I fix a couple of stupid errors, and get failure count down to 47!
Running through Aaron's first set of comments.
One question that came to my mind is whether we should be attempting to add some better const correctness as part of this refactoring. This part of the code has always been unsatisfactory in that regard and I think the lack of const correctness is part of why the semantics here are as muddled as they are. If we add more const correctness, the patch is likely to grow even larger, but it may also point out the areas where we're doing something questionable when there's now a better way.
Fri, Jun 29
The most interesting changes are in AttributeList.h. I also forgot to mention: I'd love to be more aggressive about the ownership semantics (in fact, my first 2 attempts were much stricter), however we make a lot of assumptions based on the ability to pass attribute lists between functions and have them modify on eachother. I'm hoping the tests that fail aren't too related to being able to modify the list for a different ParsedAttributes collection.
Mon, Jun 25
Fri, Jun 22
Wed, Jun 20
So, how about it? Just compiler versions for now, but hopefully this will give us the ability to use better standards versions.
Jun 15 2018
Just a quick bump, I know most of you are still recovering from the Switzerland trip, but hopefully the others will have some time.
Jun 13 2018
I believe my tests DO validate the return value correctly, don't they? It uses a sentinel, but the ternary should check that return value, right?