[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

AlexanderLanin created this revision. Herald added subscribers: JDevlieghere, mgorny. Suggestion for a new check that will warn on #defines that should rather be constant values. Const variables should be preferred as #define does not obey type checking and scope rules. Please feel free to

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

AlexanderLanin added a comment. Thanks for the feedback. As I'm new to this I cannot say whether checking only the file in question fits better with clang-tidy’s policy or not. Also, I’m not sure about ODR. Of course, it’s a possibility, but isn’t the programmer responsible for that? This will

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

AlexanderLanin marked 9 inline comments as done. AlexanderLanin added a comment. Not sure about CppCoreGuidelines as several guidelines have the same rule and I only used CppCoreGuidelines as it's convenient to link a specific rule. But I can move it if you like?! Comment

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

AlexanderLanin updated this revision to Diff 88103. AlexanderLanin marked 2 inline comments as done. AlexanderLanin added a comment. Fixes as reported in review https://reviews.llvm.org/D29692 Files: clang-tidy/modernize/CMakeLists.txt clang-tidy/modernize/ModernizeTidyModule.cpp

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

AlexanderLanin marked 5 inline comments as done. AlexanderLanin added inline comments. Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:86 +else if (isAnyParenthesis(Tok) || isAnyNumericLiteral(Tok) || + (!hasPrefix && isAnyCharLiteral(Tok))) { +

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

AlexanderLanin updated this revision to Diff 88447. AlexanderLanin marked an inline comment as done. AlexanderLanin added a comment. Applied review comments and added test cases regarding parenthesis, floats, doubles, wide chars etc https://reviews.llvm.org/D29692 Files:

[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

AlexanderLanin added a comment. On second thought maybe this should be fixed in clang-tidy and not here? Maybe besides `-export-fixes` there should be an `-export-warnings` or just `-yaml-export`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72730/new/ https://reviews.llvm.org/D72730

[PATCH] D16267: Handle C++11 brace initializers in readability-braces-around-statements

AlexanderLanin added a comment. Here is the r_brace use case: if (true) while ("ok") {} Unfortunately it looks quite similar to: if (true) s = {}; CHANGES SINCE LAST ACTION https://reviews.llvm.org/D16267/new/ https://reviews.llvm.org/D16267

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

AlexanderLanin added a comment. In D54943#1815772 , @0x8000- wrote: > This generated 56 "const const" declarations, of which 25 were triple const! I've tried this on ccache (326 fixes, nice!), but some came out as 'const const'. At least in my case

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

AlexanderLanin abandoned this revision. AlexanderLanin added a comment. This is now detected by cppcoreguidelines-macro-usage, so this seems out of date. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D29692/new/ https://reviews.llvm.org/D29692

[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

AlexanderLanin updated this revision to Diff 241895. AlexanderLanin added a comment. Review findings applied (no real measurable difference in speed, maybe 100ms) but it's indeed more readable. Without this fix the export took 12.96 seconds, with this patch 11.07 seconds. Difference is even

[PATCH] D73856: [docu] Improve docu of misc-misplaced-const

AlexanderLanin abandoned this revision. AlexanderLanin added a comment. @aaron.ballman I was ready to complain, but you are right. It would make absolutely no sense to move the const, it's just where it should be. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D73856: [docu] Improve docu of misc-misplaced-const

AlexanderLanin created this revision. AlexanderLanin added a project: clang-tools-extra. Herald added a project: clang. Herald added a subscriber: cfe-commits. East const makes the problem more obvious. With west const people were telling me the const is on the wrong side. Also it's time to use

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

AlexanderLanin updated this revision to Diff 237558. AlexanderLanin marked an inline comment as done. AlexanderLanin added a comment. Updated revision with llvm_unreachable CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72373/new/ https://reviews.llvm.org/D72373 Files:

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

AlexanderLanin added a comment. Thanks for the review! Could someone commit this? As I can not. Alexander Lanin CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72373/new/ https://reviews.llvm.org/D72373 ___ cfe-commits mailing list

[PATCH] D72730: [clang-tools-extra] run-clang-tidy -export-fixes exports only fixes, not all warnings

AlexanderLanin added a comment. Important: I'm not a python expert. It works fine as far as I can tell. I would feel better if someone with more than a day python experience would say that it is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72730: [clang-tools-extra] run-clang-tidy -export-fixes exports only fixes, not all warnings

AlexanderLanin created this revision. AlexanderLanin added a project: clang-tools-extra. Herald added a project: clang. Herald added a subscriber: cfe-commits. AlexanderLanin added a comment. Important: I'm not a python expert. It works fine as far as I can tell. I would feel better if someone

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

AlexanderLanin added a comment. ping > Could someone commit this? As I can not. > Alexander Lanin CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72373/new/ https://reviews.llvm.org/D72373 ___ cfe-commits mailing list

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

AlexanderLanin updated this revision to Diff 239665. AlexanderLanin added a comment. Updated misc-misplaced-const.c to reflect new output and fixed one wrong col in misc-misplaced-const.cpp - I really do not understand how that happened. Rebased and verified with check-clang-tools. CHANGES

[PATCH] D72057: [docs] Update dead anchor in hacking page

AlexanderLanin marked an inline comment as done. AlexanderLanin added inline comments. Comment at: clang/www/hacking.html:301 - It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang. + It is also possible

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

AlexanderLanin updated this revision to Diff 235968. AlexanderLanin retitled this revision from "[docs] Update dead anchor in hacking page" to "[docs] Remove outdated svn/git information from hacking page". AlexanderLanin edited the summary of this revision. AlexanderLanin added a comment. Not

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

AlexanderLanin updated this revision to Diff 236088. AlexanderLanin marked an inline comment as done. AlexanderLanin added a comment. `Getting Started` since it's `Getting Started with the LLVM System` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72057/new/

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

AlexanderLanin added a comment. In D72057#1802982 , @aaron.ballman wrote: > LGTM with a small typo fix. Do you need someone to commit this on your behalf? Yes, please. I cannot commit myself. Alexander Lanin CHANGES SINCE LAST ACTION

[PATCH] D71982: [docs] Update path to clang-tools-extra

AlexanderLanin updated this revision to Diff 235801. AlexanderLanin marked an inline comment as done. AlexanderLanin edited the summary of this revision. AlexanderLanin added a comment. Removed change in hacking page as discussed. Can someone commit this as apparently I cannot do it myself (I'll

[PATCH] D72057: [docs] Update dead anchor in hacking page

AlexanderLanin marked an inline comment as done. AlexanderLanin added inline comments. Comment at: clang/www/hacking.html:301 - It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang. + It is also possible

[PATCH] D71982: [docs] Update path to clang-tools-extra

AlexanderLanin marked 3 inline comments as done. AlexanderLanin added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:66 -Once you are done, change to the ``llvm/tools/clang/tools/extra`` directory, and +Once you are done, change to the

[PATCH] D72057: [docs] Update dead anchor in hacking page

AlexanderLanin created this revision. AlexanderLanin added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D72057 Files: clang/www/hacking.html Index: clang/www/hacking.html

[PATCH] D71982: [docs] Update path to clang-tools-extra

AlexanderLanin added a comment. @Jim `git commit --amend --author="Alexander Lanin "` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71982/new/ https://reviews.llvm.org/D71982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D71982: [docs] Update path to clang-tools-extra

AlexanderLanin created this revision. AlexanderLanin added projects: clang-tools-extra, clang. Herald added a subscriber: cfe-commits. > tools/clang/tools/extra has become > clang-tools-extra which was not updated in all docs. Also hacking page has an outdated anchor for git. Repository:

[PATCH] D71982: [docs] Update path to clang-tools-extra

AlexanderLanin updated this revision to Diff 235577. AlexanderLanin added a comment. Hi, I don't see the error message you mentioned anywhere, but I'm gonna assume it involves the number of surrounding lines. Here is another attempt with > git show HEAD -U99 > mypatch.patch --- Source

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

AlexanderLanin added a comment. looking at this at least many are indeed valid results: F11183407: partial-misc-misplaced-const-llvm-output.zip I guess false positives could be reduced by eliminating those cases where the variables are (intentionally)

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

AlexanderLanin created this revision. AlexanderLanin added a reviewer: aaron.ballman. Herald added subscribers: cfe-commits, xazax.hun. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D72373 Files:

[PATCH] D76037: [clang] tooling replacements are escaped when exporting to YAML

AlexanderLanin requested changes to this revision. AlexanderLanin added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Tooling/ReplacementsYaml.cpp:22 +static constexpr Escapes EscapeChars[] = { +{'\n', 'n'}, {'\r', 'r'}, {'\t', 't'},

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

AlexanderLanin added a comment. Unfortunately I cannot say whether the code is good, but the fix works and certainly helps readability-braces-around-statements which can sometimes add multiple } at the same offset to close several scopes. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D76037: [clang] tooling replacements are escaped when exporting to YAML

AlexanderLanin added a comment. Fine for me. Fixes newline bug in https://bugs.llvm.org/show_bug.cgi?id=45150. However I don't have "review privileges" here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76037/new/ https://reviews.llvm.org/D76037

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

AlexanderLanin added a comment. *ping* CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74299/new/ https://reviews.llvm.org/D74299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

AlexanderLanin added a comment. *ping* CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74299/new/ https://reviews.llvm.org/D74299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

AlexanderLanin added a comment. Observed behavior: Change: `for(string_view token : split_into_views(file_content, " \t\r\n"))` --> `for(string_view const token : split_into_views(file_content, " \t\r\n"))`. Note: `std::vector split_into_views(string_view input, const char* separators);`

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

AlexanderLanin updated this revision to Diff 298566. AlexanderLanin added a comment. Readd removed 'print help without crashing' test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74299/new/ https://reviews.llvm.org/D74299 Files:

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

AlexanderLanin marked an inline comment as done. AlexanderLanin added a comment. Could someone commit this as I cannot? Thanks a lot! Alexander Lanin Thanks for review @aaron.ballman! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74299/new/ https://reviews.llvm.org/D74299

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

AlexanderLanin added a comment. I cannot reproduce the problem and there is just not enough info to go on. Is there any way to get anything in addition? e.g. run the same test with `-vv` or add some additional print commands? Maybe it's some access rights problem, or different version of some

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

AlexanderLanin added a comment. > Could you please provide me a full reproducer (optimally without dependencies > on includes/libraries)? I can certainly do that based on my old version from ~9 months ago or preferably if you provide me some branch somewhere (github?). I have trouble applying

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

AlexanderLanin added a comment. "Minimal" example for the behavior I described before. `clang++ -fsyntax-only ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp -Wrange-loop-analysis 2>&1 | wc -l` 269 Then fix it by: `~/llvm/build-const-fix/bin/clang-tidy

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

AlexanderLanin added a comment. Off-Topic: I was just attempting to apply this to my codebase at work. Seems this will be more challenging than anticipated  My best guess is this is related to D72730 Applying fixes ... terminate called after throwing an

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

AlexanderLanin added a comment. *ping* CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74299/new/ https://reviews.llvm.org/D74299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

AlexanderLanin planned changes to this revision. AlexanderLanin added a comment. The reason was that I thought that line doesn't do anything. Of course "not crashing" is a valid expectation. If nothing else, then "not crashing" should indeed be tested. I'll try to improve this ancient diff.