Looks good, thanks! (Though we generally prefer fewer test files with more test per file, because a lot of the overhead of running tests is the setup / teardown time, so can you merge the two added tests into a single file?)

In the past, we've been resistant to adding more fine-grained compat warnings, because we don't want to encourage subsetting the language (which sounds like exactly what you're trying to do here). We generally don't think it's Clang's business to enforce coding style conventions (such as "don't use CTAD because it makes your code less readable"), but we do consider it to be in-scope for Clang to warn on constructs that are error-prone or that have a negative impact on portability or compatibility, and so on. On that basis, I think there is a case to be made for warning on this specific language feature, because using CTAD on class templates that weren't designed for it is dangerous and creates source compatibility problems for future changes to that library.

Thanks! Some simplifications are possible here, but otherwise this looks good.

Looks good other than the ODR violation issue.

Thanks!

In D53770#1297646, @greened wrote:

Oops, I didn't realize this hadn't been formally accepted yet. Still learning the Phab process. Let me know if you want it reverted for a formal accept.

In D54356#1297522, @void wrote:

Okay. I'll revert this then.

In D54356#1297506, @void wrote:

This code is called in over 90 places, so it's hard to tell if they all are in a constant context. Though I suppose that what this code is supposed to check for would work the same in a constant context as without one. I can revert this if you want, but to be honest the original function was terrible--it's huge and hard to understand what's going on. As for adding new expressions, this isn't the only place where a StmtVisitor is used. One still needs to go through all of those visitors to see if they need to add their expression.

In D54356#1297470, @rsmith wrote:

Can you explain more about the justification for this? The code today has a covered switch, which is useful for maintainability -- anyone adding a new Expr node gets told they need to think about and update this code. Are there any cases where we check for an ICE and aren't in a constant context? I would have expected that the fact we're asking implies that we are in a constant context (at least when the answer is "yes").

Can you explain more about the justification for this? The code today has a covered switch, which is useful for maintainability -- anyone adding a new Expr node gets told they need to think about and update this code. Are there any cases where we check for an ICE and aren't in a constant context? I would have expected that the fact we're asking implies that we are in a constant context (at least when the answer is "yes").

Thank you, this is looking really good.

I like the direction here, and I'd like to see this applied generally: a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

Let's do it. The diagnostic and fix-it is awesome ;)

Thanks, other than the compound literal part I think this is all fine. I'm happy for you to go ahead with this as-is; we can rearrange how we handle compound literals as a later change if you prefer.

In D53985#1284282, @aaron.ballman wrote:

In D53985#1284246, @rnk wrote:

In D53985#1284196, @ldionne wrote:

Would it make sense to add the GNU spelling to the attribute in Clang?

I'll redirect that question to @rsmith and @aaron.ballman. I have a vague recollection that it was considered and rejected, but that decision may have predated GCC 7.

I'm not opposed to it given that it's an attribute supported by GCC.

In D53787#1282995, @mcgrathr wrote:

In D53787#1282975, @rsmith wrote:

Other symbols must have exactly one definition (modulo the permission for duplicate identical definitions for some cases), but these ones have a default definition that is designed to be overridable by a different definition appearing anywhere in the program.

I don't understand this claim. These are symbols like any others at link time. A single definition must be supplied as for any other function.

In D53787#1282930, @mcgrathr wrote:

In D53787#1279899, @rsmith wrote:

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

I don't really understand how these functions are different from other functions. The language standards don't have anything to say about ELF visibility. What you say about "whole-program operation" is true of any global symbol.

In D53949#1282884, @regehr wrote:

The C99 standard (6.5.3.1.2) appears to be very clear on this point: "The expression ++E is equivalent to (E+=1)."

That is clearly not what clang is doing here.

Looks good once some tests are added.

In D53860#1280959, @twoh wrote:

For me it seems that the bug is checking destructor accessibility of Base itself, not fields of Base.

Thanks, looks good. I don't mind if you address the comments below before this commit or in a separate commit.

Looks good with an accompanying test. (You can probably copy whatever was added for the FreeScale line above.)

In D53787#1279913, @phosek wrote:

In D53787#1279899, @rsmith wrote:

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

If you really want to do this weird thing, and you understand what you're getting yourself into, I don't see a problem with having a dedicated flag for it, but don't break all existing users of -fvisibility=.

That sounds reasonable, do you have any suggestion for the name of such flag?

Replacing the global new and delete is supposed to be a whole-program operation (you only get one global allocator). Otherwise you couldn't allocate memory in one DSO and deallocate it in another. (And nor could inline functions etc.)

Thanks, LG. I bet there's a bunch more places we could change to call these two.

This needs a test.

In D53717#1276245, @rsmith wrote:

Do you have evidence that this complexity is worthwhile? (I wouldn't imagine we have very many ForStmts per translation unit, so saving 16 bytes for each of them seems unlikely to matter.)

In D53595#1276346, @Rakete1111 wrote:

In D53595#1276330, @rsmith wrote:

In D53595#1273848, @Rakete1111 wrote:

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

Please regenerate the file without your change and check that in as a separate commit prior to this.

Why not afterwards? That way the DR is marked as resolved immediately.

Also please mark PR39428 as fixed once this is submitted :)

In D53595#1273848, @Rakete1111 wrote:

Addresed review comments :)

I updated the dr status file but a lot of unrelated changes made it in. Is this okay?

In D53207#1276062, @ubsan wrote:

I don't actually know how to send this upstream, since it's been accepted... How should I proceed?

Do you have evidence that this complexity is worthwhile? (I wouldn't imagine we have very many ForStmts per translation unit, so saving 16 bytes for each of them seems unlikely to matter.)

In D53670#1275677, @ldionne wrote:

In D53670#1275188, @rsmith wrote:

What's the problem with the state of affairs prior to this change? If some compiler / environment is defining a __FLT16_MANT_DIG__ macro but not providing a _Float16 type, isn't that just a bug in that environment?

There's also the fact that e.g. __fp16 doesn't work with the stream's operator>> and similar problems. Because of this, we're effectively lying about the fact that __fp16 is a floating point type. This was Marshall's point, and I agree with it.

@EricWF I do agree that if it quacks like a floating point and it walks like a floating point, then it is a floating point. However, __fp16 doesn't seem to quack quite right, so it's not a floating point (at least not without additional work).

What's the problem with the state of affairs prior to this change? If some compiler / environment is defining a __FLT16_MANT_DIG__ macro but not providing a _Float16 type, isn't that just a bug in that environment?

Thanks! Can you also write something for the release notes (docs/ReleaseNotes.rst) describing the ABI change?

In D48896#1274919, @rsmith wrote:

In D48896#1274779, @ldionne wrote:

When merging a multi-container into a unique-container (e.g. map.merge(multimap) or set.merge(multiset)), I think we could do the following optimization: Once we've inserted an element from the multimap into the map, we could skip all the following equivalent elements in the multimap without having to perform a search in the map each time (since we know we won't insert the element anyway).

We can only do that if the comparators divide elements into the same equivalence classes, which we cannot know in general. (We can do this optimization if we can prove the comparator is a pure stateless function, but we probably can't detect that -- even an empty class type could store its state outside the class.) However, if the source and destination have the same comparator, and that comparator is std::less<T>, then I think it's correct to do this optimization.

In D48896#1274779, @ldionne wrote:

When merging a multi-container into a unique-container (e.g. map.merge(multimap) or set.merge(multiset)), I think we could do the following optimization: Once we've inserted an element from the multimap into the map, we could skip all the following equivalent elements in the multimap without having to perform a search in the map each time (since we know we won't insert the element anyway).

In D53207#1273727, @ubsan wrote:

when I don't specifically don't set the type trait kind of __alignof to UETT_PreferredAlignOf, and remove the code in AlignOfType to differentiate between PreferredAlignOf and AlignOf, all the tests pass again.

Looks good.

In D53207#1268535, @ubsan wrote:

I'm having real difficulty with this - I get really odd errors in seemingly unrelated tests when I change things.

For what it's worth, I think the language rule here is wrong, and we should instead be injecting the simple-captures into the lambda's function scope: http://lists.isocpp.org/core/2018/10/5145.php
But this appears to be a correct implementation of the rule as written, so let's go with it for now.

Just some minor nits.

I'm very happy with reducing our configuration space like this, if the distinction is indeed unused (which it appears to be). (If compiling in ObjC1 mode actually is useful, we should instead have a command-line flag for that and tests; the fact that we don't tells us almost everything we need to know here.)

Thanks, this is looking good.

Looks fine as far as it goes, but we're going to need to change all the places that skip past ExprWithCleanups to also step over this node. Since both nodes represent the boundary of a particular kind of full-expression, it'd make sense to me for them to at least have a common base class for such "skipping" purposes.

(Looks fine with a suitably-adjusted comment.)