In D34574#789539, @ahatanak wrote:

Is there any cases where we can turn the linker error into a compile time error?

Unlike with sized deallocation (which also requires a new stdlib entry point), it was intentional that this be enabled by default in C++17 mode. Implicit calls are only generated to the aligned forms of operator new and operator delete in cases where the program would otherwise be broken (due to requiring more alignment than the allocator would provide). A link error seems strictly preferable to silent misalignment at runtime. Plus, you have no way to tell whether the user is providing their own aligned allocation / deallocation functions, so disabling this by fiat for certain targets breaks legitimate use cases -- we need to at least respect the command-line -faligned-allocation flag and only use this target-based detection as a default / fallback.

The remaining missing pieces were ObjCInterfaceDecl::visible_categories and ObjCInterfaceDecl::visible_extensions. The new revision of the patch gets a bit further down the road of passing through real visibility information into those, but that approach seems to snowball out of control. For example, ASTContext::mergeTypes now needs visibility information in order to determine whether two ObjC object types are compatible (because this depends on whether the types have compatible sets of protocols, which in turn can vary depending on which categories are visible for those two types). I have not completed this patch: there are still a lot of places that an IsHiddenFunction would need to be plumbed through to make this work.

It'd be good to also add tests for the other traits you're changing.

Looks good, thanks!

We use the -Wc++NN-compat- prefix on all the other subwarnings of -Wc++NN-compat warnings (examples: -Wc++98-compat-bind-to-temporary-copy, -Wc++11-compat-reserved-user-defined-literal, ...). I'd prefer to include the -compat in the name for consistency; I also find it less clear what the warning flag would mean without it (I would expect -Wc++1z-mangling to only fire in C++1z mode, whereas this warning never fires in C++1z mode).

If the lookup table is expected to be computed from the lexical decls rather than provided via "external visible storage", this makes sense to me.

LGTM

LGTM as-is, but a couple of questions for possible further improvements.

Generally, all behavioural changes should have accompanying tests. The code change itself looks good, thanks.

LGTM

This is a libstdc++ bug: https://gcc.gnu.org/PR79162 (They implemented a speculative fix for a different issue and ended up breaking this case.)

Looks great, thanks!

Is there any way to add a test for this? Does it affect the behavior of clang or any in-tree clang-based tools?

LGTM

If you want to take this path, you should also add ActOnStartOfFunctionBody calls to the other parts of the parser that parse function definitions: Parser::ParseLateTemplatedFuncDef, Parser::ParseLexedObjCMethodDefs, and Parser::ParseLexedMethodDefs. You should also rename ActOnFinishFunctionBody to something like ActOnFinishFunctionDef to avoid confusion, because it pairs with ActOnStartOfFunctionDef not the new ActOnStartFunctionBody.

Looks good to me, thanks! Do you need someone to commit this for you?

Looks good, with a couple of tweaks (and corresponding test changes).

Rebased and added requested test.

In D33703#770896, @bruno wrote:

• I've noticed in the patch that on the ASTWriter side we serialize the introduced size / mtime, but there are no changes to the ASTReader, so I assume in the reader side you still need the module map around to fetch those infos, right?

Looks great, thanks!

My feeling is that there's not much value in adding an LLVM intrinsic for something that can be expressed directly in a handful of IR instructions; those instructions seem like the way to express this that would allow the most optimization potential, and the backend should be able to pattern match this to a BT instruction. Based on that, I'm reluctant to add clang builtins for this, partly because it looks like we'd need quite a lot of them and partly because (in the absence of an IR intrinsic) they'd just get lowered into the same instructions as the plain C code anyway.

In D33398#769152, @rnk wrote:

Isn't there a space in the mangling for vendor extensions? Can we use that here?

In D33616#768287, @hans wrote:

From looking in the Intel manual (Table 3-2, in 3.1.1.9 about Bit(BitBase,BitOffset)) it does sound like the bit offset can be negative *shudder*, so I suppose this is necessary and explains why the type is signed in the first place? Hopefully most of these will be inlined into a context where BitPos is constant or unsigned.

In D29877#766196, @EricWF wrote:

No. But I can point you to range-v3 which uses this pattern and I think the idiom is somewhat appealing, but that's orthogonal to Clang diagnosing it.

In D29877#765968, @EricWF wrote:

I think this patch still gets the following case wrong:

// foo.h
constexpr struct {
  template <class T> void operator()(T) {} // emits unused template warning
} foo;

In D33538#765146, @EricWF wrote:

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

That's correct. I was mistaken as to why this was needed. I mistook a bug in libc++ for the reason this was needed.
So I have no need for this patch anymore.

Do you still want to land this for the reasons you mentioned?

Thanks!

In D33538#765062, @rsmith wrote:

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

Oh wait, I see what's going on. You're not testing for whether coroutines is enabled, you're testing for whether the __builtin_coro_* builtins exist. Are we sufficiently confident that those aren't going to change that we're prepared to make libc++ rely on this? (If we change the signature of those builtins in the future, then new versions of clang would stop being able to build old versions of the libc++ module.)

In D33538#765045, @rsmith wrote:

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.

In D29951#750885, @v.g.vassilev wrote:

In order to create a reasonable test I need to use -error-on-deserialized-decl and I hit a bug: https://bugs.llvm.org/show_bug.cgi?id=32988

Richard, could you help me out with the test? Maybe we could trigger this avoiding the abovementioned bug...

I like it, and it seems like it would nicely generalize if there are more cases that we find need this treatment.

Counterproposal: in -std=*++14 onwards, treat this construct as a user-defined literal, but fall back on the built-in interpretation if name lookup doesn't find an operator""i function. (The two interpretations only conflict if the source code explicitly does something like using namespace std::complex_literals;, which seems like a pretty big clue that the user expected 1.0ito be std::complex<double> rather than _Complex double.)

LGTM