- User Since
- Oct 31 2019, 10:15 AM (16 w, 6 d)
Mon, Feb 24
Fri, Feb 21
Thu, Feb 20
Tue, Feb 18
Wed, Feb 12
Mon, Feb 10
Fri, Feb 7
Tue, Feb 4
Mon, Feb 3
Thu, Jan 30
Jan 27 2020
LGTM with fixes to the test.
Thank you for the contribution! I didn't review the code thoroughly yet, only the tests.
Please talk to Hans Wennborg <firstname.lastname@example.org> about cherry-picking this change into the release. I think it is a safe change, if Hans needs that sort of review from someone.
Jan 26 2020
Jan 24 2020
LGTM! Please commit if you have commit access, or let me know if I should push it for you.
Jan 23 2020
Jan 22 2020
I'd also appreciate if you updated the docs for the changes done in this patch.
Sorry, just getting back to this review. Your justification makes sense, and the patch LGTM.
Jan 21 2020
Let me know if you want me to commit this change for you.
Jan 17 2020
Sorry, no, it broke it in a different way:
The only functional change that I see in this patch is in clang/lib/Tooling/Transformer/Stencil.cpp. However, I don't understand how that change in the (deprecated) selection() stencil can affect other stencils.
Since I seem to be in the minority about thinking that this check does not pull its weight, I reviewed the code, and will LGTM and push once the few small issues are fixed.
Thank you for factoring our this library!
This change broke buildbots and I reverted it in 10b4aece528936bb7f75a9758ae95c61b6434d2f.
Jan 16 2020
A test would be nice, but we don't have infrastructure for checking call ordering.
Jan 15 2020
Jan 14 2020
Thank you for the patch!
Jan 2 2020
Could you provide a more fleshed out example of a case where it is useful?
Dec 30 2019
This change broke buildbots: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/21066.
Dec 20 2019
@Ka-Ka Thanks for bringing it to our attention! I fixed these warnings in:
Dec 19 2019
Sorry, I reverted this change because it broke the build on aarch64. https://github.com/llvm/llvm-project/commit/0109efe7513dd984cf67d102ce5179a5b24d58f6
Dec 18 2019
I believe this commit broke the expensive checks bot. Could you take a look?
Updated version LGTM, thanks!
LGTM, feel free to push.
Updated version LGTM, thanks!
Dec 16 2019
Sorry, I reverted this change in 079ef783dd5530b5f87beefe624b9179547ded7e. The tests depend on builtin headers, which is not intentionally supported in clangd tests; these tests are broken in some build environments.
Dec 13 2019
Generally looks good, just nitpicks.
I reverted this change in 34536db7bbe0b8c5f8ffa70df307312b451aca2e. This change didn't compile: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/20462. Please always run ninja check-all before pushing.
Dec 9 2019
LGTM with comments fixed.
Dec 4 2019
Is this a common problem? There's a lot of silly code we could try to find, but if people don't actually write it, then we get all downsides of maintenance without the benefits of the checker.
I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?
Dec 3 2019
@twardakm: I'm not convinced this feature is worth implementing at all, because there's a good alternative to a macro here -- a namespace alias. What is the reason to use a macro instead of a namespace alias?
With that last comment, LGTM. Do you have commit access?
Dec 2 2019
A few more comments, but generally looks good!
Nov 29 2019
Nov 27 2019
Nov 22 2019
Thank you very much for the quick workaround!
Sorry for the delayed response, but the OCaml test is failing: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/19714/steps/test-check-all/logs/stdio
Thank you for the contribution and sorry for the review delay!
The newly added tests fail on buildbots: http://lab.llvm.org:8011/builders/openmp-gcc-x86_64-linux-debian/builds/236
This patch introduces a way to apply the fix-its by the Analyzer:
Nov 21 2019
@chandlerc has LGTM'ed this patch over chat.
Nov 19 2019
hence not throwing the warning on any platform?