Page MenuHomePhabricator

poelmanc (Conrad Poelman)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 11 2019, 11:23 AM (82 w, 3 d)

Recent Activity

Mar 1 2021

poelmanc added a comment to D97620: Fix DecisionForestBenchmark.cpp compile errors.

Fixed comment as requested (keys()->size().) I don't have commit access so if you can push it that would be great, thanks!

Mar 1 2021, 10:22 AM · Restricted Project, Restricted Project
poelmanc updated the diff for D97620: Fix DecisionForestBenchmark.cpp compile errors.

Fix comment.

Mar 1 2021, 10:21 AM · Restricted Project, Restricted Project

Feb 28 2021

poelmanc added a reviewer for D97620: Fix DecisionForestBenchmark.cpp compile errors: adamcz.
Feb 28 2021, 12:55 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Removed a function in modernize/LoopConvertCheck.cpp that's no longer needed due to this patch. Fixed clang-format and clang-tidy issues identified by HarborMaster in prior submission.

Feb 28 2021, 2:06 AM · Restricted Project, Restricted Project
poelmanc requested review of D97625: fix check-clang-tools tests that fail due to Windows CRLF line endings.
Feb 28 2021, 12:41 AM · Restricted Project, Restricted Project

Feb 27 2021

poelmanc updated the diff for D97620: Fix DecisionForestBenchmark.cpp compile errors.

Shorten comment and add period.

Feb 27 2021, 5:02 PM · Restricted Project, Restricted Project
poelmanc requested review of D97620: Fix DecisionForestBenchmark.cpp compile errors.
Feb 27 2021, 4:55 PM · Restricted Project, Restricted Project

Feb 26 2021

poelmanc added a comment to D96744: clang-format IncludeBlocks: Regroup determination of "main" for framework-style includes fix.

Does this need to be an option?

It's easy to add an option, but there are already two main include-related options, so before adding a third I wanted to give this some thought. As someone new to IncludeCategories, I was genuinely impressed at how easy it was to use and how it gave me such complete control over the grouping via regular expressions. Yet in comparison the determination of main headers was less clear and more hard-coded; I had to examine the source code to figure out that the comparison is case-insensitive, it doesn't consider <> includes, only file stems are considered (e.g. the foo/bar in foo/bar/baz.h is ignored), and the behaviors of IncludeIsMainSourceRegex and IncludeIsMainRegex were a bit murky.

Feb 26 2021, 12:46 PM · Restricted Project, Restricted Project, Restricted Project

Feb 16 2021

poelmanc added a comment to D96744: clang-format IncludeBlocks: Regroup determination of "main" for framework-style includes fix.

Thanks for the speedy reply and the great tool. With appropriate Regex and Priority settings, IncludeCategories worked flawlessly for me aside from not identifying the main header. Treating #include "foo/bar/baz.h" as the main header in file bar/random/baz.h seems like a bug, but I certainly see the dangers of changing current <> behavior. I also considered treating <> includes as main headers only if they also contain a forward-slash, e.g.:

if (!IncludeName.startswith("\"") && !IncludeName.contains("/"))
  return false;

That would resolve the <string.h> case, although #include <sys/types.h> in a file anything/sys/types.h would be identified as the main header. So making an option seems like the cleanest solution. Say, bool IncludeIsMainAllowBraces?

Feb 16 2021, 1:53 AM · Restricted Project, Restricted Project, Restricted Project

Feb 15 2021

poelmanc requested review of D96744: clang-format IncludeBlocks: Regroup determination of "main" for framework-style includes fix.
Feb 15 2021, 7:41 PM · Restricted Project, Restricted Project, Restricted Project

Feb 8 2021

poelmanc updated the diff for D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Capitalize IsOrHasDescendant, add } // namespace std per HarborMaster feedback.

Feb 8 2021, 11:55 PM · Restricted Project, Restricted Project
poelmanc added a comment to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

It looks like premerge tests skipped your last diff with id 321451 (https://reviews.llvm.org/harbormaster/?after=87950). You can re-trigger by uploading a new diff, in the meantime i would also file a bug to https://github.com/google/llvm-premerge-checks/issues. mentioning your diff id.

Thanks @kadircet, actually just reviewing the current bug list explained the problem: https://github.com/google/llvm-premerge-checks/issues/263, Build is not triggered if diff repository is not set. I wasn't selecting rG LLVM Github Monorepo per instructions at https://llvm.org/docs/Phabricator.html that said Leave the Repository field blank.

Feb 8 2021, 9:04 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Call llvm::Annotation constructor rather than operator= to fix linux build issue, fix some issues identified by clang-format and clang-tidy.

Feb 8 2021, 6:44 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Thanks for your patience, this one should work, as I used my normal git show HEAD -U999999 workflow.

Feb 8 2021, 5:49 PM · Restricted Project, Restricted Project
poelmanc added a comment to D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

LGTM, Same as last time for the commit?

Feb 8 2021, 8:32 AM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Comment change, "beginning" to "start" for consistency, being sure to set Repository on "diff" page (not just on edit page) to see if https://github.com/google/llvm-premerge-checks/issues/263 was the problem.

Feb 8 2021, 7:46 AM · Restricted Project, Restricted Project
poelmanc set the repository for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank to rG LLVM Github Monorepo.
Feb 8 2021, 7:43 AM · Restricted Project, Restricted Project

Feb 7 2021

poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Update one one comment, hopefully trigger HarborMaster to rerun.

Feb 7 2021, 10:51 PM · Restricted Project, Restricted Project
poelmanc added a comment to D95771: [clang-tidy] fix modernize-loop-convert to retain needed array-like operator[].

Should this be committed using poelmanc <cpllvm@stellarscience.com>?

That or Conrad Poelman <cpllvm@stellarscience.com> would be great, thanks.

I committed using the email provided but its not attributed you on github. It's attributed you here though. is that email not linked to your github account?

Thanks for the commit, and thanks for the heads-up! I've now added the address in my github account.

Feb 7 2021, 12:05 PM · Restricted Project, Restricted Project

Feb 6 2021

poelmanc added a comment to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

On 2 Feb Harbormaster found a bug from my switching some char* code to use StringRef. I uploaded a new patch on 4 Feb, but Harbormaster does not appear to have rerun. What triggers Harbormaster - do I need to take some action? I haven't been able to find such options myself.

Feb 6 2021, 6:30 PM · Restricted Project, Restricted Project
poelmanc added a comment to D95771: [clang-tidy] fix modernize-loop-convert to retain needed array-like operator[].

Should this be committed using poelmanc <cpllvm@stellarscience.com>?

That or Conrad Poelman <cpllvm@stellarscience.com> would be great, thanks.

Feb 6 2021, 6:04 PM · Restricted Project, Restricted Project

Feb 4 2021

poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Change loop end condition in findLineEnd and add several assert statements.

Feb 4 2021, 8:36 AM · Restricted Project, Restricted Project

Feb 3 2021

poelmanc added a comment to D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Can I ask if you could tidy the description of this, basically remove all the stuff about hasGrandparent etc, probably best just remove everything after result = (a1 nullptr a2); in the desc. It shows in the commit message and its not strictly relevant.

Thanks, done. I never thought about all that showing up in the commit message, I'll be more concise.

Feb 3 2021, 4:45 PM · Restricted Project, Restricted Project
poelmanc updated the summary of D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.
Feb 3 2021, 4:43 PM · Restricted Project, Restricted Project
poelmanc added a comment to D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Thanks for all the great feedback I received here. To give credit where credit's due, this updated revision to UseNullptrCheck.cpp is now actually 100% @steveire's suggested code. Even one of the tests cases was his. Whenever it's ready to land I'd appreciate it if someone could push it as I lack llvm-project commit access.

Feb 3 2021, 3:57 PM · Restricted Project, Restricted Project
poelmanc added a comment to D95771: [clang-tidy] fix modernize-loop-convert to retain needed array-like operator[].

@njames93 Thanks for the review and for accepting this revision. I lack llvm-project commit access so if it's good to go I would greatly appreciate it if you or someone could push this whenever you have have a chance. Thanks!

Feb 3 2021, 3:45 PM · Restricted Project, Restricted Project

Feb 2 2021

poelmanc updated the diff for D95771: [clang-tidy] fix modernize-loop-convert to retain needed array-like operator[].

Fix formatting, add suggested test case (which works.)

Feb 2 2021, 12:34 AM · Restricted Project, Restricted Project

Feb 1 2021

poelmanc added inline comments to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.
Feb 1 2021, 11:10 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Glad to be back after a year away from clang-tidy, and sorry to have let this patch linger. Running clang-tidy over a large codebase shows this patch still to be needed. I believe I've addressed all identified issues but welcome feedback.

Feb 1 2021, 10:41 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Add period to end of comment.

Feb 1 2021, 5:13 PM · Restricted Project, Restricted Project
poelmanc added a comment to D95725: clang-extra: fix incorrect use of std::lock_guard by adding variable name (identified by MSVC [[nodiscard]] error).

s/Guard/Lock/! I don't have commit access so appreciate a push.

Feb 1 2021, 2:03 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D95725: clang-extra: fix incorrect use of std::lock_guard by adding variable name (identified by MSVC [[nodiscard]] error).

Change Guard to Lock.

Feb 1 2021, 2:02 PM · Restricted Project, Restricted Project

Jan 31 2021

poelmanc requested review of D95771: [clang-tidy] fix modernize-loop-convert to retain needed array-like operator[].
Jan 31 2021, 10:24 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Thanks @steveire, that suggestion worked perfectly! I added the additional test case and shortened the mimicked strong_ordering code to a version from clang/unittests.ASTMatchers/ASTMatchersTraversalTest.cpp, but also manually tested this using both MSVC's and libstdc++v3's <compare> header.

Jan 31 2021, 9:52 PM · Restricted Project, Restricted Project
poelmanc added a comment to D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

This does highlight an issue where the mimicked std library stubs used for tests don't correspond exactly to what the stdlib actually looks like and can result in subtly different ASTs that have added/removed implicit nodes.

Going a little off point here but a few months back I pushed a fix for another check that passed its tests.
However the bug report was re-opened as the bug was still observable in the real word.
Turned out the implementation of std::string used for the test had a trivial destructor resulting in the AST not needed to emit CXXBindTemporaryExprs all over the place, which threw off the matching logic.

Unfortunately this kind of disparity is hard to detect in tests so it may be wise to test this locally using the compare header from a real standard library implementation (preferably all 3 main ones if you have the machines) and see if this behaviour is correct.

Jan 31 2021, 1:46 PM · Restricted Project, Restricted Project

Jan 30 2021

poelmanc added a comment to D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

@njames93 Thank you so much for the quick feedback, I made your suggested changes and added a test that it properly converts result = (a1 > (ptr == 0 ? a1 : a2)); to result = (a1 > (ptr == nullptr ? a1 : a2)); now.

Jan 30 2021, 4:37 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Thanks to the great feedback, changed unless(hasAncestor(cxxRewrittenBinaryOperator())) to unless(hasParent(expr(hasParent(cxxRewrittenBinaryOperator())))) and added a test to verify the improvement (and removed an extraneous comment.)

Jan 30 2021, 4:27 PM · Restricted Project, Restricted Project

Jan 29 2021

poelmanc updated the diff for D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.

Fix test failure in modernize-use-nullptr-cxx20.cpp by replacing #include <compare> with some minimal equivalent std code.

Jan 29 2021, 11:48 PM · Restricted Project, Restricted Project
poelmanc requested review of D95725: clang-extra: fix incorrect use of std::lock_guard by adding variable name (identified by MSVC [[nodiscard]] error).
Jan 29 2021, 8:31 PM · Restricted Project, Restricted Project
poelmanc requested review of D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons.
Jan 29 2021, 7:21 PM · Restricted Project, Restricted Project

Dec 22 2019

poelmanc added inline comments to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.
Dec 22 2019, 9:00 PM · Restricted Project, Restricted Project
poelmanc added a comment to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

ToolingTests/ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved also seems to be failing, you can run it with ninja ToolingTests && ./tools/clang/unittests/Tooling/ToolingTests --gtest_filter="ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved"

Dec 22 2019, 9:00 PM · Restricted Project, Restricted Project
poelmanc added inline comments to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.
Dec 22 2019, 8:51 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Fix algorithm to fix failing ToolingTests/ApplyAtomicChangesTest.FormatsCorrectLineWhenHeaderIsRemoved. That test revealed a very specific but real failure case: if existing Removals removed all whitespace including the line's ending newline, and the subsequent line contained only a newline, it would delete the subsequent newline too, which was not desired. Added a new test CleanUpReplacementsTest.RemoveLineAndNewlineLineButNotNext to explicitly test this case.

Dec 22 2019, 8:32 PM · Restricted Project, Restricted Project

Dec 21 2019

poelmanc updated the diff for D70270: clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef.

Update patch to rebase on latest: Changed SourceLocation::contains to SourceLocation::fullyContains, and removed new SourceLocation comparison operators since coincidentally @sammccall added them just days ago.

Dec 21 2019, 11:29 PM · Restricted Project, Restricted Project

Dec 20 2019

poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Address most of the feedback, I'll comment individually.

Dec 20 2019, 1:58 PM · Restricted Project, Restricted Project
poelmanc added a comment to D70270: clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef.

Any further feedback or thoughts on this?

Dec 20 2019, 1:58 PM · Restricted Project, Restricted Project

Nov 20 2019

poelmanc added a comment to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

also could you rename the revision so that it reflects the fact that, this is a change to clang-format and has nothing to do with clang-tidy ?

Done but feel free to suggest a better title.

Nov 20 2019, 11:35 PM · Restricted Project, Restricted Project
poelmanc added inline comments to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.
Nov 20 2019, 11:35 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

I addressed the latest code review comments, added tests to clang/unittests/Format/CleanupTest.cpp, and updated numerous tests to reflect improved removal of blank lines.

Nov 20 2019, 11:25 PM · Restricted Project, Restricted Project

Nov 19 2019

poelmanc updated subscribers of D70144: clang-tidy: modernize-use-equals-default avoid adding redundant semicolons.

Just a quick update, I made some progress addressing the architectural limitation of AnnotatedLines and evaluate(). I changed the AnnotatedLines constructor to also take a Line *PrevAnnotatedLine argument and set Line->First->Prev to the Last of the previous line and Line->Last->Next to the First of the next line. So now the Tokens remain connected across lines, so individual TokenAnalyzer subclasses can easily peek ahead and behind the current changed Line if they wish. Places that previously iterated e.g. while(Tok) had to be changed to iterate while(Tok && Tok != Line->Last->Next) - I probably haven't found all of those places yet.

Nov 19 2019, 3:18 PM · Restricted Project

Nov 18 2019

poelmanc added a comment to D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

Thanks @lebedev.ri for taking the time to think this through and reply. All that makes sense, so I've changed the default to 0.

Nov 18 2019, 3:22 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

Switch default to 0. Add Release Note with some detail to increase the chances of someone finding this with an Internet search on the error message.

Nov 18 2019, 3:16 PM · Restricted Project, Restricted Project

Nov 16 2019

poelmanc updated subscribers of D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

Exactly due to the issue you are fixing here, we ended up disabling the complete check because we didn't want to live with the warnings it produced on -Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or even break their build (with -Werror), they might not look into check options, but just disable the check directly.

Just pinging to see if anyone has any thoughts on moving forward with this. Thanks in advance for any feedback!

Nov 16 2019, 12:35 AM · Restricted Project, Restricted Project

Nov 15 2019

poelmanc added a comment to D70270: clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef.

If there's a way to match only CXXRecordDecls that are immediately followed by a TypedefDecl...

Alternatively, within check() when we get the TypedefDecl, is there any way to navigate up the AST to find its immediately preceding sibling node in the AST and check whether it's a CXXRecordDecl? If so we could eliminate Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this); altogether. I didn't see a way to do that though.

Nov 15 2019, 9:11 PM · Restricted Project, Restricted Project
poelmanc added inline comments to D70270: clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef.
Nov 15 2019, 8:45 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D70270: clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef.
Nov 15 2019, 8:36 PM · Restricted Project, Restricted Project

Nov 14 2019

poelmanc added a comment to D70144: clang-tidy: modernize-use-equals-default avoid adding redundant semicolons.

! In D70144#1745583, @JonasToth wrote:

! In D70144#1745532, @malcolm.parsons wrote:

Should clang::format::cleanupAroundReplacements() handle this instead?

My initial attempt did not go well. I thought perhaps adding:

cleanupLeft(Line->First, tok::semi, tok::semi);

to clang/lib/Format/Format.cpp:1491 might do the trick. Stepping through that in the debugger shows that cleanupPair iterates over tokens on affected lines over the affected range. But after the newly added default token and subsequent semi token comes a nullptr - I could not see how to peek past the default; to see what else is on the line.

Nov 14 2019, 9:08 PM · Restricted Project
poelmanc added a comment to D67460: clang-tidy: modernize-use-using work with multi-argument templates.

I'm a bit worried that this manual parsing technique will need fixing again in the future, but I think this is at least a reasonable incremental improvement.

Thanks and I agree. Your comments encouraged me to take another stab at improving things. See D70270, a whole new approach that removes manual parsing in favor of AST processing and successfully converts many more cases from typedef to using.

Nov 14 2019, 1:29 PM · Restricted Project, Restricted Project
poelmanc created D70270: clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef.
Nov 14 2019, 1:20 PM · Restricted Project, Restricted Project
poelmanc added a comment to D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal.

While I have no objections against this patch, I wonder whether someone had a chance to ask GCC developers about this? Is it a conscious choice to suggest override when final is present? What's the argument for doing so?

Thanks, someone should ask them as I believe this issue extends beyond clang-tidy: code with functions marked final cannot satisfy both gcc -Wsuggest-override and clang -Winconsistent-missing-override; gcc demands override final and clang demands just final.

Nov 14 2019, 1:02 PM · Restricted Project
poelmanc added a comment to D70144: clang-tidy: modernize-use-equals-default avoid adding redundant semicolons.

Should clang::format::cleanupAroundReplacements() handle this instead?

Of yes. Totally forgot that. That would probably the most elegant way :)

Interesting, so is the advantage of that approach that any fixit Replacement or Insertion that ends with a semicolon would have it removed if a semicolon already immediately follows it? That makes sense - one less thing for individual check developers to worry about.

Nov 14 2019, 9:02 AM · Restricted Project

Nov 13 2019

poelmanc added a comment to D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal.

LGTM!
Did you check on a real code-base that suffers from the issue, if that works as expected?

Thanks! I have now run it on our real code base and it worked as expected.

Nov 13 2019, 4:50 PM · Restricted Project
poelmanc updated the diff for D70144: clang-tidy: modernize-use-equals-default avoid adding redundant semicolons.

Change to add just one helper function findNextTokenSkippingComments to LexerUtils.h, requiring no change to Token::isOneOf, and properly call it from UseEqualsDefaultCheck.cpp.

Nov 13 2019, 4:40 PM · Restricted Project
poelmanc updated the diff for D70144: clang-tidy: modernize-use-equals-default avoid adding redundant semicolons.

Update to add and use new findNextTokenSkippingComments and findNextTokenSkippingKind utility functions. Since the former calls the latter with just one token type, this required generalizing Token::isOneOf() to work with 1-to-N token kinds versus requiring 2-to-N.

Nov 13 2019, 12:47 PM · Restricted Project
poelmanc added inline comments to D70144: clang-tidy: modernize-use-equals-default avoid adding redundant semicolons.
Nov 13 2019, 8:09 AM · Restricted Project

Nov 12 2019

poelmanc updated the summary of D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal.
Nov 12 2019, 11:29 PM · Restricted Project
poelmanc updated the summary of D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal.
Nov 12 2019, 11:29 PM · Restricted Project
poelmanc created D70165: clang-tidy: modernize-use-override new option AllowOverrideAndFinal.
Nov 12 2019, 11:20 PM · Restricted Project
poelmanc created D70144: clang-tidy: modernize-use-equals-default avoid adding redundant semicolons.
Nov 12 2019, 2:03 PM · Restricted Project

Nov 11 2019

poelmanc added inline comments to D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.
Nov 11 2019, 7:03 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

Move isWhitespace and skipNewlines to clang/Basic/CharInfo.h (renaming to isWhitespaceStringRef and skipNewlinesChars to resolve name clashes) and add double-quotes around "/n" and "/r" in comments.

Nov 11 2019, 6:45 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix.

Make requested fixes to documentation.

Nov 11 2019, 2:01 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.

Thanks! Do you have commit access or do you need me to commit for you?

Nov 11 2019, 1:52 PM · Restricted Project, Restricted Project
poelmanc added a comment to D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Done, thanks.

Nov 11 2019, 1:52 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Changed post-increment/decrement to pre-increment/decrement.

Nov 11 2019, 1:44 PM · Restricted Project, Restricted Project

Nov 8 2019

poelmanc updated the diff for D68682: format::cleanupAroundReplacements removes whole line when Removals leave previously non-blank line blank.

I just rebased this patch on the latest master. I believe I've addressed all the comments raised so far. Should I add mention of this change to the release notes?

Nov 8 2019, 9:57 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.
In D69238#1739627, @NoQ wrote:

Wow. That makes this so much easier... Thank you so much!

Nov 8 2019, 9:50 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.
Nov 8 2019, 9:39 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

I like that this check warns about copy constructors that don't copy.
The warning can be suppressed with a NOLINT comment if not copying is intentional.

Thanks for the comment! It's not clear to me how you suggest proceeding though.

Nov 8 2019, 5:54 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.

If it is indeed the extra AST node for the elidable constructor, see if you can use the ignoringElidableConstructorCall AST matcher to ignore it, therefore smoothing over AST differences between language modes.

Nov 8 2019, 5:17 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix.

Now allows namespaces on types and defaults to ::std::basic_string as requested. The code uses namespaced string type names to check types, and uses non-namespaced string type names to check for the required one-argument or two-argument-defaulted constructors.

Nov 8 2019, 4:41 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.
In D69238#1736365, @NoQ wrote:

I suspect that it's not just the source range, but the whole AST for the initializer is different, due to C++17 mandatory copy elision in the equals-initializer syntax. Like, before C++17 it was a temporary constructor, a temporary materialization (ironic!), and a copy constructor, but in C++17 and after it's a single direct constructor which looks exactly like the old temporary constructor (except not being a CXXTemporaryObjectExpr). You're most likely talking about different construct-expressions in different language modes.

That said, it should probably be possible to change the source range anyway somehow.

Thanks to all the encouragement here, I spent a few more hours stepping through code and have found a one-line change to clang\lib\Sema\SemaInit.cpp:8053 that fixes this bug! Change:

SourceLocation Loc = CurInit.get()->getBeginLoc();

to

SourceLocation Loc = Kind.getLocation();

For SK_UserConversion, CurInit is set at SemaInit.cpp:7899 to Args[0], i.e. the first argument to the constructor, which is "" in this case. By changing Loc to Kind.getLocation(), the BuildCXXConstructExpr at SemaInit.cpp:8064 gets created with a SourceRange spanning a = "". With just that change, the SourceRange for an expression like std::string a = "" becomes consistent across C++11/14/17/2a and readability-redundant-string-init tests pass in all C++ modes (so we can throw away my 70 lines of manual parsing code.)

Nov 8 2019, 2:15 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.

Just documenting here that I sent the following email to cfe-dev@lists.llvm.org:

Nov 8 2019, 2:06 PM · Restricted Project, Restricted Project
poelmanc added a comment to D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Thanks @aaron.ballman, I don't have commit access so will someone else commit this?

Nov 8 2019, 10:17 AM · Restricted Project, Restricted Project

Oct 29 2019

poelmanc added inline comments to D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix.
Oct 29 2019, 4:16 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix.

@aaron.ballman Thanks for the hasAnyName feedback! From the name internal::VariadicFunction I assumed arguments were needed at compile-time, but thanks to your suggestion I found the overload taking ArrayRef<ArgT>.

Oct 29 2019, 4:06 PM · Restricted Project, Restricted Project
poelmanc updated the diff for D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix.

Added release notes, fixed backticks in documentation, removed blank line, removed new hasListedName matcher and used existing hasAnyName matcher.

Oct 29 2019, 3:56 PM · Restricted Project, Restricted Project
poelmanc added inline comments to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.
Oct 29 2019, 2:14 PM · Restricted Project, Restricted Project
MyDeveloperDay awarded D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix a Like token.
Oct 29 2019, 5:23 AM · Restricted Project, Restricted Project
poelmanc updated the diff for D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

Changed default to 1, updated help accordingly, and removed an extraneous set of braces around a one-line statement.

Oct 29 2019, 12:30 AM · Restricted Project, Restricted Project

Oct 28 2019

poelmanc added inline comments to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.
Oct 28 2019, 10:29 PM · Restricted Project, Restricted Project
poelmanc created D69548: Give clang-tidy readability-redundant-string-init a customizable list of string types to fix.
Oct 28 2019, 10:27 PM · Restricted Project, Restricted Project
poelmanc added a comment to D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.

Thanks for the feedback, the new patch removes the extra const, clang::, and braces on one-line if statements, and now explains the regex in readability-redundant-string-init.cpp.

Oct 28 2019, 10:14 AM · Restricted Project, Restricted Project
poelmanc updated the diff for D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a.

Remove extra const, braces for one-line if statements, and clang::. Added a comment explaining the need for a regex on a warning test line.

Oct 28 2019, 10:11 AM · Restricted Project, Restricted Project

Oct 24 2019

poelmanc added a comment to D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

What do @malcolm.parsons, @alexfh, @hokein, @aaron.ballman, @lebedev.ri think of @mgehre's suggestion to enable IgnoreBaseInCopyConstructors as the default setting, so gcc users won't experience build errors and think "clang-tidy broke my code!"

Oct 24 2019, 11:55 AM · Restricted Project, Restricted Project

Oct 21 2019

poelmanc updated the diff for D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

Rebase to latest master (tests moved into new "checkers" directory.)

Oct 21 2019, 9:01 AM · Restricted Project, Restricted Project
poelmanc added a comment to D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra.

Addressed these the other day but failed to check the "Done" boxes. Done!

Oct 21 2019, 9:01 AM · Restricted Project, Restricted Project
poelmanc added a comment to D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Checked "Done". (I addressed @jonathanmeier's comment feedback with a previous update but forgot to check the box!)

Oct 21 2019, 8:51 AM · Restricted Project, Restricted Project
poelmanc updated the diff for D67460: clang-tidy: modernize-use-using work with multi-argument templates.

Rebase to latest master (tests moved into new "checkers" subdirectory.)

Oct 21 2019, 8:51 AM · Restricted Project, Restricted Project