In D67647#1674795, @dblaikie wrote:

Right - I was suggesting that could be changed. Would it be OK to you to change arguments() to return ArrayRef? Or would you rather avoid that to avoid baking in more dependence on that alias violation?

Addressed feedback.

In D67740#1675564, @dblaikie wrote:

Looks like this might benefit from being split into independent changes - the work related to templates (I haven't looked closely, but I assume that's fairly indivisible) and the work related to other diagnostics seems fairly separable - and maybe there's other pieces too.

In D67743#1675533, @dblaikie wrote:

"Also, fix the order of if statements so that an explicit return_typestate annotation takes precedence over the default behavior for rvalue refs."

I'd probably have split that out into a separate patch - in part to discuss the design implications of that. I have some doubts about supporting non-consumed after passing by rvalue ref, but don't feel too strongly & the alternative would be having a warning about the attribute being ignored, etc - when it has a fairly clear meaning if it is there, just not sure people 'should' be doing that.

Fixed a mistake where the diagnostic would print the wrong parameter type.

In D67647#1674773, @dblaikie wrote:

I wasn't expecting it to involve all this work - but instead to change "arguments()" to return ArrayRef. Though perhaps you didn't go that route to allow future fixes to the strict aliasing (by having an adapting iterator - or perhaps arguments() already uses such an iterator that doesn't hit the strict aliasing issue?) needing to do even more cleaunp work?

Here's a new version of the patch that uses iterator ranges instead of ArrayRef, to avoid adding new uses of CallExpr::getArgs() in case it has to be removed in the future due to the strict aliasing issue.

Ugh, it looks like getArgs() is a massive strict aliasing violation. And it's used in enough different places in Clang that fixing the violation may require extensive refactoring... I've reported the issue as https://bugs.llvm.org/show_bug.cgi?id=43344, but I don't really want to fix it :)

Sounds good; I'll split this into a few separate patches.

Ping. I'm not qualified to review this myself but I'd like to see the bug fixed. :)

Ping.

Heh, I guess I'll request commit access, although I'm not sure if I have enough of a 'track record of submitting high quality patches'. But for now, can you commit this? Thanks :)

Addressed review comments.

Changes since last version:

Oops, I forgot to re-run git diff after fixing the patch.

Thanks for the feedback, updated.

Oops, forgot to mention:

Thanks for the review. I don't have commit access.

Second ping.

Ping.

Oh, forgot to mention in the initial submission: I don't have commit access so I'll need someone else to commit this.

Fix disassembling ARM instructions as big-endian

Sorry for the delay; here's an updated diff with a test. (The test didn't seem to clearly belong in any existing .s file I saw, so I just made a new one.)

Ping, and adding potential reviewers like I was supposed to do in the first place.

I don't have SVN access. If you want to commit it, feel free to remove that comment, or else I'll update the diff tomorrow.

Addressed comments. Thanks, rsmith!

(Ping? I verified yesterday that the patch still applies and passes regression tests.)

One more ping. As far as I know, everything has been addressed.

Okay.

Fixed raised issues.

Ping again.

Ping?

Per vsk:

• Changed to an explicit switch, and changed ContainsPlaceholderType logic.