- User Since
- Jun 28 2016, 8:37 AM (220 w, 5 d)
Wed, Aug 26
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
Aug 12 2020
Aug 10 2020
Aug 7 2020
Aug 5 2020
Aug 4 2020
Aug 3 2020
Jul 31 2020
Jul 30 2020
Thanks! I'm done for the day, so I'll push in the morning when I'll have a few hours to respond to build bots.
Jul 29 2020
Jul 27 2020
This seems like a strict improvement as well. That said, there are two opportunities to clean up the code that I suggest evaluating.
I looked through the code changes, and they all seem quite mechanical. I believe they are all correct.
Based on richard's suggestions, that seems about right. However I believe all the asserts should have a message to go along with them as we typicaly do.
Ping! I believe I've done all of the requested changes.
Jul 24 2020
I wasn't able to add you to the bug report, but I discovered this: https://bugs.llvm.org/show_bug.cgi?id=46837
Jul 23 2020
Fix all the things that @rsmith suggested.
Jul 22 2020
Thanks for the review! I'll get this updated in the morning. I DO have a question on your suggestion for the ToVisit/Visited example, so if you could explain a little better, I'd be grateful.
Fix the comment as I mentioned, otherwise this LGTM.
As CWG seems unmoved by my argument as to why CWG2303 should change, I've opted to implement it as worded. As you can see, the implementation is somewhat more complicated, but I believe I made it at least reasonably understandable.
Ive got a bit a of a problem here: This flag (suggest-override) on GCC 8.3 (and others) warns on missing-override EVEN WHEN the function is marked 'final'. This was not fixed until GCC 9.2: https://godbolt.org/z/55KeM3
Jul 21 2020
I'd attempted to regenerate the cxx_dr_status.html using make_cxx_dr_status, but I get a 'no such
Revert change to cxx_dr_status and update the test to have the right tag.
Also make sure to update the the cxx_dr_status.html document.
Jul 20 2020
Jul 17 2020
Woops, left in a TODO from when I was planning on how to do this patch. Should be ready now :)
I added @rsmith directly, since I suspect he's the most knowledgeable in SemaTemplateDeduction.cpp, but if anyone knows of someone else who is a good fit for this review, please add them as well!
This patch doesn't seem to build for me:
/iusers/ekeane1/workspaces/llvm-project/llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp: In function ‘llvm::Error llvm::exegesis::parseDataBuffer(const char*, size_t, const void*, const void*, llvm::SmallVector<long int, 4>*)’:
/iusers/ekeane1/workspaces/llvm-project/llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:99:37: error: ‘struct perf_branch_entry’ has no member named ‘cycles’
Jul 14 2020
Jul 13 2020
What about powers-of-2 larger than, say, 256 bit? So a 512 bit value. Does this give reasonable codegen?
Jul 9 2020
Please fix the clang-format concerns, otherwise LGTM.
Gah, misspelled differential revision.
Jul 8 2020
Jul 7 2020
Jul 6 2020
Jul 2 2020
I just took a look at SemaChecking, the rest I'm not sure I get the intent of the patch sufficiently to understand.
Jul 1 2020
Jun 30 2020
As @craig.topper suggested, extract the four lookups into 2 bool instead.
Jun 29 2020
Do all feedback as @craig.topper recommended.
A bunch of clang-format, plus 1 odd looking equation, otherwise this like good.
Jun 25 2020
A few small nits, but I don't sufficiently care enough about the rule, so long as there IS one (hence my reason for opening this can of worms). I added @lattner since he requested that he be added to changes to the coding standards in the past.
Jun 24 2020
I talked to others about the test, and they suggested the correct way to validate this is to make it simply a 'doesn't crash' test, so I'll remove the filecheck/check lines and submit this. The comment there seems sufficient to describe, so I'll live it in place.
1 nit, plus the clang-tidy suggestions.
Jun 23 2020
1 suggestion to the test, but otherwise looks OK. Let me know if you'd like this committed for you.
@LukeZhuang : This patch causes the buildbots to fail, as O1 means something slightly different with the new pass manager :
Jun 22 2020
I'll get this fixed, thanks both of you for letting me know.
The test needs work (check/check-not lines+ filecheck), otherwise I think this should be alright, particularly if no one else has commented in a while. I'd like to have you upload an updated test validating what you think should be happening before approving though.
Jun 19 2020
Interesting patch! I'm definitely interested in seeing the numbers. Though note this patch was made as a followup, which seems to have gotten about 1/2 of the original regression back: https://github.com/llvm/llvm-project/commit/837ca4796065ccd0ac0d20860341ac06a9645009
Jun 18 2020
Jun 16 2020
Note that this causes a regression (reported here) https://bugs.llvm.org/show_bug.cgi?id=46336