# Projects

User does not belong to any projects.

# User Details

User Since
Sep 11 2019, 11:23 AM (146 w, 6 d)

# Feb 4 2022

Sorry I somehow missed the acceptance back on 10 Jan. I lack commit access, it would be great if you could push for me, thanks!

Feb 4 2022, 10:55 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

# 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

Feb 28 2021, 12:55 PM · Restricted Project, Restricted Project

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
Feb 28 2021, 12:41 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

# Feb 27 2021

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

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

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

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

Feb 15 2021, 7:41 PM · Restricted Project, Restricted Project, Restricted Project

# Feb 8 2021

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

Feb 8 2021, 11:55 PM · Restricted Project, Restricted Project

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

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

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

LGTM, Same as last time for the commit?

Feb 8 2021, 8:32 AM · Restricted Project, Restricted Project

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
Feb 8 2021, 7:43 AM · Restricted Project, Restricted Project

# Feb 7 2021

Update one one comment, hopefully trigger HarborMaster to rerun.

Feb 7 2021, 10:51 PM · Restricted Project, Restricted Project

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

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

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

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

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

# Feb 3 2021

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
Feb 3 2021, 4:43 PM · Restricted Project, Restricted Project

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

@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

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

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

# Feb 1 2021

Feb 1 2021, 11:10 PM · Restricted Project, Restricted Project

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

Add period to end of comment.

Feb 1 2021, 5:13 PM · Restricted Project, Restricted Project

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

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

Change Guard to Lock.

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

# Jan 31 2021

Jan 31 2021, 10:24 PM · Restricted Project, Restricted Project

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

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

@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

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

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
Jan 29 2021, 8:31 PM · Restricted Project, Restricted Project
Jan 29 2021, 7:21 PM · Restricted Project, Restricted Project

# Dec 22 2019

Dec 22 2019, 9:00 PM · Restricted Project, Restricted Project

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
Dec 22 2019, 8:51 PM · Restricted Project, Restricted Project

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

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

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

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

Any further feedback or thoughts on this?

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

# Nov 20 2019

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
Nov 20 2019, 11:35 PM · Restricted Project, Restricted Project

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

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

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

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

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
Nov 15 2019, 8:45 PM · Restricted Project, Restricted Project
Nov 15 2019, 8:36 PM · Restricted Project, Restricted Project

# Nov 14 2019

! In D70144#1745583, @JonasToth wrote:

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

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

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
Nov 14 2019, 1:20 PM · Restricted Project, Restricted Project

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

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

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

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

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
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
Nov 12 2019, 11:20 PM · Restricted Project
Nov 12 2019, 2:03 PM · Restricted Project

# Nov 11 2019

Nov 11 2019, 7:03 PM · Restricted Project, Restricted Project

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

Make requested fixes to documentation.

Nov 11 2019, 2:01 PM · Restricted Project, Restricted Project

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

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

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

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

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

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

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

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

Oct 29 2019, 4:16 PM · Restricted Project, Restricted Project

@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

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
Oct 29 2019, 2:14 PM · Restricted Project, Restricted Project
Oct 29 2019, 5:23 AM · Restricted Project, Restricted Project

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

Oct 28 2019, 10:29 PM · Restricted Project, Restricted Project
Oct 28 2019, 10:27 PM · Restricted Project, Restricted Project

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

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

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

Oct 21 2019, 9:01 AM · Restricted Project, Restricted Project

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

Oct 21 2019, 9:01 AM · Restricted Project, Restricted Project

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