Fix committed in https://reviews.llvm.org/rG16501782c8d849bc1812d527dc8466574700663d. Thanks for working on this!

Hi Roger,

Reid, thanks for looking at this!

Ping!

Chandler, thank you for the review!

Pinging again! (Third time's the charm?)

...pinging again!

Ping!

Chris, Chandler, thank you for the review!

@lattner, thank you for the quick review -- I hoped / feared for more discussion! :-)

@uweigand Ulrich, any objections here? If not, I will commit this patch tomorrow (as the author, Vladimir, doesn't have commit access yet).

Hi Renato,

I can commit this patch on Monday, when I will reach my work machine.

In D18035#450247, @hfinkel wrote:

The link was posted here: http://sourcerytools.com/pipermail/cxx-abi-dev/2016-June/002919.html - which is the mailing list where the C++ ABI specification is discussed.

In D18035#448057, @hfinkel wrote:

We just received the first draft of that specification yesterday.

Added a test for __unaligned arrays.

In D20103#425855, @majnemer wrote:

Can we test pointers to data members? Is it possible to have __unaligned int *S::* or int *S::* __unaligned or even __unaligned int *S::* __unaligned ?

Patch updated to address David's comments.

David, thank you for the thorough review! -- it definitely made the patch stronger and me even more paranoid than the rest of Intel. :-)

David, just noticed that your first question doesn't have a redefinition. For this program:

David, thank you for the review!

Fixed a bug uncovered by David Majnemer's review; added tests he asked for.

Reid, thank you!

1. Added more comprehensive overloading tests
2. Added a warning (in C mode) / error (in C++ mode) on unaligned to non-unaligned type conversion

This patch implements "__unaligned" as a qualifier (as per David Majnemer's request).

In D18596#392341, @majnemer wrote:

In D18596#392098, @andreybokhanko wrote:

Do we want to do this for an ignored qualifier? I don't see any practical purpose.

It's not ignored for Windows on ARM.

In D18596#388295, @aaron.ballman wrote:

Regression is a bit of a question mark, to me depending on the diagnostic. I think warning the user "this has absolutely no effect" is a reasonable diagnostic for that situation -- the user wrote something, possibly expecting it to have an effect, and it turns out that it does absolutely nothing (including in the compiler that implements the language extension). If MSVC were to ever add semantic effect in those cases (diverging from what Clang implements), the diagnostic becomes even more important for Clang users. So I think it's fine to accept __unaligned for non-pointer types, but issue an "attribute ignored" warning diagnostic.

Added Sema test, as per Aaron's and Reid's request.

Aaron, Reid, David, thank you for the review!

Resolved [some of] comments made by Aaron and Reid.

It seems the discussion (http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160229/thread.html#151860) concluded that we don't want this fix to be committed until we see some compelling code that relies on "static_assert" availability in C language without <assert.h> being included.

@majnemer, this error in the header is now gone. Thank you! On to more errors! ;-)

Patch updated in response to Reid's proposal (to introduce KEYMSCOMPAT).

@rnk, fair enough. OK, please consider this review request dropped.

OK, so is initial patch a good one? ;-)

In D17330#354730, @majnemer wrote:

Why not just stick clang in C90 mode when targeting C if the -fms-compatibility-version is 18?

We have similar code for the C++ mode in https://github.com/llvm-mirror/clang/blob/master/lib/Driver/Tools.cpp#l5069

Along with Alexey's addition, this looks good.

John, thank you for all these tireless re-reviews -- very much appreciated!

In D16851#349537, @silvas wrote:

Assuming the features are implemented this seems fine. LGTM.

John, thanks for the re-review! -- I did as you advised, and added *a lot* of switches. :-)

Ping

John, thank you for the review!

Ping!

Rafael, thanks for the review!

@rafael, all these changes are driven by tests.

@rafael, thank you!

@tra, @rnk, @rjmccall, thanks for the review!

Patch updated to be in line with llvm trunk. Sorry for the noise.

In D15686#325266, @rnk wrote:

I thought we already addressed this issue with @rjmccall and decided that, if the user intentionally declares extern "C" variables with an _Z prefix, then we know they are intentionally attempting to name some C++ global variable.

I'd rather just warn on all extern "C" declarations starting with _Z, and let users keep the pieces when things break.

In D15686#319725, @tra wrote:

A better description of the problem would help. PR itself is somewhat short on details.

Fixed tra's notes. All the fixes are local, logic of the patch is not changed.

Fixed tra's note.

Ping!

Thanks for the review!

OK -- I removed the check in Itanium ABI part, restored back the test and committed the patch.

Reid, thank you for the review!

Sorry for delayed reply. All your comments are fixed (it turned out that usage of DefaultCC instead of ToCC is the root of all evil; after I fixed this, all other problems went away).

Patch updated after fixing all (but one) of Reid's comments.

Reid, thanks for looking into this!

In D11297#235525, @rjmccall wrote:

Yes, please make it an error.

Fixed last note from John McCall.