- User Since
- May 26 2015, 5:42 PM (138 w, 1 d)
Sat, Jan 13
Thanks! This is quite nice.
Fri, Jan 12
The diagnostic text looks to me, but I do have a comment about the nested 'if' inline.
LGTM. Thanks for fixing this.
Thanks for adding these! This looks good to me. Do you have commit access, or do you need someone to commit this?
Looks great. It is nice to have this fixed and cleaned up!
LGTM as well.
This looks good to me, as well.
LGTM with the TODO and the test case I requested inline.
Tue, Jan 9
Mon, Jan 8
This is great!
LGTM as well.
Looks good to me with some minor nits inside (and also a request to consider factoring out common code).
Yes, this looks great!
LGTM as well.
At a high level it seems pretty weird that the relevant lines for a diagnostic is represented a path diagnostic piece. This information is not a piece of the path; rather it is metadata about the entire path. It seems to me like this would be better represented as part of the path diagnostic itself and not pieces.
Thu, Dec 21
LGTM. The tests are great!!
Wed, Dec 20
Dec 18 2017
Looks good to me, although I have a super nit about wording in a comment.
Dec 17 2017
Looks good to me.
Dec 14 2017
Some comments on the C++ inline.
Dec 13 2017
This looks good to me for a quick fix that we plan to address in a more principled fashion later.
This is seems like a very useful visualization, *especially* for loops. Can we this patch get it into a state where it can be committed and used for debugging purposes?
Dec 11 2017
Thanks for looking into this!
Dec 4 2017
LGTM. Ship it!
Minor nits inline. Other than that, looks great to me.
Dec 3 2017
Dec 1 2017
Nov 30 2017
Nov 29 2017
Thanks, this looks good to me!
Nov 28 2017
Thanks for tackling this! There is one major comment inline (you are generating an extra node that you shouldn't, which is causing the analyzer to explore code it shouldn't) and a suggestion for simpler diagnostic text.
Nov 27 2017
LGTM, but as noted inline you should update the new llvm_unreachable() to have a more descriptive error message.
Nov 25 2017
Nov 24 2017
I don't think LLVM/Clang is a good project to evaluate this on, since it has very little multi-threaded code.
Nov 21 2017
For clang itself I think we also use a stage-2 clang to build the same version of clang and make sure that it matches the stage-2 clang. This doesn't help for the analyzer though.
Nov 20 2017
Thanks! Do you have commit access, or do you need someone to commit this for you?
Nov 18 2017
Thanks for finding and fixing this!
Can you give examples of the kinds of bugs that this check is designed to find? I'm worried that "writing a variable in one critical section and then reading it in another" is too general of a pattern to warn on and will result in a lot of false positives.
Nov 17 2017
LGTM! Thanks Anna!
Nov 13 2017
Nov 10 2017
Update to address Gabor's feedback and adopt "option 3". This changes the casting logic to always forget the specialized type arguments on an explicit cast regardless of whether it is an upcast/downcast or a cross cast.
I suppose we could move the logic that constructs pointers to members for fields here (right now that logic is in ExprEngine::VisitUnaryOperator()'s handling of '&'). However, since the AST's DeclRefExpr for the field is marked as an lvalue this would be slightly odd -- we would have the value of an lvalue expression be a non-Loc.
Nov 9 2017
Yes, looks great. Thanks!
This seems reasonable to me. Please commit it. @NoQ can do a post-commit review and fix it up if he would rather address the issue differently.
This looks good to me. It is very clean! But please add a comment in the places where you are assuming a two's complement value representation for signed integers.
Nov 8 2017
I believe your motivating examples used errno_t, which is a signed type.
Nov 7 2017
Interesting bug! The workaround looks good to me.
Nov 6 2017
@xazax.hun Would you be willing to take a look?
Yeah, I'm not a fan of this style of testing for path diagnostics, either.
Looks good to me.
Nov 2 2017
Another round of comments inline. With those addressed it looks good to me -- but you should wait on Artem's go-ahead before committing.
LGTM. It is a nice cleanup.
Looks great. Thank you!
I believe that the intent of __builtin_debugtrap() is to provide an in-source mechanism so that the developer can trap into the debugger and then continue execution. For example, in the Swift codebase this is used in combination with a debug flag to break into the debugger at the beginning a specified pass. https://github.com/apple/swift/blob/master/lib/SILOptimizer/PassManager/PassManager.cpp#L339
Nov 1 2017
Personally, I don't think this is worth it and I find it unpleasant to add untestable code -- especially if that code is going to stick around in release builds.
Oct 31 2017
I think this is a great idea, and I expect it to find some nasty bugs!
Looks good to me with the comments by Gabor addressed.
Oct 27 2017
Thanks Gabor. Looks great to me!
Oct 25 2017
I agree with Alexander here. The LLVM naming convention is not going to change and we should err on the side of using descriptive names. How about "FunctionBodyFarm"?
I think it would be better to add a new "test/Analysis/block-in-critical-section.m" file rather than enabling a random alpha checker in a file that tests the analyzer core.
Oct 24 2017
Oct 23 2017
That would require going into the past and requiring everyone back then to run clang-format as well. Unfortunately they didn't -- so human judgment is needed when modifying code that doesn't obey the guidelines.
I think a good strategy is to look at your diffs and consider whether the benefits of normalizing the style outweigh the cost of losing the history. In this case, I think keeping the history makes sense. (Imagine you are a future maintainer and want to know when and why a new option was added. Is is very helpful to use git annotate to understand why the code is the way it is.)
Content-wise, LGTM. There is a style nit inline.
Oct 20 2017
Some nits inline, but looks good to me!
Thanks for fixing this!
What is the workflow where this is needed? Is it when an end-user is running a build with assertions and can't provide a reproducer but can provide the console output?
Oct 17 2017
Looks good to me.
Oct 13 2017
Looks good to me!