In D56760#1368054, @erik.pilkington wrote:

So it seems like the GCC people want to keep __builtin_object_size static.

In D56723#1366529, @ilya-biryukov wrote:

I'll try playing around with your idea, my initial plan is to store preferred type alongside the current token as a member of the Parser class and update it when advancing to next token, when the parser backtracks and in the places where we care about propagating the types.

This seems to be introducing a requirement that enterUnknown is called on all paths through the parser where we recurse to parse a subexpression and don't have specific type information. That seems like an unfortunate requirement to me from a maintenance perspective; have you considered alternative approaches? For example, we could store the preferred type alongside the SourceLocation of the corresponding token (and propagate the information when we parse parentheses and any other completion-type-preserving construct), and only apply the information when the code completion token is at that location. That way, we only need to annotate cases where we know the preferred type, not when we don't.

Thanks, looks great!

@jwakely, we should come to a common understanding of the right behavior here :) (and then probably report back to SG-10).

[This discussion should probably move to cfe-dev@, since it's not directly relevant for this patch.]

Thanks, this seems like a useful feature to me.

I like @Quuxplusone's suggestions for more heuristics here, but I think they should probably live under a different warning flag (collectively grouped under -Wctad as @lebedev.ri suggested). Concretely, I think we could warn if, during template instantiation from a type-dependent initializer, we select the copy deduction candidate and there was another viable candidate (the optional o(x) case, the tuple t{args...}case, etc), maybe under a new -Wctad-unwrap or similar.

In D55500#1347493, @EricWF wrote:

@rsmith, what sorts of additional tests are needed?

If someone wants to pick this up and run with it, this patch is fine as a starting point (though will need to be rebased).

I don't like having a solution that doesn't work for C++ and can't reasonably be extended to a solution for C++ (we really don't want to be demangling the alias target here and trying to match it against the program, and ideally we don't want to delay the warning in question to codegen time), but this does reduce warning false-positives so it seems hard to object to. (We'll still need to tell people to mark declarations as __attribute__((used)) when they're used in this way if their names are mangled.)

LGTM (assuming the FreeBSD folks are OK with it).

Thanks, this looks great. A couple of the changes to the tests look like the diagnostic output is slightly worse in some error recovery conditions, but generally this is a nice improvement.

Looks good, thank you!

LGTM with s/ConstantExpr/FullExpr/.

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.

Personally I would go stronger: using CTAD on class templates that were designed for it (1) is dangerous and (2) may create source compatibility problems for future changes to that library!

I think the ideal solution would be something like:

Thanks, you've addressed my comments to my satisfaction. (Not approving as I've not done a detailed review of the implementation, only of the approach, but it looks like your review with @pcc is nearly complete.)

Thanks! Do you need someone to commit for you?

Looks good. Is there anything in www/cxx_status.html that we should update to track that we've implemented this part of P1353R0?

I'm concerned that this will hide real bugs by suppressing the bogus warnings in (only) the simple cases. There is no "read a value from memory" operation on type nullptr_t (just like for, say, class types), and any warning that believes there is is incorrect. For example, given

For the record: I'm OK with this direction. (I somewhat prefer removing the -enable-long-wordy-thing option and instead automatically disabling the zero option for clang release builds, but I'm OK with either approach.)

Thanks, this makes a lot of sense to me.

In D55395#1322244, @ABataev wrote:

This is wrong, the original implementation is correct and should not be changed.

Is this change blocked on something?

Thanks for your patience.

Thanks!

Thanks (and sorry for dropping the ball on this review).

In D54966#1317750, @hfinkel wrote:

In D54966#1317428, @rsmith wrote:

@chandlerc, @hfinkel: does an attribute-only implementation (with no constant evaluation enforcement) materially hurt the ability for the optimizer to use this annotation? Eg, in:

extern char k[16];
void f() {
  // the initializer of p is a constant expression and might get constant-folded to &k by the frontend
  char *p = std::assume_aligned<16>(&k);
  // ...do stuff...
}

the alignment assumption may well never be emitted as IR. Is that acceptable?

__attribute__((assume_aligned(N))) and __builtin_assume_aligned are, at the IR level, implemented in a very-similar way. For functions with that attribute on the return type, we essentially emit an alignment assumption on the return value at every call site (in CodeGenFunction::EmitCall). Thus, from the optimizer's perspective, I don't think that it makes a big difference.

In D51568#1314004, @andrewjcg wrote:

I don't think we need to change the serialization format for this: a serialized path beginning with / is already treated as absolute and any other path is already treated as relative, so we don't need a flag to carry that information.

But I think we need this since we now have two types of relative paths -- a CWD-relative path and a module-home-relative path -- and we use this flag to discern them for the AST reader. Previously, because cleanPathForOutput would always absolutify input paths, we didn't need this flag -- any relative path was relative the module home and all other paths were absolute.

Looks good with minor changes, thanks.

In D54966#1314038, @EricWF wrote:

We don't need to use __builtin_assume_aligned to implement this. We can use __attribute__((assume_aligned(N))) instead. That solves our constexpr problem.

I'm assuming this is a patch from a time traveller who has come back from 11 years in the future seeking to prevent some disaster, after this test case somehow becomes part of some critical infrastructure. Definitely don't want to stand in the way of that!

I don't think we need to change the serialization format for this: a serialized path beginning with / is already treated as absolute and any other path is already treated as relative, so we don't need a flag to carry that information.

In D54986#1312435, @lebedev.ri wrote:

Will -fforce-emit-vtables still work?

• Do not emit unused discardable vtables.

If CodeGen is able to emit inline key functions when the vtable is needed anyway, can we instead change DeclMustBeEmitted to not treat inline key functions specially? There is no reason we need to emit any symbols for: