[PATCH] D65526: [Clangd] Initial prototype version of ExtractFunction

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:211
+  // Note that DRELocType is never OUTSIDECONTEXT.
+  if (DeclLocType + 1 != DRELocType)
+    return;
----------------
SureYeaah wrote:
> sammccall wrote:
> > this line seems very suspect, what are you trying to do and why?
> > 
> This just checks whether (DeclLocType, DRELocType) is either (PRETARGET, TARGET) or (TARGET, POSTTARGET). Could explicitly write it if needed.
Definitely needed, apart from the lack of clarity the current version has UB sometimes (`DeclLocType + 1` might not be a valid value)

But by "why" i mostly mean - this looks like the place for recording the location, not the place for deciding not to record the location because some algorithm later may not need it. Seems a lot simpler to record it unconditionally and worry about it later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65526/new/

https://reviews.llvm.org/D65526