- User Since
- Jun 28 2016, 8:37 AM (259 w, 6 d)
Thu, Jun 17
Yep, that works, still approved.
Wed, Jun 16
Two small changes to the test that you can do as a part of committing. Thanks!
This seems to do a lot of work that I think already exists, I believe I've seen that SPIRV targets already prohibit long-double, so doing another implementation specifically for x86 seems like the wrong approach. I'd suggest seeing if the SPIRV version works for these. If it doesn't, I'd prefer you generalize that implementation.
Tue, Jun 15
Mon, Jun 14
Just a couple of nits here, basically see how much we can put in the 'cheap to check' branch.
Wed, Jun 2
Tue, Jun 1
Ah! Thats what I missed!
The patch looks fine to me, though I don't particularly get the test changes that move a diagnostic by 1 line?
Thu, May 27
Wed, May 26
Apply clang-format patch, except for the changes to IdentifierTable.
Ok, this should get me up to date! Fixed a bunch of the comments referring to 'expression', added LangOpts::isSYCL, and changed it to DiscriminatorOverride.
Woops! Last update was JUST the changes, and I forgot to squash. Here is the whole patch.
Replace the DeviceLambdaManglingNumber mechanism with the callback mechanism.
Remove 'expression' form per suggestion. Still need to do the device-mangling-number removal/rewrite business.
Fix comments from aaron.
I like this, since the entire CFE does a ton of recursion on purpose for all AST related things. Please give others time to disagree however :)
Tue, May 25
Apply the clang-format patch from CI, also apply the unused variable fix clang-tidy suggested. The rest of the issues in clang-tidy are identifying functions in a recursive call-chain, but they are all part of the various visitors, so I don't think I have a way to fix those.
No real feedback, however I note that the bug link you have is one that was resolved in 2009, so I suspect it is completely unrelated.
May 21 2021
May 12 2021
Moved IfStmt/discarded case logic to ExprEvaluatorBase per @rsmith 's suggestion. This is the 'highest' in the tree that has a Sema/ASTContext reference, so anything 'higher' in the tree would be higher touch. @rsmith: WDYT?
May 11 2021
LGTM, but want others to look first.
Apr 30 2021
Apr 27 2021
Apr 23 2021
I have one 'nit of preference', otherwise I think I don't want to +1 this without giving people a few days to take a look. Based on my looks through the surrounding code, this _LOOKS_ right enough as far as I can tell, but I still want to give others a few days.
I guess my point is: a better comment would have saved me some time. Basically point out that the 'debug' info for the whole type is emitted with a virtual method, and that non-virtual types have it emitted in every TU. Also that this causes it to be emitted in only 1 place, since now there is only a single virtual method definition in a single TU.
Apr 20 2021
Apr 14 2021
Apr 7 2021
Alright, validating it now, then I'll push.
Hrmph... Phab ate my other comment, which was that the name EmitCastBetweenScalarTypes feels clunky. Does EmitScalarCast or EmitScalarScalarCast sound better and capture the meaning correctly?
Apr 5 2021
It is really sad that the attributes can't 'auto transform' themselves. ParsedAttr could (since it has a union of expressions/identifiers), but we don't really seem to have a good way to do it for Attr.
Mar 26 2021
Mar 24 2021
So I guess I don't understand what this does differently than stripPointerCastsAndAliases? Is limiting this to local aliases that important?
Mar 11 2021
The description of the problem, and hte patch and tests seem to make sense, so I think this should be ok. Please give some of the others a little bit of time before committing to speak their final bit, but LGTM .
Mar 8 2021
Mar 5 2021
Mar 1 2021
Feb 23 2021
Jan 29 2021
For target-specific builtins I ended up implementing this: https://github.com/llvm/llvm-project/commit/81a73fde5cea304d31294fd26c2f051f1685e97c at one point to make sure that 'host' builtins get looked up in the aux-target if necessary.
Can you show any tests where this builtin would be a part of the aux-triple, and not in active code? So an nvptx triple, x86_64 aux-triple, and in non-device code? (such as in a CUDA app)?
Jan 20 2021
Jan 14 2021
Jan 11 2021
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).
Jan 7 2021
Jan 6 2021
Jan 5 2021
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).
Jan 4 2021
I note here: https://llvm.org/doxygen/namespacellvm_1_1Intrinsic.html#a7157f9fa9dd11f234ec3c58517cb6d96 that the documentatoin to the getName function variant says: