Page MenuHomePhabricator

JonasToth (Jonas Toth)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 31 2016, 11:13 AM (111 w, 1 d)

Recent Activity

Thu, Dec 13

JonasToth added inline comments to D55433: [clang-tidy] Adding a new modernize use nodiscard checker.
Thu, Dec 13, 6:38 AM · Restricted Project
JonasToth added inline comments to D55433: [clang-tidy] Adding a new modernize use nodiscard checker.
Thu, Dec 13, 6:36 AM · Restricted Project
JonasToth accepted D55595: [clang-tidy] Share the forced linking code between clang-tidy tool and plugin.

LGTM, chatted with steveire on IRC: blocking this for a better cmake based solution makes no sense. So your free to commit :)

Thu, Dec 13, 6:31 AM · Restricted Project
JonasToth accepted D55245: [clang-tidy] Add the abseil-duration-subtraction check.

LGTM with the doc-nit.

Thu, Dec 13, 5:58 AM · Restricted Project
JonasToth added a comment to D55595: [clang-tidy] Share the forced linking code between clang-tidy tool and plugin.

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.

Thu, Dec 13, 5:55 AM · Restricted Project
JonasToth added a comment to D55433: [clang-tidy] Adding a new modernize use nodiscard checker.

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"

Thu, Dec 13, 3:52 AM · Restricted Project
JonasToth removed a reviewer for D54349: [clang-tidy] new check 'readability-redundant-preprocessor': JonasToth.
Thu, Dec 13, 3:21 AM · Restricted Project
JonasToth removed a reviewer for D55044: [clang-tidy] check for Abseil make_unique: JonasToth.
Thu, Dec 13, 3:21 AM · Restricted Project
JonasToth added a comment to D55245: [clang-tidy] Add the abseil-duration-subtraction check.

Did you rebase the check onto the new master with your refactorings in?

Thu, Dec 13, 3:21 AM · Restricted Project
JonasToth resigned from D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one.
Thu, Dec 13, 3:14 AM
JonasToth added inline comments to D55523: [clang-tidy] Linting .rst documentation.
Thu, Dec 13, 3:13 AM · Restricted Project

Wed, Dec 12

JonasToth added inline comments to D55245: [clang-tidy] Add the abseil-duration-subtraction check.
Wed, Dec 12, 9:06 AM · Restricted Project
JonasToth added a comment to D55595: [clang-tidy] Share the forced linking code between clang-tidy tool and plugin.

Can you say where else it is common in LLVM? I'm curious. Maybe those places could be changed too.

Wed, Dec 12, 7:53 AM · Restricted Project
JonasToth added a comment to D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV)..

Sounds like we might not correctly set the parent map for CXXConstructorDecl then?

Wed, Dec 12, 4:38 AM
JonasToth added a comment to D55523: [clang-tidy] Linting .rst documentation.

Neat script, I think we could even plug that into arc diff. AFAIK it can run linters

Wed, Dec 12, 3:46 AM · Restricted Project
JonasToth added a comment to D55595: [clang-tidy] Share the forced linking code between clang-tidy tool and plugin.

FYI, CMake target property INTERFACE_SOURCES is designed to make this easy.

For each module you would generate a file containing

extern volatile int ${MODULE_NAME}ModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED ${MODULE_NAME}ModuleAnchorDestination =
    ${MODULE_NAME}ModuleAnchorSource;

and then put that generated file in the INTERFACE_SOURCES of each module.

target_sources(${MODULE_NAME} INTERFACE ${THE_GENERATED_FILE}.cpp)

Then, you don't need to maintain it in C++ like this. It is DRY because the target_link_libraries entry for the library is what causes the symbol to be used.

Wed, Dec 12, 3:30 AM · Restricted Project
JonasToth added a reviewer for D55594: [clang-tidy] update readability-* and zircon-* check documentation to remove extraneous characters: alexfh.
Wed, Dec 12, 3:30 AM · Restricted Project
JonasToth retitled D55595: [clang-tidy] Share the forced linking code between clang-tidy tool and plugin from Share the forced linking code between clang-tidy tool and plugin to [clang-tidy] Share the forced linking code between clang-tidy tool and plugin.
Wed, Dec 12, 3:24 AM · Restricted Project
JonasToth added a comment to D55594: [clang-tidy] update readability-* and zircon-* check documentation to remove extraneous characters.

I thought I would just correct readability-* and zircon-* to get some feedback if you think this is improving the documentation the way you think it should, if this is OK I can follow up with the other checks (not sure if they are better in separate commits or all together)

Wed, Dec 12, 12:48 AM · Restricted Project

Tue, Dec 11

JonasToth accepted D55561: Stop stripping comments from AST matcher example code.

LGTM

Tue, Dec 11, 11:17 AM
JonasToth added a comment to D55541: Use the standard Duration factory matcher.

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).

And to add to that, please do set the correct "Repo" field, correct tag if appropriate (clang-tools-extra), and subscribe the appropriate -commits list (in this case, cfe-commits.)

Tue, Dec 11, 4:54 AM · Restricted Project
JonasToth changed the repository for D55541: Use the standard Duration factory matcher from rL LLVM to rCTE Clang Tools Extra.
Tue, Dec 11, 4:54 AM · Restricted Project
JonasToth added a comment to D55541: Use the standard Duration factory matcher.

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).

Tue, Dec 11, 4:52 AM · Restricted Project
JonasToth committed rCTE348842: Use the standard Duration factory matcher.
Use the standard Duration factory matcher
Tue, Dec 11, 4:49 AM
JonasToth committed rL348842: Use the standard Duration factory matcher.
Use the standard Duration factory matcher
Tue, Dec 11, 4:49 AM
JonasToth closed D55541: Use the standard Duration factory matcher.
Tue, Dec 11, 4:48 AM · Restricted Project
JonasToth committed rCTE348840: [clang-tidy] NFC Consolidate test absl::Time implementation.
[clang-tidy] NFC Consolidate test absl::Time implementation
Tue, Dec 11, 4:46 AM
JonasToth committed rL348840: [clang-tidy] NFC Consolidate test absl::Time implementation.
[clang-tidy] NFC Consolidate test absl::Time implementation
Tue, Dec 11, 4:45 AM
JonasToth accepted D55541: Use the standard Duration factory matcher.

LGTM

Tue, Dec 11, 4:45 AM · Restricted Project
JonasToth closed D55540: Consolidate test absl::Time implementation.
Tue, Dec 11, 4:45 AM · Restricted Project
JonasToth accepted D55540: Consolidate test absl::Time implementation.

LGTM

Tue, Dec 11, 4:44 AM · Restricted Project
JonasToth added a comment to D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV)..

See PR39949 as well, as it seems to trigger the same or a similar problem.
@ioeric and @klimek maybe could give an opinion, too?

Tue, Dec 11, 4:44 AM

Mon, Dec 10

JonasToth added inline comments to D55245: [clang-tidy] Add the abseil-duration-subtraction check.
Mon, Dec 10, 1:19 PM · Restricted Project
JonasToth added inline comments to D55433: [clang-tidy] Adding a new modernize use nodiscard checker.
Mon, Dec 10, 1:05 PM · Restricted Project
JonasToth added inline comments to D55523: [clang-tidy] Linting .rst documentation.
Mon, Dec 10, 11:53 AM · Restricted Project
JonasToth committed rCTE348793: [clang-tidy] insert release notes for new checkers alphabetically.
[clang-tidy] insert release notes for new checkers alphabetically
Mon, Dec 10, 11:51 AM
JonasToth added a comment to D55508: [clang-tidy] insert release notes for new checkers alphabetically.

By the word, will be good idea to have script which check alphabetical order and use it during build. Sometimes alphabetical order may be violated after merge with trunk.

Mon, Dec 10, 11:51 AM · Restricted Project
JonasToth committed rL348793: [clang-tidy] insert release notes for new checkers alphabetically.
[clang-tidy] insert release notes for new checkers alphabetically
Mon, Dec 10, 11:45 AM
JonasToth closed D55508: [clang-tidy] insert release notes for new checkers alphabetically.
Mon, Dec 10, 11:45 AM · Restricted Project
JonasToth added a comment to D55508: [clang-tidy] insert release notes for new checkers alphabetically.

LGTM. Do you have commit access?

I do not I'm afraid

Mon, Dec 10, 11:43 AM · Restricted Project
JonasToth accepted D55508: [clang-tidy] insert release notes for new checkers alphabetically.

LGTM. Do you have commit access?

Mon, Dec 10, 10:18 AM · Restricted Project
JonasToth added a comment to D55523: [clang-tidy] Linting .rst documentation.

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 ;)

Mon, Dec 10, 10:17 AM · Restricted Project
JonasToth added a reviewer for D55523: [clang-tidy] Linting .rst documentation: aaron.ballman.
Mon, Dec 10, 10:16 AM · Restricted Project
JonasToth added a comment to D55508: [clang-tidy] insert release notes for new checkers alphabetically.

I think this patch is fine, AFAIK these utility scripts are not tested directly but are just adjusted if they dont work as expected :)

Mon, Dec 10, 9:52 AM · Restricted Project
JonasToth added a comment to D55433: [clang-tidy] Adding a new modernize use nodiscard checker.

Hi MyDeveloperDay,

Mon, Dec 10, 9:52 AM · Restricted Project
JonasToth added inline comments to D55245: [clang-tidy] Add the abseil-duration-subtraction check.
Mon, Dec 10, 8:34 AM · Restricted Project
JonasToth added inline comments to D55508: [clang-tidy] insert release notes for new checkers alphabetically.
Mon, Dec 10, 8:02 AM · Restricted Project

Fri, Dec 7

JonasToth added a comment to D55409: [clang-tidy] check for using declarations not in an anonymous namespace when there exists one.

Please upload with full context.

Fri, Dec 7, 5:07 AM
JonasToth added a comment to D55410: [clang-tidy] check for flagging using declarations in headers.

Please upload with full context.

Fri, Dec 7, 5:06 AM · Restricted Project
JonasToth added a comment to D55411: [clang-tidy] check for flagging using declarations not in the inner most namespace.

Please upload the patch with full context.

Fri, Dec 7, 5:05 AM · Restricted Project
JonasToth added reviewers for D55410: [clang-tidy] check for flagging using declarations in headers: aaron.ballman, alexfh, hokein.
Fri, Dec 7, 5:01 AM · Restricted Project
JonasToth added reviewers for D55411: [clang-tidy] check for flagging using declarations not in the inner most namespace: aaron.ballman, alexfh, hokein.
Fri, Dec 7, 5:00 AM · Restricted Project

Wed, Dec 5

JonasToth added a comment to D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV)..

Hi Sam,

Wed, Dec 5, 1:30 PM
JonasToth added a comment to D55245: [clang-tidy] Add the abseil-duration-subtraction check.

Ping.

I assume I've got the right reviewers here, but I've also been sending a bunch of stuff your way lately, so if I'm overwhelming review capacity, please let me know.

Wed, Dec 5, 1:11 PM · Restricted Project
JonasToth added inline comments to D54395: [clang-tidy] implement utility-function to add 'const' to variables.
Wed, Dec 5, 5:12 AM
JonasToth added a comment to D54757: [clang-tidy] new check: bugprone-branch-clone.

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)

Wed, Dec 5, 1:38 AM · Restricted Project
JonasToth committed rCTE348344: Revert "[clang-tidy] new check: bugprone-branch-clone".
Revert "[clang-tidy] new check: bugprone-branch-clone"
Wed, Dec 5, 1:37 AM
JonasToth committed rL348344: Revert "[clang-tidy] new check: bugprone-branch-clone".
Revert "[clang-tidy] new check: bugprone-branch-clone"
Wed, Dec 5, 1:37 AM
JonasToth committed rL348343: [clang-tidy] new check: bugprone-branch-clone.
[clang-tidy] new check: bugprone-branch-clone
Wed, Dec 5, 1:19 AM
JonasToth committed rCTE348343: [clang-tidy] new check: bugprone-branch-clone.
[clang-tidy] new check: bugprone-branch-clone
Wed, Dec 5, 1:19 AM
JonasToth closed D54757: [clang-tidy] new check: bugprone-branch-clone.
Wed, Dec 5, 1:19 AM · Restricted Project
JonasToth added a comment to D55255: Fix a false positive in misplaced-widening-cast.

Committed, Thank you for the patch! Was there a bug-report for this issue? If yes can you please close it/reference?

Wed, Dec 5, 12:34 AM · Restricted Project
JonasToth committed rCTE348341: Fix a false positive in misplaced-widening-cast.
Fix a false positive in misplaced-widening-cast
Wed, Dec 5, 12:33 AM
JonasToth committed rL348341: Fix a false positive in misplaced-widening-cast.
Fix a false positive in misplaced-widening-cast
Wed, Dec 5, 12:33 AM
JonasToth closed D55255: Fix a false positive in misplaced-widening-cast.
Wed, Dec 5, 12:32 AM · Restricted Project

Tue, Dec 4

JonasToth retitled D55245: [clang-tidy] Add the abseil-duration-subtraction check from Add the abseil-duration-subtraction check to [clang-tidy] Add the abseil-duration-subtraction check.
Tue, Dec 4, 4:52 AM · Restricted Project
JonasToth added a comment to D54757: [clang-tidy] new check: bugprone-branch-clone.

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.

Tue, Dec 4, 3:14 AM · Restricted Project

Mon, Dec 3

JonasToth added a comment to D54737: [clang-tidy] Add the abseil-duration-comparison check.

int8 ? Did you mean int8_t or am I missing somthing ?

Mon, Dec 3, 11:45 AM · Restricted Project
JonasToth committed rCTE348172: [clang-tidy] Fix unordered_map failure with specializing std::hash<> and remove….
[clang-tidy] Fix unordered_map failure with specializing std::hash<> and remove…
Mon, Dec 3, 11:44 AM
JonasToth committed rL348172: [clang-tidy] Fix unordered_map failure with specializing std::hash<> and remove….
[clang-tidy] Fix unordered_map failure with specializing std::hash<> and remove…
Mon, Dec 3, 11:44 AM
JonasToth added a comment to D54737: [clang-tidy] Add the abseil-duration-comparison check.

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 ;)

Mon, Dec 3, 11:27 AM · Restricted Project
JonasToth committed rL348169: [clang-tidy] Recommit: Add the abseil-duration-comparison check.
[clang-tidy] Recommit: Add the abseil-duration-comparison check
Mon, Dec 3, 11:25 AM
JonasToth committed rCTE348169: [clang-tidy] Recommit: Add the abseil-duration-comparison check.
[clang-tidy] Recommit: Add the abseil-duration-comparison check
Mon, Dec 3, 11:25 AM
JonasToth committed rL348165: Revert "[clang-tidy] Add the abseil-duration-comparison check".
Revert "[clang-tidy] Add the abseil-duration-comparison check"
Mon, Dec 3, 11:02 AM
JonasToth committed rCTE348165: Revert "[clang-tidy] Add the abseil-duration-comparison check".
Revert "[clang-tidy] Add the abseil-duration-comparison check"
Mon, Dec 3, 11:02 AM
JonasToth committed rCTE348161: [clang-tidy] Add the abseil-duration-comparison check.
[clang-tidy] Add the abseil-duration-comparison check
Mon, Dec 3, 10:39 AM
JonasToth committed rL348161: [clang-tidy] Add the abseil-duration-comparison check.
[clang-tidy] Add the abseil-duration-comparison check
Mon, Dec 3, 10:39 AM
JonasToth closed D54737: [clang-tidy] Add the abseil-duration-comparison check.
Mon, Dec 3, 10:38 AM · Restricted Project
JonasToth added a comment to D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range.

Hi russell

Mon, Dec 3, 10:21 AM · Restricted Project
JonasToth retitled D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range from Prevent Clang-Format from editing leading whitespace on lines outside of the format range to [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range.
Mon, Dec 3, 10:17 AM · Restricted Project
JonasToth added a comment to D55006: [clang] - Simplify tools::SplitDebugName.

from my side no objections, mailing list did not react AFAIK (just in case your waiting for me until you recommit).

Mon, Dec 3, 10:02 AM
JonasToth accepted D54757: [clang-tidy] new check: bugprone-branch-clone.
Mon, Dec 3, 8:41 AM · Restricted Project
JonasToth added a comment to D54757: [clang-tidy] new check: bugprone-branch-clone.

I applied this check to the llvm + clang codebase, and I was surprised to see that it produced about 600 results (for comparison, the clang-tidy checks which are enabled by default produce approximately 6000 results together). I didn't go through the whole list, but after examining a few dozen random reports it seems that most of these are true positives: relatively short branches (usually one or two lines of code) are repeated for no obvious reason. I would guess that most of these fall into the "correct but too verbose" case, because otherwise the tests would have caught the problem, but I didn't try to understand their context.

I have seen a few false positives related to preprocessor trickery: when an .inc file is included to create a huge switch, sometimes it will become a huge switch with lots of identical branches. There were also some situations where the check reported identical branches which are annotated with different comments; I don't know if this should be considered a false positive.

If this is acceptable, then I would be grateful if someone would commit this patch for me as I don't have commit rights.

Mon, Dec 3, 8:41 AM · Restricted Project
JonasToth added inline comments to D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check.
Mon, Dec 3, 7:51 AM · Restricted Project

Sun, Dec 2

JonasToth added a comment to D55044: [clang-tidy] check for Abseil make_unique.

Thanks for working on this.
Semi-duplicate of D50852 Please see discussion there.
It should not be an abseil-specific check, the prefix (std,abseil), and the function (make_unique) should be a config option, and it should likely be a part of modernize-use-auto.

Sun, Dec 2, 3:59 AM · Restricted Project
JonasToth added a comment to D54737: [clang-tidy] Add the abseil-duration-comparison check.

Oh, and thanks for taking the time to review this. :)

Sun, Dec 2, 3:55 AM · Restricted Project
JonasToth added a comment to D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check.

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).

Sun, Dec 2, 3:51 AM · Restricted Project

Fri, Nov 30

JonasToth added a comment to D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check.

@JonasToth this is the Lexer based expression equality check I talked about in D54757#1311516. The point of this patch is that the definition is a macro sure could be build dependent, but the macro name is not, so it wouldn't warn on the case I showed. What do you think?

Fri, Nov 30, 8:43 AM · Restricted Project
JonasToth accepted D54737: [clang-tidy] Add the abseil-duration-comparison check.

Only the test and your opinion on the Optional thing, If you want to keep it that way its fine.

Fri, Nov 30, 8:28 AM · Restricted Project
JonasToth added a comment to D54757: [clang-tidy] new check: bugprone-branch-clone.

LGTM, could you please run this check over real world code and check that there are no obvious false positives?

Fri, Nov 30, 8:24 AM · Restricted Project

Thu, Nov 29

JonasToth added inline comments to D54757: [clang-tidy] new check: bugprone-branch-clone.
Thu, Nov 29, 10:59 AM · Restricted Project
JonasToth added a comment to D54757: [clang-tidy] new check: bugprone-branch-clone.

Macros:

The current implementation of the check only looks at the preprocessed code, therefore it detects the situations where the duplication is created by macros. I added some tests to highlight this behavior.

The only option I see to detect macro related errors is to use a Lexer, and compare the tokens one by one, but that might be overkill, and I'm aware of a check in the works that already implements that. I think it's perfectly fine not to cover those cases, but you should definitely document it somewhere, preferably in the rst file.

Thu, Nov 29, 10:56 AM · Restricted Project

Wed, Nov 28

JonasToth added inline comments to D54737: [clang-tidy] Add the abseil-duration-comparison check.
Wed, Nov 28, 1:55 PM · Restricted Project
JonasToth added a comment to D55006: [clang] - Simplify tools::SplitDebugName.

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.

Wed, Nov 28, 1:50 PM
JonasToth added inline comments to D54737: [clang-tidy] Add the abseil-duration-comparison check.
Wed, Nov 28, 12:03 PM · Restricted Project
JonasToth added a comment to D54737: [clang-tidy] Add the abseil-duration-comparison check.

LG from my side, only the style nits left.
other reviewers are invited to take a look too :)

Wed, Nov 28, 10:57 AM · Restricted Project
JonasToth added a comment to D55006: [clang] - Simplify tools::SplitDebugName.

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.

Wed, Nov 28, 7:37 AM
JonasToth added a comment to rL347035: [clang] - Simplify tools::SplitDebugName..

Hi Jonas, I am sorry for the breakage.

Wed, Nov 28, 2:04 AM

Tue, Nov 27

JonasToth added a comment to rL347035: [clang] - Simplify tools::SplitDebugName..

Revert in https://reviews.llvm.org/rL347676

Tue, Nov 27, 9:33 AM
JonasToth added inline comments to rL347035: [clang] - Simplify tools::SplitDebugName..
Tue, Nov 27, 9:32 AM