- User Since
- Sep 17 2012, 3:16 AM (548 w, 6 d)
Wed, Mar 22
While I am OK with this approach, I wonder if it would be too much work to split the HTML generation and emitting data up. E.g., the logger could emit YAML or JSON that could be parsed by a python script to create the HTML (or the HTML itself could use JS to parse it and generate the DOM).
This would have a couple of advantages like:
- People could add additional visualizations or other tools like querying program states more easily just consuming the JSON/YAML file
- Other static analysis tools could produce similar YAML/JSON so your visualization could potentially work for other analysis tools. E.g., I think people were working on a dataflow analysis framework for MLIR.
Tue, Mar 21
I wonder if we should also open tickets on GitHub to reduce the chance of forgetting addressing the root cause for these. What do you think?
This fix looks good for me for this particular problem, but I wonder whether the solution is general enough. In case the analysis figures out that a call would not return (e.g., the value method is called on a provably empty optional, and it would throw instead of returning), would this approach still work? Would the analysis update the BlockReachable bitvector on demand?
Mon, Mar 13
Do you plan to selectively enable warnings coming from the STL to catch misuses of certain STL types?
Tue, Feb 28
Feb 22 2023
I think at this point it is ok to merge. Any other comments can be addressed in follow-up commits.
Feb 21 2023
Feb 20 2023
Overall, I like the structure of this patch and the references back to the standard. But every time we compare type pointers, they might compare inequal when one of the types have sugar on it while the other does not. Please review all of those comparisons to see where do we need to get the canonical types instead, and add some tests with type aliases.
Feb 15 2023
I am ok with committing this to unblock people hitting this assert, but at the same time I wonder if we want to open a ticket on GitHub that we might want to rethink how some of this works.
Feb 13 2023
Feb 10 2023
Feb 7 2023
Thanks! This is exactly what I had in mind! Let's wait for @NoQ just in case he had something different in mind.
Feb 6 2023
I think this patch should also add a test that fails before your changes.
Feb 1 2023
Jan 31 2023
I hope we will be able to get rid of this SkipPast thing at some point by looking at the value categories of the AST instead.
This change looks good to me. I wonder, however, whether the behavior should be parameterized in the future. E.g., whether the user of the analysis should be able to make a decision whether the analysis should be pessimistic or optimistic about unmodeled values.
Jan 24 2023
Overall looks good to me, I wonder if the tests could be less manual though. E.g., instead of asserting true/false, checking if the assignment would compile. This way we can be sure that the method in ASTContext matches the behavior of the compiler (and we get notified when the two diverge). If we could extract the corresponding code from Sema, it would be even better, but I do not insist as that might be a lot of work depending on how it interacts with other conversions.
Since now the patch touches Clang proper, adding one more reviewer.
Jan 16 2023
Also, could you open a bug report about the strange exception behavior on GitHub? Hopefully someone working on conformance can take a look.
Jan 13 2023
Jan 10 2023
Jan 9 2023
I have one nit that you should look into, otherwise sounds reasonable and looks good to me.
Jan 8 2023
Here is the gist of one *new* TP:
Jan 6 2023
I think in these cases you can recommit without waiting for a new round of reviews, but I hit accept just in case :)
Jan 5 2023
Sorry, I got a bit swamped, will try to take a look next week. In the meantime, did you have a chance to run this over some open source projects? Did you find any interesting diffs?
I realized I did not say this explicitly in my previous comment, but feel free to commit :)
Jan 4 2023
This is a big improvement, I skimmed through it and looks good to me. There might be some sneaky ways to change values of fields without mentioning them, e.g., casting a pointer to a struct to an array of chars. But I think reasoning about those scenarios is probably not something we want to model anytime soon.
Jan 3 2023
Dec 21 2022
Dec 20 2022
Dec 19 2022
Dec 16 2022
I don't mind committing these patches, I have a high confidence in you going back and addressing feedback post-commit if something is coming up.
Dec 14 2022
Dec 9 2022
This approach looks good to me. Some context: we kept the CFGs lightweight because it looks like we did not need to do any extensions for the Clang Static Analyzer. I'm glad the dataflow framework can also work with the current representation. On the other hand, I think structured bindings in C++ are really limited, e.g., we cannot nest them, and it is nowhere near what pattern matching can do in other languages like Rust. I do remember seeing papers about extending structured bindings in at least some dimensions like nesting. I wonder if making the representation more explicit in the CFG as structured bindings get more complex will simplify implementations in the future, but that is a problem for tomorrow :)
Dec 7 2022
Dec 5 2022
Nov 24 2022
Nov 21 2022
I am a bit conflicted. It is unfortunate that C and C++ compilers regarded single element array members as flexible array members. On the other hand, looking at GCC, it recently added -fstrict-flex-arrays=2 as an option to no longer consider single element member arrays as FAM. So, it looks like the community wants to migrate away from this. My main concern is whether this option would make the experience worse for people who keep their code tidy and favor people who did not update their FAMs. Overall, I wonder if diagnosing single element arrays that are likely FAMs and suggesting users to fix their code is a better way forward.
I guess there are some more options. We could try keeping representatives alive no matter what. It could be a good exercise to see if doing that makes any difference in the analysis results.
I did not spend too much time thinking about this yet, but this sounds scary. I wonder if we should target the underlying problem instead, i.e., making sure we never have dead symbols as representatives for eq. classes. What do you think?
Sounds reasonable to me.
Nov 18 2022
Is the problem forEachDescendant matching statements inside blocks and lambdas? I wonder if this behavior would surprise people, so I think it would be better to:
- Potentially add a template bool parameter to forEachDescendant controlling this behavior.
- Review existing uses because I am not entirely sure if the current behavior is the right default.
Overall, the document looks good to me, I like the general direction. I still see some pending comments (mostly small wording fixes) from Aaron.
Nov 16 2022
Nov 15 2022
Nov 14 2022
Nov 11 2022
Thanks, the latest version looks good to me.
Nov 7 2022
Mostly looks good to me, but there still is a comment that is unaddressed.
Nov 4 2022
Nov 3 2022
Nov 2 2022
Start to look good, I still have a couple of questions/comments inline.
Oct 27 2022
Oct 25 2022
With the rest of the comments addressed it looks good to me.
Oct 14 2022
I'd argue the warning is correct as you could store "non-enum" integer values into an enum typed variable (and people do that fairly often with bitwise enums).
Oh, I see, this look deliberate. I wonder if we want to add an exception for lambdas though but that would be a different discussions unrelated to this PR.