This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Remove inline Specifier for DefineOutline Tweak
ClosedPublic

Authored by bgluzman on May 23 2023, 10:50 PM.

Details

Summary

inline specifiers should be removed from from the function declaration and
the newly-created implementation.

For example, take the following (working) code:

cpp
// foo.hpp
struct A {
  inline void foo() { std::cout << "hello world\n" << std::flush; }
};

// foo.cpp
#include "foo.hpp"

// main.cpp
#include "foo.hpp"

int main() {
  A a;
  a.foo();
  return 0;
}

// compile: clang++ -std=c++20 main.cpp foo.cpp -o main

After applying the tweak:

// foo.hpp
struct A {
  inline void foo();
};

// foo.cpp
#include "foo.hpp"

inline void A::foo() { std::cout << "hello world\n" << std::flush; }

// main.cpp
#include "foo.hpp"

int main() {
  A a;
  a.foo();
  return 0;
}

// compile: clang++ -std=c++20 main.cpp foo.cpp -o main

We get a link error, as expected:

/usr/bin/ld: /tmp/main-4c5d99.o: in function `main':
main.cpp:(.text+0x14): undefined reference to `A::foo()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This revision removes these specifiers from both the header and the source file. This was identified in Github issue llvm/llvm-project#61295.

Diff Detail

Event Timeline

bgluzman created this revision.May 23 2023, 10:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 10:50 PM
Herald added a subscriber: arphaman. · View Herald Transcript
bgluzman requested review of this revision.May 23 2023, 10:50 PM
bgluzman added inline comments.May 23 2023, 10:58 PM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
136

This was extracted from the DelKeyword lambda in getFunctionSourceCode since it now is used within apply as well.

Note that since this returns a tooling::Replacements, the caller must combine the result with other Replacements via tooling::Replacements::merge. This is my first time contributing to this project, so please let me know if using merge (instead of add) could cause performance issues. I can change this so that we add to a tooling::Replacements out-parameter, instead.

kadircet accepted this revision.May 25 2023, 6:59 AM

thanks, LGTM!

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
136

thanks merge is fine, we execute this code path only when the user tries to apply the code action. hence it's performance is not as critical as prepare.

137

let's just drop the FD from signature, you can get SourceManager from syntax::TokenBuffer.

also can you update the documentation, we should rather say:

Returns replacements to delete tokens with kind `Kind` in the range `FromRange`.

nit: s/getDelKeywordReplacements/deleteTokensWithKind

489

you can directly construct Replacements with this particular Replacement first, that way no need to add and check for Err

This revision is now accepted and ready to land.May 25 2023, 6:59 AM
bgluzman updated this revision to Diff 525715.May 25 2023, 11:30 AM

address diff comments

bgluzman added inline comments.May 25 2023, 11:31 AM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
137

Getting it from syntax::TokenBuffer is a lot nicer, thanks!

489

That's much cleaner than doing this with add, thanks!

bgluzman marked 2 inline comments as not done.May 25 2023, 11:32 AM
bgluzman updated this revision to Diff 525722.May 25 2023, 11:40 AM

resubmit diff update with corrected commit range

Thank you for reviewing @kadircet!

Per the MyFirstTypoFix page, I don’t have commit access. If everything looks good, could you land this patch for me and use “Brian Gluzman bgluzman@gmail.com” for the commit?

This revision was automatically updated to reflect the committed changes.