- 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!
Mar 1 2021
Fixed comment as requested (keys()->size().) I don't have commit access so if you can push it that would be great, thanks!
Feb 28 2021
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 27 2021
Shorten comment and add period.
Feb 26 2021
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 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 15 2021
Feb 8 2021
Capitalize IsOrHasDescendant, add } // namespace std per HarborMaster feedback.
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.
Call llvm::Annotation constructor rather than operator= to fix linux build issue, fix some issues identified by clang-format and clang-tidy.
Thanks for your patience, this one should work, as I used my normal git show HEAD -U999999 workflow.
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 7 2021
Update one one comment, hopefully trigger HarborMaster to rerun.
Thanks for the commit, and thanks for the heads-up! I've now added the address in my github account.
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.
That or Conrad Poelman <email@example.com> would be great, thanks.
Feb 4 2021
Change loop end condition in findLineEnd and add several assert statements.
Feb 3 2021
Thanks, done. I never thought about all that showing up in the commit message, I'll be more concise.
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.
@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 2 2021
Fix formatting, add suggested test case (which works.)
Feb 1 2021
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.
Add period to end of comment.
s/Guard/Lock/! I don't have commit access so appreciate a push.
Change Guard to Lock.
Jan 31 2021
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 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.
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 29 2021
Fix test failure in modernize-use-nullptr-cxx20.cpp by replacing #include <compare> with some minimal equivalent std code.
Dec 22 2019
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 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 20 2019
Address most of the feedback, I'll comment individually.
Any further feedback or thoughts on this?
Nov 20 2019
Done but feel free to suggest a better title.
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 19 2019
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 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.
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 16 2019
Just pinging to see if anyone has any thoughts on moving forward with this. Thanks in advance for any feedback!
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 14 2019
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.
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.
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.
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 13 2019
Thanks! I have now run it on our real code base and it worked as expected.
Change to add just one helper function findNextTokenSkippingComments to LexerUtils.h, requiring no change to Token::isOneOf, and properly call it from UseEqualsDefaultCheck.cpp.
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 12 2019
Nov 11 2019
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.
Make requested fixes to documentation.
Changed post-increment/decrement to pre-increment/decrement.
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?
Wow. That makes this so much easier... Thank you so much!
Thanks for the comment! It's not clear to me how you suggest proceeding though.
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.
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();
SourceLocation Loc = Kind.getLocation();
For SK_UserConversion, CurInit is set at SemaInit.cpp:7899 to Args, 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.)
Just documenting here that I sent the following email to firstname.lastname@example.org:
Thanks @aaron.ballman, I don't have commit access so will someone else commit this?
Oct 29 2019
@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>.
Added release notes, fixed backticks in documentation, removed blank line, removed new hasListedName matcher and used existing hasAnyName matcher.
Changed default to 1, updated help accordingly, and removed an extraneous set of braces around a one-line statement.
Oct 28 2019
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.
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 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 21 2019
Rebase to latest master (tests moved into new "checkers" directory.)
Addressed these the other day but failed to check the "Done" boxes. Done!
Checked "Done". (I addressed @jonathanmeier's comment feedback with a previous update but forgot to check the box!)