- User Since
- Jul 12 2012, 2:19 PM (331 w, 6 d)
Tue, Nov 20
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.
Mon, Nov 19
Looks good other than the ODR violation issue.
Wed, Nov 14
Tue, Nov 13
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").
Mon, Nov 12
Thank you, this is looking really good.
Sat, Nov 10
Thu, Nov 8
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.
Fri, Nov 2
Thu, Nov 1
Wed, Oct 31
That is clearly not what clang is doing here.
Looks good once some tests are added.
Tue, Oct 30
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.)
Mon, Oct 29
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.
Fri, Oct 26
This needs a test.
Thu, Oct 25
Also please mark PR39428 as fixed once this is submitted :)
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.)
Wed, Oct 24
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?
Tue, Oct 23
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.
Oct 22 2018
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.
Oct 20 2018
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.
Oct 19 2018
(Looks fine with a suitably-adjusted comment.)