- User Since
- Jun 28 2016, 8:37 AM (238 w, 2 d)
Wed, Jan 20
Thu, Jan 14
Mon, Jan 11
Fix looks fine, I'm on the fence about how to handle the test, whether it is valuable to convert it to a C++ test and omit the #ifdef/extra run line, or leave it as it is (without the 2nd run with Wmost).
Thu, Jan 7
Wed, Jan 6
Tue, Jan 5
I'm not sure how well Attr.td's constraints are enforced on type attributes, as these often happen before parsing is completely done. I'd imagine this code was put into place at least the 1st time for good reason, but I'm curious as to why we wouldn't have tests that cover that (or, as you assert, it could simply be that this is simply dead code).
Mon, Jan 4
I note here: https://llvm.org/doxygen/namespacellvm_1_1Intrinsic.html#a7157f9fa9dd11f234ec3c58517cb6d96 that the documentatoin to the getName function variant says:
Dec 17 2020
Dec 15 2020
Since there seems to be at least a little renewed interest in this from a handful of people lately, I spend the time to rebase this and do some minor cleanup tasks.
From the CFE's perspective, this is all that should be needed. Is there a BPF code-owner who can test it and see if it is acceptable?
Dec 7 2020
From my perspective I think this is right... I don't feel comfortable approving this however until someone from OpenCL/OpenMP takes a look.
Nov 17 2020
Nov 5 2020
Nov 4 2020
Ping! This was requested in post-commit on the last patch, so I'm hoping it shouldn't be too controversial.
I went through all the comments here, plus looked at the code myself. I believe all of the comments by other reviewers have been fixed/answered acceptably. I don't have any additional comments to add, therefore I think it is appropriate to accept this revision.
Added test for +lambda as @rsmith requested.
Nov 3 2020
Made another attempt at fixing the comment. Wordsmithing welcomed/encouraged :)
update comment as @aaron.ballman requested.
Nov 2 2020
Oct 30 2020
Oct 29 2020
@rjmccall : Think I got everything, is this what you mean?
Oct 26 2020
Please make sure the commit message is significantly more descriptive here than what is on the review.
Oct 23 2020
Alright, I have the multi-emit dealt with, and the problems that come with that solved. This doesn't do the MSVC-emit-for-all-CCs thing, but I intend to do that in a separate patch with the same infrastructure.
Oct 22 2020
Turns out this patch: https://github.com/llvm/llvm-project/commit/2e1e0491b7098fcfe01945e8f62cafe1fcb3cf36 is my problem. The issue has to do deducing a 'local' type in the return type. As a result, we choose to mangle any type 'containing' auto as 'auto'.
Same here :)
Hmm... so I got distracted the last few days with a handful of small SEMA issues that I believe to be solved, so I'm going back to my codegen issues.
Oct 19 2020
Why did we select the 'bracket-depth' for this? The documentation on that doesn't seem to be anything close to what this should be based on:
Oct 16 2020
Perhaps a better example:
Oct 13 2020
Oct 12 2020
Oct 8 2020
*PING!* Any feedback?
Sep 29 2020
I'm not really a good person to review this, OMP isn't nearly my expertise.
Sep 25 2020
Sep 24 2020
Ok then, I'll take a look to see what it would take to make the MaterializeTemporaryExpr be created with the complete type here instead. Thanks!
Gah, forgot 'Differential Revision' again :/
Aug 26 2020
Aug 21 2020
This looks right to me.
Its a bit of a shame that the two implementations (left/right) differ by only a single character, an y ideas for combining them cleanly? I might also consider combining the two 'if' statements into a single one (if !EvalInteger(E->getArg(0)...) || !EvalInteger(E->getArg(1)...), but that is generally just preference.
Aug 19 2020
CFE changes look right to me. Please give the rest of the reviews a little while to take a look though
Aug 18 2020
Were you able to make any progress on this?
Aug 17 2020
1 comment, otherwise seems alright to me.
Aug 14 2020
Yep! Declaring a global variable that isn't 'extern' with an incomplete type is disallowed anyway, so if you call RequireCompleteType, you're likely just diagnosing that early.
I did a little debugging, and the problem is the template itself isn't a complete type until you require it with RequireCompleteType. You have this same problem with incomplete types: https://godbolt.org/z/hvf3M4
Also, see this bug: This crashes immediately when used on a template instantiation: https://bugs.llvm.org/show_bug.cgi?id=47169