- User Since
- Jul 12 2012, 2:19 PM (421 w, 6 d)
- Remove development aid from test.
Mon, Aug 10
Only relatively trivial comments; please feel free to commit once they're addressed.
Please add a corresponding test case. Other than that this looks good. Thanks!
Sun, Aug 9
Did this only crash during error recovery before, or also for your void g() example? If we were only crashing in error recovery, that would suggest that error recovery was producing a bad AST, and perhaps we should be fixing this elsewhere.
Fri, Aug 7
Thu, Aug 6
Looks reasonable to me. I expect you'll find more places that need to learn how to handle dependent types in C, but this looks like a solid start.
The diagnostic we get for the case of three or more comparison operators here doesn't seem ideal. Perhaps we could do this check as part of the SemaChecking pass over the completed expression rather than doing it as we form the individual comparisons? That way we'll have the contextual information necessary to find the complete chained comparison; we'd also be able to detect the special case where the operator sequence is the operand of an &/|/^ so that we need parentheses in the fixit.
Wed, Aug 5
Two observations that are new to me in this review:
Tue, Aug 4
Thanks. I'd like @rjmccall to approve too, but the design of these intrinsics and the Sema and constant evaluation parts seem fine. (We don't strictly need constant evaluation to abort on library UB, so I think not catching the misalignment case is OK.)
Unbreak test that was autobroken by lint.
This is not ideal, since it makes the
calculations char size dependent, and breaks for sizes that
are not a multiple of the char size.
Fri, Jul 31
Thu, Jul 30
Thanks, looks nice =)
We already check that explicit is only used in the proper scopes in ActOnFunctionDeclarator (and the proper scopes aren't just class scopes, due to deduction guides). In fact, the assert appears to have nothing to do with whether the expression appears at function scope; that's a red herring caused by unary && (address of label) doing different things at that scope. An analogous case crashes at class scope too:
Wed, Jul 29
Tue, Jul 28
Looks good with suggestions applied, and with the portability problems in the test fixed. (Maybe just add a -triple? Though it would be good to also test inalloca and ARM constructor this returns and so on.)
Mon, Jul 27
This is at best only a partial fix. Sema::NC_ContextIndependentExpr is supposed to be used (unsurprisingly) only if we form a context-independent annotation, but here we're forming a context-dependent expression that depends on whether it appears in an unevaluated context. I think the better approach would be to fix the case in Sema::ClassifyName that violates context-independence instead (there's a FIXME there for this issue).
Thu, Jul 23
These new builtins should ideally support constant evaluation if possible.
Wed, Jul 22
Tue, Jul 21
I'd like to see some testcases for the C++ side of this. The following things all seem like they might be interesting: passing and returning classes and unions, especially ones that aren't trivially-copyable, nullptr_t, pointers to members, this parameters, VTTs, the "complete object" flag for MS ABI constructors, the this return from constructors in the ARM ABI, parameters and return values of virtual adjustment thunks.
Fri, Jul 17
Thu, Jul 16
Wed, Jul 15
Tue, Jul 14
Are we at a point where we can test this now? Perhaps by adding an assert in codegen that we always have an evaluated value for any constexpr variable that we emit?
Mon, Jul 13
Would it make sense to split this into a separate utility, so you could use (eg)
Jul 12 2020
Hm, I think this is not quite right. For example, given:
Jul 9 2020
Jul 8 2020
Jul 7 2020
@aaron.ballman We will need to do something like this in general, but I'm not sure it's going to be as easy as just inheriting the attribute in the general case. What do you think?