@aaron.ballman s comments.

@mclow.lists s comments.

In D49504#1167045, @mclow.lists wrote:

This looks ok to me - I'm assuming that somewhere there's a test for int foo = 0x123d, so we're sure that didn't get broken.

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

In D49403#1165845, @rjmccall wrote:

Can you explain why this is important for the optimizer?

Bump! Would love to have someone take a look!

In D46915#1162095, @leonardchan wrote:

In D46915#1162038, @erichkeane wrote:

See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161

This patch doesn't seem to properly consider integers, and ignores octals, which I suspect shouldn't be the case.

I just requested an account on bugs.llvm.org and will respond here for now until I can comment on the thread there.

So the syntax for fixed point literals should be the same as that for floating point constants according to clause 6.4.4.2a in the embedded C spec (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1169.pdf). Integers and octal literals should not be supported since floating point literals can only be declared in decimal or hexadecimal notation.

See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161

@aaron.ballman any more feedback here? Or am I good to go?

@lebedev.ri 's comments, plus a rebase.

In D47474#1154858, @lebedev.ri wrote:

Some drive-by nits.
General observation: this is kinda large.
I would personally split split off the codegen part into a second patch.

In D47073#1155018, @chandlerc wrote:

I like the idea of having CMake warn users about older host toolchains that are at risk of ceasing to be supported. I might target the warning point at 2 years rather than 1.5 years though.

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.

Bump!

I think I got all of @aaron.ballman s comments.

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?

In D48788#1150203, @Meinersbur wrote:

In D48788#1150183, @erichkeane wrote:

All tests pass!

Very cool, thank you!

In D48100, @echristo mentioned that it broke test/Sema/attr-target-mv.c. No tests are changed here (so it's a NFC patch?), i.e. this is not fixed yet? The test case could be adapted by just changing the order of __vectorcall and __attribute__((target to emit the same message as before.

How do you intend to commit it? Just merging with D48100 into a single commit?

All tests pass!

In D48788#1149657, @Meinersbur wrote:

Would it be possible (not in this patch) to consolidate the different lists of attributes: ParsedAttributes, AttributeFactory, AttributeList, AttributePool, NonOwningParsedAttributes, NonOwningParsedAttributesWithRange? Naively, expect to use something like std::vector<Attribute*>. Currently AttributeList can be a single attribute as well as a list of attributes.

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.

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.

In D48100#1142842, @rsmith wrote:

This patch appears to have at least caused us to process attributes too many times in some cases. For example:

__attribute__((noreturn,noinline,unused)) void f();

before your patch gives this with clang -Xclang -ast-dump:

`-FunctionDecl 0xccef1e0 <<stdin>:1:1, col:50> col:48 f 'void () __attribute__((noreturn))'
  |-UnusedAttr 0xccef280 <col:34> unused
  `-NoInlineAttr 0xccef2c8 <col:25>

and after your patch gives this:

`-FunctionDecl 0xb913740 <<stdin>:1:1, col:50> col:48 f 'void () __attribute__((noreturn))'
  |-NoInlineAttr 0xb9137e0 <col:25>
  |-UnusedAttr 0xb913828 <col:34> unused
  |-NoInlineAttr 0xb913838 <col:25>
  `-UnusedAttr 0xb913848 <col:34> unused

So, how about it? Just compiler versions for now, but hopefully this will give us the ability to use better standards versions.

Just a quick bump, I know most of you are still recovering from the Switzerland trip, but hopefully the others will have some time.

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?

Separated out the other patch as Eli suggested (which has now been committed), and rebased this patch on top of it.