This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add readability-reference-to-constructed-temporary check
ClosedPublic

Authored by PiotrZSL on Mar 18 2023, 1:55 PM.

Details

Summary

Detects code where a temporary object is directly constructed by calling
a constructor or using an initializer list and immediately assigned to a
reference variable.

Diff Detail

Event Timeline

PiotrZSL created this revision.Mar 18 2023, 1:55 PM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL requested review of this revision.Mar 18 2023, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2023, 1:55 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

There are 2 issues found in llvm repository:

  • llvm/lib/Target/AMDGPU/AMDGPULibFunc.cpp:721:22: warning: reference variable 'S' extends the lifetime of a just-constructed temporary object 'const StringRef', consider changing reference to value [readability-reference-to-constructed-temporary]
  • clang/lib/Sema/SemaChecking.cpp:10096:36: warning: reference variable 'AT2' extends the lifetime of a just-constructed temporary object 'const analyze_printf::ArgType', consider changing reference to value [readability-reference-to-constructed-temporary]
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp
22

const auto *.

26

Ditto.

clang-tools-extra/test/clang-tidy/checkers/readability/reference-to-constructed-temporary.cpp
31

Excessive newline.

PiotrZSL updated this revision to Diff 506399.Mar 19 2023, 8:50 AM
PiotrZSL marked 3 inline comments as done.

Review comment fixes

PiotrZSL updated this revision to Diff 513693.Apr 14 2023, 11:39 AM

Ping + Rebase

xgupta added a subscriber: xgupta.Jul 25 2023, 1:41 AM
xgupta added inline comments.
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp
21

A comment might be good to describe this struct.

23

This can be written as const auto *Other = Nodes.getNodeAs<ValueDecl>(ID); following other patterns.

40

Can be written using direct initialization as NotExtendedByDeclBoundToPredicate Predicate{ID};

64

This part looks like boilerplate in the middle of a specific implementation., isLanguageVersionSupported is mostly in the header file.

70

This part look like boilerplate in the middle of a specific implementation., getCheckTraversalKind() is mostly in the header file.

clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst
11

May be good to mention dangling references and resource leakage as potential issues.

19

The below comment is not matching, do you want to write -
const std::string& str = std::string("hello");
?

PiotrZSL added inline comments.Jul 25 2023, 4:14 AM
clang-tools-extra/clang-tidy/readability/ReferenceToConstructedTemporaryCheck.cpp
40

Yes, but I followed like others are done.

clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst
19

actually comment is +- fine, constructor to std::string will be called anyway, and for compiler I think those 2 are equal.
Will verify.

PiotrZSL marked 4 inline comments as done.Jul 25 2023, 11:40 AM
PiotrZSL added inline comments.
clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst
11

There will be no dangling references and resource leakage because lifetime of temporary is extended by variable.

PiotrZSL updated this revision to Diff 544057.Jul 25 2023, 11:40 AM
PiotrZSL marked an inline comment as done.

Update

xgupta added inline comments.Jul 25 2023, 7:08 PM
clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst
19

but you have written below reference variable str is assigned a temporary object and there is no str variable.

PiotrZSL marked 2 inline comments as done.Jul 25 2023, 10:08 PM
PiotrZSL added inline comments.
clang-tools-extra/docs/clang-tidy/checks/readability/reference-to-constructed-temporary.rst
19

Ok, that's actually a bug. It's value.

PiotrZSL updated this revision to Diff 544205.Jul 25 2023, 10:08 PM
PiotrZSL marked an inline comment as done.

Rename value to str in doc.

This LGTM, I will wait for two days before accepting the revision in case other reviewers might have some more comments/suggestions.

xgupta accepted this revision.Jul 27 2023, 9:43 PM

LGTM!

This revision is now accepted and ready to land.Jul 27 2023, 9:43 PM