Page MenuHomePhabricator

poelmanc (Conrad Poelman)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 11 2019, 11:23 AM (5 w, 2 d)

Recent Activity

Yesterday

poelmanc created D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.
Thu, Oct 17, 3:25 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line.

cleanupAroundReplacements is not used by clang-format itself. I believe, it's a part of clang-format only because it was easier to implement it on top of some infrastructure clang-format provides.

Thanks for the correction @alexfh. I've updated the patch to follow @gribozavr's suggestion to perform the line removal within cleanupAroundReplacements().

Thu, Oct 17, 2:46 PM · Restricted Project, Restricted Project

Fri, Oct 11

poelmanc added a comment to D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line.

I guess here's the high-level question: should all removals that remove all non-blank text from a line also delete the line?

I see your point, but as https://llvm.org/PR43583 shows, we have a much larger problem: textual replacements don't compose. So, whatever we do, it will only be hacky workarounds for specific high-value cases.

About how exactly to do it. It would be preferred if checkers that already know that they are deleting a full line, just deleted the newline characters. However, for other cases, I feel like the place where this patch implements newline deletion is not ideal. It implements newline deletion while applying all fixes. Applying fixes is not the only client of fixit information. Probably it is one of the least-important ones, to be honest. I don't know who will run clang-tidy and just trust it to apply whatever fixes it wants. More likely, the user will want to see a preview of findings and fixes, and fixit application will be done by some other tool. Therefore, implementing newline deletion in the code that applies all fixes is not going to be helpful for most workflows.

Therefore, I think the most appropriate place to do this cleanup is cleanupAroundReplacements. What do you think?

Fri, Oct 11, 2:22 PM · Restricted Project, Restricted Project
poelmanc added a comment to D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line.

You may be interested to also look on PR43583 related to readability-redundant-member-init.

Thanks Eugene, I'm having trouble finding that. https://reviews.llvm.org/D43583 seems related to MIPS instructions rather than readability-redundant-member-init. Could you please post a link? Thanks.

PRs are bug reports. You can access them like this: https://llvm.org/PR43583

Or direct link to LLVM Bugzilla (https://bugs.llvm.org/show_bug.cgi?id=<Number>). Ideally Phabricator should recognize PR prefixes in same way as it recognize D.

Fri, Oct 11, 2:13 PM · Restricted Project, Restricted Project

Wed, Oct 9

poelmanc added inline comments to D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line.
Wed, Oct 9, 2:41 PM · Restricted Project, Restricted Project
poelmanc added a comment to D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line.

You may be interested to also look on PR43583 related to readability-redundant-member-init.

Wed, Oct 9, 12:23 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line.

Thanks for the prompt review and feedback! MyDeveloperDay, your "drive-by" comments are quite welcome as the goal is to make clang-tidy and clang-format work better in tandem. You are correct that my prior version could have thwarted someone's chang-tidy checker that adds whitespace, which was an oversight on my part. I've updated the patch to change isWhitespace( RemovalText ) to RemovalText.isBlank(). Now this only applies to Removals that remove all non-blank text from a line. I also updated the Title and Summary accordingly, adding the example that most confounded me of ": a()\n", which readability-redundant-member-init makes blank with a combination of two separate removals: one for the ":" and one for the "a()".

Wed, Oct 9, 12:04 PM · Restricted Project, Restricted Project

Tue, Oct 8

poelmanc created D68682: Clang-tidy fix removals removing all non-blank text from a line should remove the line.
Tue, Oct 8, 8:02 PM · Restricted Project, Restricted Project

Fri, Oct 4

poelmanc added a comment to D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Hi @alexfh, @jonathanmeier has reviewed my pull request but lacks commit access. It changes ~30 lines of code isolated to modernize-use-using.cpp and adds ~60 lines of tests. If you have time I'd greatly appreciate it if you could provide any feedback or commit it. Alternatively can you suggest someone else who can review it? Thanks!

Fri, Oct 4, 10:25 AM · Restricted Project, Restricted Project

Sep 12 2019

poelmanc updated the diff for D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Sorry one more test at the end to make sure a multi-typedef with all that Variadic stuff still doesn't get changed to using.

Sep 12 2019, 2:44 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Nice catch, that simplifies the code quite a bit! I added two more nested, complex multiple-template-argument tests and am happy to add more.

Sep 12 2019, 2:37 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Thanks for the stressing test cases. I reverted to your original test case with a single >, added a separate one with a single <, and expanded the final case to have a non-balanced number of > and <. I added all your new cases, with variations for non-fixed (multiple typedef) and fixed (single typedef) examples.

Sep 12 2019, 11:38 AM · Restricted Project, Restricted Project

Sep 11 2019

poelmanc added a comment to D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Wow, thanks for the super-quick testing, feedback and a new test case! I added a slightly enhanced version of your test case to the modernize-use-using.cpp test file and got it passing by treating tok::less and tok::right as AngleBrackets only when ParenLevel == 0. See updated patch above. Holler if you think of any other stressing cases!

Sep 11 2019, 5:21 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D67460: clang-tidy: modernize-use-using work with multi-argument templates.
Sep 11 2019, 5:17 PM · Restricted Project, Restricted Project
poelmanc updated the summary of D67460: clang-tidy: modernize-use-using work with multi-argument templates.
Sep 11 2019, 12:50 PM · Restricted Project, Restricted Project
poelmanc created D67460: clang-tidy: modernize-use-using work with multi-argument templates.
Sep 11 2019, 12:48 PM · Restricted Project, Restricted Project