- User Since
- Oct 31 2016, 11:13 AM (111 w, 1 d)
Thu, Dec 13
LGTM, chatted with steveire on IRC: blocking this for a better cmake based solution makes no sense. So your free to commit :)
LGTM with the doc-nit.
One thing: Could you please check the utility-scripts in clang-tidy that create new checks, if they need adjustments? Not sure if we have something in there that relies on the old structure.
You also get into trouble because there are many header files that it add LLVM_NODISCARD to but they don't include "Compiler.h" where LLVM is defined (so you end up with lots of errors)
3>C:\llvm\include\llvm/ADT/iterator_range.h(46): error C3646: 'IteratorT': unknown override specifier (compiling source file C:\llvm\tools\clang\utils\TableGen\ClangCommentHTMLNamedCharacterReferenceEmitter.cpp)
I'm wondering if there is anything I can add to the checker, to check to see if the macro being used for the ReplacementString is defined "in scope"
Did you rebase the check onto the new master with your refactorings in?
Wed, Dec 12
Sounds like we might not correctly set the parent map for CXXConstructorDecl then?
Neat script, I think we could even plug that into arc diff. AFAIK it can run linters
Tue, Dec 11
Please remember to upload your patches with full context (i can highly recommend using arc, see https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch and https://llvm.org/docs/Phabricator.html for the relevant command).
Mon, Dec 10
LGTM. Do you have commit access?
i think the idea is good to have more tools that do style checks for us.
Plugging them into the normal test-suite is maybe a good idea, as otherwise everyone forgets to use them ;)
I think this patch is fine, AFAIK these utility scripts are not tested directly but are just adjusted if they dont work as expected :)
Fri, Dec 7
Please upload with full context.
Please upload with full context.
Please upload the patch with full context.
Wed, Dec 5
I had to revert this patch because it broke (at least one) buildbot with an assertion-failure (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40436/steps/test/logs/stdio)
Committed, Thank you for the patch! Was there a bug-report for this issue? If yes can you please close it/reference?
Tue, Dec 4
A quick, tangentially related idea: Would it be useful to implement multiline nolint sections (e.g. //BEGINNOLINT – //ENDNOLINT or something similar)? This would be helpful for using clang-tidy on projects that contain some automatically generated fragments.
Mon, Dec 3
int8 ? Did you mean int8_t or am I missing somthing ?
I had to revert and recommitted in rCTE348169. std::unordered_map<enum class Something, ...> does not work, as std::hash is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best solution, but now I specialized the hash-function to std::hash<std::int8>() in unordered_set which worked for me locally and was suggested. Lets see ;)
from my side no objections, mailing list did not react AFAIK (just in case your waiting for me until you recommit).
Sun, Dec 2
Wiring out the lexical comparison and AST based comparison sounds like an interesting idea, however IMHO such a setting is too coarse-grained on the file level. My guess would be that it depends from expression (fragment) to expression fragment how you want to compare them: for macros lexical comparison is better, for arithmetic expressions with many parentheses AST based recursive comparison may be a better fit (as implemented also in this checker).
Fri, Nov 30
Only the test and your opinion on the Optional thing, If you want to keep it that way its fine.
LGTM, could you please run this check over real world code and check that there are no obvious false positives?
Thu, Nov 29
Wed, Nov 28
The issues seems to be resolved for me as well, i will post this patch to the mailing list and ask the other guy if his build is fine, too.
LG from my side, only the style nits left.
other reviewers are invited to take a look too :)
Thank you for the fix! I will try this version as soon as I am at home (~2 hours from now) and report back if the issues I had are gone.
Hi Jonas, I am sorry for the breakage.
Tue, Nov 27
Revert in https://reviews.llvm.org/rL347676