This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] performance-unnecessary-copy-initialization: Do not remove comments on new lines.
ClosedPublic

Authored by flx on Jul 9 2021, 2:00 PM.

Details

Summary

When deleting the copy assignment statement because copied variable is not used
only remove trailing comments on the same line.

Diff Detail

Event Timeline

flx created this revision.Jul 9 2021, 2:00 PM
flx requested review of this revision.Jul 9 2021, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 2:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Given that you're removing a declaration, consider using tooling::getAssociatedRange https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/clang/include/clang/Tooling/Transformer/SourceCode.h;l=44 It does have a different semantics than the simpler logic that you have here, but, its based on code in a dead-code removal tool, so it may still be what you want.

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
51

what happens for the last line in the file? I think the Offset + 1 below will be sketchy (because PastNewLine will be invalid). It might work, but seems best avoided.

56

I suspect this could get tripped up by macros and other expansions. Instead of std::min, consider using SourceManager::isBeforeInTranslationUnit
(clang/include/clang/Basic/SourceManager.h;l=1616)? Alternatively, check that both locations are file ids before calling std::min.

flx updated this revision to Diff 357993.Jul 12 2021, 10:59 AM

Make offset operations safer.

flx marked 2 inline comments as done.Jul 12 2021, 11:01 AM

Thanks for the review!

clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
51

I added a check to not go past the end of the string.

56

We don't apply any fixes when the decl statement is inside of a macro, but switched to using SourceManager::isBeforeInTranslationUnit.

ymandel accepted this revision.Jul 12 2021, 11:11 AM
ymandel added inline comments.
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
50

optional nit: invert the condition and the branches. i just find it a little easier to read positive predicates. So, when i have both branches, I try for the condition to be positive (or, in other words, I try not to not the condition. ;) )

This revision is now accepted and ready to land.Jul 12 2021, 11:11 AM
flx updated this revision to Diff 358010.Jul 12 2021, 11:40 AM
flx marked 2 inline comments as done.

Invert end of string logic.

flx marked an inline comment as done.Jul 12 2021, 11:41 AM