This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho][nfc] Minor refactoring + clang-tidy fixes
ClosedPublic

Authored by oontvoo on Apr 6 2021, 10:53 PM.

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rGffc65824f0ee: [lld-macho][nfc] Minor refactoring + clang-tidy fixes
Summary
  • use "empty()" instead of "size()"
  • refactor the re-export code so it doesn't create a new vector every time.

Diff Detail

Event Timeline

oontvoo created this revision.Apr 6 2021, 10:53 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Apr 6 2021, 10:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 10:53 PM
int3 accepted this revision.Apr 7 2021, 8:31 AM

I actually snuck in a clang-format cleanup yesterday :) https://github.com/llvm/llvm-project/commit/174deb0539ee4af7d20bfead9f73055782e878e3 (so just update the commit title I guess)

lld/MachO/Driver.cpp
1064–1067
  1. no need for braces
  2. I'm pretty sure the compiler can hoist out the construction of the vector
This revision is now accepted and ready to land.Apr 7 2021, 8:31 AM
int3 added inline comments.Apr 7 2021, 8:53 AM
lld/MachO/Driver.cpp
1064–1067

I'm pretty sure the compiler can hoist out the construction of the vector

well, I'm wrong: https://godbolt.org/z/szKbeojrd

oontvoo updated this revision to Diff 335852.Apr 7 2021, 9:53 AM
oontvoo marked an inline comment as done.

removed unnecessary braces

I actually snuck in a clang-format cleanup yesterday :) https://github.com/llvm/llvm-project/commit/174deb0539ee4af7d20bfead9f73055782e878e3 (so just update the commit title I guess)

These are clang-tidy's suggested fixes (the size() vs empty()) - not clang-format :)

lld/MachO/Driver.cpp
1064–1067

I'm pretty sure the compiler can hoist out the construction of the vector

well, I'm wrong: https://godbolt.org/z/szKbeojrd

Probably because it can't know what foo does with the reference - hoisting it out could change the logic entirely:

void foo(const std::vector<int>& v) {
  static auto* last_vector = &v;
  static bool first = true;
  if (first) first = false;
  else  assert (&v != last_vector);  
}
int3 added inline comments.Apr 7 2021, 10:20 AM
lld/MachO/Driver.cpp
359–361

ahh well this was one of the formatting changes I committed yesterday, hence the tidy/format confusion 😅

1064–1067

hmm true. Though I'm not sure if temporary rvalues are guaranteed to be given a fresh address every time...

oontvoo marked an inline comment as done.Apr 7 2021, 10:57 AM
oontvoo added inline comments.
lld/MachO/Driver.cpp
359–361

Done - sync'ing to head removed this change.

This revision was landed with ongoing or failed builds.Apr 7 2021, 10:58 AM
This revision was automatically updated to reflect the committed changes.
oontvoo marked an inline comment as done.