- User Since
- Jun 28 2016, 8:37 AM (313 w, 1 d)
All but the 1 comment from @ChuanqiXu fixed, not sure what to do about the 'info'.
Is there any chance you can validate this against https://reviews.llvm.org/D126907 as well? We touch similar code, and I'm intending to get that committed in the near future. Otherwise, after a quick look, I don't see anything particualrly bad, other than a lack of release notes and update of www-status.
All tests pass now, I was able to get the template-template checks working correctly, and it passes all the tests I have available. @ChuanqiXu if you could review/run what tests you have, it would be greatly appreciated.
So I don't think this is the right fix for this... But I DID check and see that the test case seems to be fixed here: https://reviews.llvm.org/D126907 , which does a lot of rework for this sort of thing.
Mon, Jun 27
I'm generally OK with this. BUT Aaron/JF/Tom should review it.
Wed, Jun 22
Ah, i see! Thanks for the explanation!
Tue, Jun 21
Note that the failure comes down to:
As promised, got it down to the 1 failure, and added tests for @ChuanqiXu and the std::vector example. That all seems to work, just a problem with the CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp test left.
Fri, Jun 17
Made good progress today, but got trapped down the CheckDeducedArgs path for quite a bit longer with partial specializations. I have all of the crashes I know of 'fixed', but have a few tests that still fail for unknown reasons. Because of that, i don't have anything I can really upload, so I'll have to do so mid-next-week.
Thu, Jun 16
I probably need to spend more time on this at one point, but first look seemed fine to me. I think the approach is about right, and the solution is there.
Wed, Jun 15
I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.
Tue, Jun 14
Did another quick read through and made a handful of 'nit' level suggestions. Since much of it is my words, I'd love it if someone else could take a read through as a double-check.
Mon, Jun 13
The organization and content of the document are quite good, I learned quite a lot, so thank you! I found the prose itself a bit difficult to read, so did my best to reword in each section as I could. Feel free to take/modify/review whatever you'd like of it.
The command line on one of the tests is a little strange to me, and I don't see the need for the -std=gnu11 on either, but the test content itself looks fine enough to me.
Fri, Jun 10
So from what I can tell, the problem is evaluating Constraint on initiate, and trying to find the 'right' argument from the MultiLevelTemplateArgumentList for RawCompletionToken. The problem is that the RawCompletionToken, as a template parameter, gets 'filled in to' the Concept relative to initiate instead of relative to the "root".
I'm not quite understanding this yet, so I'll have to take another look early next week. However, I AM intending to get https://reviews.llvm.org/D126907 committed in the next week or so. Could you perhaps see how it interacts with that? Its a sizable, multi-month project that I'd like to make sure doesn't get stuck in a rebase-loop again.
Thu, Jun 9
I was able to reduce it today. BUT didn't get a chance to debug:
Wed, Jun 8
LGTM! Thanks for this!
All of the comments from @ChuanqiXu done, thank you so much!
Tue, Jun 7
@ChuanqiXu : i was hoping you could take a look at this, since you did such a great job reviewing the rest of this (note this is mostly the same patch as the last one, just with the 'friends' stuff dealt with).
Fri, Jun 3
This mostly/all seems reasonable to me, and the diagnostics, while not perfect, are IMO a vast improvement.
Thu, Jun 2
Wed, Jun 1
Note this was mentioned as a 'must also do' in my discussion on deferred constraints with @rsmith, however as this already fails, I don't see this as a prerequisite. However, I think this is easy enough to just do.
Tue, May 31
I think I'm Ok with this as a 1st step for the cleanup. I think we should probably evaluate what amount of work is necessary to extract the ExtInfo out into trailing storage, depending on whether it saves us room in the FunctionProtoType and FunctionNoProtoType types.
I agree with the above, we probably need an RFC on discourse for this.
This seems innocuous enough, and seems to better reflect the intent of the standard, so LGTM.
I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here. Also, the 'hasBitfields' seems like the answer should just be 'no' in the case when its 'no'...
I'd prefer we limit the scope of this to JUST a 'do-while' loop. I think we can clarify the "if an inner statement uses braces, the outer one should too", AND special-case the do-while loop as needing braces.
May 24 2022
I have a change to the release note that I'd like to see improved in SOME way, but I trust you to correct/reword as you wish. I'm still not particularly happy with the mechanism of the test, but I cannot come up with a way to cause the Semantic Analyzer to cause this.
May 23 2022
How much of P0848 is missing after this? If nothing/not much, should we update cxx_status.html as well?
Also needs some release notes.
May 20 2022
May 19 2022
May 18 2022
Took me a bit to get the prose, but I think I got it now. PERHAPS there is use to 'finish the thought' on the outcome there? Either way, LGTM.
May 17 2022
This likely also needs a Release Note.
I think I agree with the justification here, though am a touch confused in the test. I'm also a touch concerned that we have getSubobjectSizeInBits returning a 'rounded up to power of 2' bit happening here. The bitfield case returns non-powers-of-two, but the _BitInt case does not.